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