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
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 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.
"""
See the full comment at https://github.com/SSSD/sssd/pull/65#issuecomment-256683069
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
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
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
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
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
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
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
URL: https://github.com/SSSD/sssd/pull/65 Title: #65: Fixing of nitpicks
Label: +Pushed
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
sssd-devel@lists.fedorahosted.org