[sssd PR#65][opened] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Author: celestian
 Title: #65: Fixing of nitpicks
Action: opened

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

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/65/head:pr65
git checkout pr65

[sssd PR#65][comment] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Title: #65: Fixing of nitpicks

jhrozek commented:
"""
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 
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

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.

"""

See the full comment at https://github.com/SSSD/sssd/pull/65#issuecomment-256683069

[sssd PR#65][comment] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Title: #65: Fixing of nitpicks

celestian commented:
"""
```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.
"""

See the full comment at https://github.com/SSSD/sssd/pull/65#issuecomment-259134075

[sssd PR#65][comment] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Title: #65: Fixing of nitpicks

lslebodn commented:
"""
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

"""

See the full comment at https://github.com/SSSD/sssd/pull/65#issuecomment-259138069

[sssd PR#65][comment] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Title: #65: Fixing of nitpicks

celestian commented:
"""
@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-    }
```
"""

See the full comment at https://github.com/SSSD/sssd/pull/65#issuecomment-259402267

[sssd PR#65][synchronized] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Author: celestian
 Title: #65: Fixing of nitpicks
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/65/head:pr65
git checkout pr65

[sssd PR#65][comment] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Title: #65: Fixing of nitpicks

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

See the full comment at https://github.com/SSSD/sssd/pull/65#issuecomment-260325605

[sssd PR#65][comment] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Title: #65: Fixing of nitpicks

lslebodn commented:
"""
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

"""

See the full comment at https://github.com/SSSD/sssd/pull/65#issuecomment-260333555

[sssd PR#65][comment] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Title: #65: Fixing of nitpicks

lslebodn commented:
"""
master:
* e16b3174e465deceaca9bfef29d05dfe5537cf9c
* 8c256b6708e52b20eb7d294cdd8b19c8615859d2

"""

See the full comment at https://github.com/SSSD/sssd/pull/65#issuecomment-260334926

[sssd PR#65][+Pushed] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Title: #65: Fixing of nitpicks

Label: +Pushed

[sssd PR#65][closed] Fixing of nitpicks

[fidencio]

URL: https://github.com/SSSD/sssd/pull/65
Author: celestian
 Title: #65: Fixing of nitpicks
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/65/head:pr65
git checkout pr65