Stephen Gallagher <sgall...@redhat.com> wrote:
> On 08/31/2010 07:57 AM, Jan Zelený wrote:
> > Stephen Gallagher <sgall...@redhat.com> wrote:
> >>>> 0005-Deleted-some-needless-assignments.patch
> >>>> Ticket: #582
> >>>
> >>> I think we should check for potential errors from sdap_id_op_done()
> >>> calls instead of removing the assignments altogether.
> >>
> >> I agree with Jakub. These assignments shouldn't be removed, they should
> >> be tested for failures.
> >
> > Sorry for an extra email, forgot to mention this in the previous one. I
> > don't think this is the case. The result of operation is subsequently
> > checked (it is stored also in dp_error variable). We can check ret value
> > as well as dp_error, but I don't think it will bring any code
> > improvement. The same applies for call of sysdb_attrs_get_string() - its
> > return value isn't checked, but the content of "value" variable is
> > tested.
>
> For the record, clang and Coverity both throw warnings if we do not
> receive and check the return value of any function that has a return
> value. In order to be in compliance with these tools, we need to handle
> this.
>
> Also, our unofficial policy is that the return value needs to be
> accurate (since the returned-by-reference value may not be valid or safe
> to dereference if the return value is a failure).
Both your points are valid. I'm attaching a patch where the check is
performed. I still think it is redundant, but it might be more safe in the
future in case the code around it changes.
--
Jan
From 00f38b7391861999f2d1b90804b0833394b9b022 Mon Sep 17 00:00:00 2001
From: Jan Zeleny <jzel...@redhat.com>
Date: Mon, 30 Aug 2010 15:54:23 +0200
Subject: [PATCH] Cleaned some dead assignments
Two needless assignments were deleted, two were complemented
with code checking function results.
Ticket: #582
---
src/providers/ipa/ipa_access.c | 28 ++++++++++++++--------------
src/providers/ipa/ipa_auth.c | 2 +-
2 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
index 895bd76..e9c5108 100644
--- a/src/providers/ipa/ipa_access.c
+++ b/src/providers/ipa/ipa_access.c
@@ -1852,20 +1852,22 @@ static bool hbac_check_step_result(struct hbac_ctx *hbac_ctx, int ret)
}
ret = sdap_id_op_done(hbac_ctx_sdap_id_op(hbac_ctx), ret, &dp_error);
- if (dp_error == DP_ERR_OFFLINE) {
- /* switching to offline mode */
- talloc_zfree(hbac_ctx->sdap_op);
- dp_error = DP_ERR_OK;
- }
-
- if (dp_error == DP_ERR_OK) {
- /* retry */
- ret = hbac_retry(hbac_ctx);
- if (ret == EOK) {
- return false;
+ if (ret != EOK) {
+ if (dp_error == DP_ERR_OFFLINE) {
+ /* switching to offline mode */
+ talloc_zfree(hbac_ctx->sdap_op);
+ dp_error = DP_ERR_OK;
}
- dp_error = DP_ERR_FATAL;
+ if (dp_error == DP_ERR_OK) {
+ /* retry */
+ ret = hbac_retry(hbac_ctx);
+ if (ret == EOK) {
+ return false;
+ }
+
+ dp_error = DP_ERR_FATAL;
+ }
}
ipa_access_reply(hbac_ctx, PAM_SYSTEM_ERR);
@@ -1923,7 +1925,6 @@ static void hbac_get_host_info_done(struct tevent_req *req)
ipa_hostname = dp_opt_get_cstring(hbac_ctx->ipa_options, IPA_HOSTNAME);
if (ipa_hostname == NULL) {
DEBUG(1, ("Missing ipa_hostname, this should never happen.\n"));
- ret = EINVAL;
goto fail;
}
@@ -1944,7 +1945,6 @@ static void hbac_get_host_info_done(struct tevent_req *req)
}
if (local_hhi == NULL) {
DEBUG(1, ("Missing host info for [%s].\n", ipa_hostname));
- ret = EOK;
pam_status = PAM_PERM_DENIED;
goto fail;
}
diff --git a/src/providers/ipa/ipa_auth.c b/src/providers/ipa/ipa_auth.c
index 2bd92ba..2d91457 100644
--- a/src/providers/ipa/ipa_auth.c
+++ b/src/providers/ipa/ipa_auth.c
@@ -188,7 +188,7 @@ static void get_password_migration_flag_done(struct tevent_req *subreq)
}
ret = sysdb_attrs_get_string(reply[0], IPA_CONFIG_MIRATION_ENABLED, &value);
- if (strcasecmp(value, "true") == 0) {
+ if (ret == EOK && strcasecmp(value, "true") == 0) {
state->password_migration = true;
}
--
1.7.2.1
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
