Fixing of nitpicks #65

celestian · 2016-10-25T07:56:18Z

Hello,
there are two simple patches. I found those things during static analysis of SSSD code.
Petr

lslebodn · 2016-10-25T08:02:54Z

src/util/sss_krb5.c

@@ -1104,7 +1104,6 @@ bool sss_krb5_realm_has_proxy(const char *realm)

    kerr = profile_get_values(profile, profile_path, &list);
    if (kerr == PROF_NO_RELATION || kerr == PROF_NO_SECTION) {
-        kerr = 0;
        goto done;


The value of res will be false here.
Should it be true? It's my assumption based on kerr = 0

@jhrozek is author of this code

False is correct, please see the commit message of cbf20090fa92cc9a6e31e3c903b21d020c519367. I don't remember why I used kerr=0 there, maybe in the past we used to return kerr.

hmm,

sh$ $git show cbf20090fa92cc9a6e31e3c903b21d020c519367 tree cbf20090fa92cc9a6e31e3c903b21d020c519367 .git-commit-template .gitignore .tx/ BUILD.txt COPYING Makefile.am README Vagrantfile configure.ac contrib/ m4/ po/ scripts/ src/ version.m4 zanata.xml

lslebodn · 2016-10-25T08:08:23Z

src/responder/common/data_provider/rdp_message.c

+    if (ret != EOK) {
+        /* Something bad happened. Just kill the request. */
+        goto done;
+    }
    if (reply == NULL) {


This change is not really required because reply is initialized to NULL in the function rdp_process_pending_call
The question is whether we need a if (reply == NULL) branch after this change.

I like consistency but It would be good to know why @pbrezina wrote is in such way,
BTW return value of rdp_process_pending_call is checked on line 203 but here the output argument was checked.

I am little confused. It is more about philosophy of code.

I discussed similar issue with some members of sssd team. The question is what is more important if error code or returned values. I show short example:

Let we have function call like:

errno_t ret = my_function(input_argument, &output_data);

Is better to rely on value of ret or on value of output_data?

Our result was if ret != EOK it we should assume that output_data could be corrupted.

I understand that this comment is not strictly connected to recent issue. But, I would like to ask: "Do we have genral rule for those situations?"

I don't insist on the patch RESPONDER: Adding of return value checking.

jhrozek · 2016-10-27T15:45:42Z

On Tue, Oct 25, 2016 at 09:28:27AM -0700, lslebodn wrote:

lslebodn commented on this pull request.

@@ -1104,7 +1104,6 @@ bool sss_krb5_realm_has_proxy(const char *realm)
 kerr = profile_get_values(profile, profile_path, &list);
 if (kerr == PROF_NO_RELATION || kerr == PROF_NO_SECTION) {
   kerr = 0;
 goto done;
hmm,
sh$ $git show cbf20090fa92cc9a6e31e3c903b21d020c519367
tree cbf20090fa92cc9a6e31e3c903b21d020c519367

Sorry, I meant this one:

commit f0815f5dff315576c8d1b6fedf00165a4161f8c0
Author: Jakub Hrozek <jhrozek@redhat.com>
Date:   Fri Sep 4 10:30:03 2015 +0200

    KRB5: Don't error out reading a minimal krb5.conf

    With some setups, krb5.conf can be really minimal. In those cases, we
    should ignore PROF_NO_RELATION and PROF_NO_SECTION and just return
    "false" as in "no proxy" without a loud debug message.

    Reviewed-by: Petr Cech <pcech@redhat.com>

diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index e29c753..2d2dfc4 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -1103,7 +1103,10 @@ bool sss_krb5_realm_has_proxy(const char *realm)
     profile_path[1] = realm;

     kerr = profile_get_values(profile, profile_path, &list);
-    if (kerr != 0) {
+    if (kerr == PROF_NO_RELATION || kerr == PROF_NO_SECTION) {
+        kerr = 0;
+        goto done;
+    } else if (kerr != 0) {
         DEBUG(SSSDBG_OP_FAILURE, "profile_get_values failed.\n");
         goto done;
     }

The commit message says that in that case we return false and even in retrospective it makes sense to me because the config doesn't have any related information to proxying.

celestian · 2016-11-08T13:19:49Z

UTIL: Removing of never read value

@@ -1104,7 +1104,6 @@ bool sss_krb5_realm_has_proxy(const char *realm)

     kerr = profile_get_values(profile, profile_path, &list);
     if (kerr == PROF_NO_RELATION || kerr == PROF_NO_SECTION) {
-        kerr = 0;
         goto done;

How @jhrozek said above false is right returning value. Proposed patch is about removing of kerr = 0 because it is not read anywhere.

lslebodn · 2016-11-08T13:38:15Z

On (08/11/16 05:26), celestian wrote:

celestian commented on this pull request.

@@ -269,6 +269,10 @@ static void rdp_message_send_and_reply_done(DBusPendingCall *pending,
sbus_req = talloc_get_type(ptr, struct sbus_request);
ret = rdp_process_pending_call(sbus_req, pending, &reply);
if (ret != EOK) {
   /\* Something bad happened. Just kill the request. */
   goto done;
}
if (reply == NULL) {
I don't insist on the patch RESPONDER: Adding of return value checking.

The patch is not absolutelly wrong.
But following check for NULL is redundant.

As I previously wrote, we should be consistent.
We shoudl either check ret != EOK after each invovation of
rdp_process_pending_call or we should check reply == NULL.

Ask @pbrezina why he wrote the code in such way.

LS

celestian · 2016-11-09T12:20:11Z

@pbrezina please, could you join discussion?

How we can see we call function rdp_process_pending_call() on two places. And we are not consistent on return value checking. Is there reason for this? Or should we use one form for it?

$ git grep -n -a5 'rdp_process_pending_call' -- '*.c'
src/responder/common/data_provider/rdp_message.c-81-    }
src/responder/common/data_provider/rdp_message.c-82-
src/responder/common/data_provider/rdp_message.c-83-    return ret;
src/responder/common/data_provider/rdp_message.c-84-}
src/responder/common/data_provider/rdp_message.c-85-
src/responder/common/data_provider/rdp_message.c:86:static errno_t rdp_process_pending_call(TALLOC_CTX *mem_ctx,
src/responder/common/data_provider/rdp_message.c-87-                                        DBusPendingCall *pending,
src/responder/common/data_provider/rdp_message.c-88-                                        DBusMessage **_reply)
src/responder/common/data_provider/rdp_message.c-89-{
src/responder/common/data_provider/rdp_message.c-90-    DBusMessage *reply;
src/responder/common/data_provider/rdp_message.c-91-    dbus_bool_t bret;
--
src/responder/common/data_provider/rdp_message.c-198-    errno_t ret;
src/responder/common/data_provider/rdp_message.c-199-
src/responder/common/data_provider/rdp_message.c-200-    req = talloc_get_type(ptr, struct tevent_req);
src/responder/common/data_provider/rdp_message.c-201-    state = tevent_req_data(req, struct rdp_message_state);
src/responder/common/data_provider/rdp_message.c-202-
src/responder/common/data_provider/rdp_message.c:203:    ret = rdp_process_pending_call(state, pending, &state->reply);
src/responder/common/data_provider/rdp_message.c-204-    if (ret != EOK) {
src/responder/common/data_provider/rdp_message.c-205-        tevent_req_error(req, ret);
src/responder/common/data_provider/rdp_message.c-206-        return;
src/responder/common/data_provider/rdp_message.c-207-    }
src/responder/common/data_provider/rdp_message.c-208-
--
src/responder/common/data_provider/rdp_message.c-266-    dbus_bool_t dbret;
src/responder/common/data_provider/rdp_message.c-267-    errno_t ret;
src/responder/common/data_provider/rdp_message.c-268-
src/responder/common/data_provider/rdp_message.c-269-    sbus_req = talloc_get_type(ptr, struct sbus_request);
src/responder/common/data_provider/rdp_message.c-270-
src/responder/common/data_provider/rdp_message.c:271:    ret = rdp_process_pending_call(sbus_req, pending, &reply);
src/responder/common/data_provider/rdp_message.c-272-    if (reply == NULL) {
src/responder/common/data_provider/rdp_message.c-273-        /* Something bad happened. Just kill the request. */
src/responder/common/data_provider/rdp_message.c-274-        ret = EIO;
src/responder/common/data_provider/rdp_message.c-275-        goto done;
src/responder/common/data_provider/rdp_message.c-276-    }

celestian · 2016-11-14T12:43:28Z

OK, I prefer checking of return value. So I pushed new version. I kept EIO error code for corrupted result.

lslebodn · 2016-11-14T13:25:34Z

On (14/11/16 04:43), celestian wrote:

OK, I prefer checking of return value. So I pushed new version. I kept EIO error code for corrupted result.

ACK

LS

lslebodn · 2016-11-14T13:32:05Z

master:

lslebodn reviewed Oct 25, 2016

View reviewed changes

Petr Čech added 2 commits November 14, 2016 13:35

RESPONDER: Adding of return value checking

714c0b0

UTIL: Removing of never read value

2728213

lslebodn added the Pushed label Nov 14, 2016

lslebodn closed this Nov 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing of nitpicks #65

Fixing of nitpicks #65

celestian commented Oct 25, 2016

lslebodn Oct 25, 2016

jhrozek Oct 25, 2016

lslebodn Oct 25, 2016

lslebodn Oct 25, 2016

celestian Oct 25, 2016

celestian Nov 8, 2016

jhrozek commented Oct 27, 2016

celestian commented Nov 8, 2016

lslebodn commented Nov 8, 2016

celestian commented Nov 9, 2016

celestian commented Nov 14, 2016

lslebodn commented Nov 14, 2016

lslebodn commented Nov 14, 2016

Fixing of nitpicks #65

Fixing of nitpicks #65

Conversation

celestian commented Oct 25, 2016

lslebodn Oct 25, 2016

Choose a reason for hiding this comment

jhrozek Oct 25, 2016

Choose a reason for hiding this comment

lslebodn Oct 25, 2016

Choose a reason for hiding this comment

lslebodn Oct 25, 2016

Choose a reason for hiding this comment

celestian Oct 25, 2016

Choose a reason for hiding this comment

celestian Nov 8, 2016

Choose a reason for hiding this comment

jhrozek commented Oct 27, 2016

celestian commented Nov 8, 2016

lslebodn commented Nov 8, 2016

celestian commented Nov 9, 2016

celestian commented Nov 14, 2016

lslebodn commented Nov 14, 2016

lslebodn commented Nov 14, 2016