On Mon, Jul 06, 2015 at 01:35:32PM +0200, Jakub Hrozek wrote: > On Mon, Jul 06, 2015 at 11:52:45AM +0200, Jakub Hrozek wrote: > > On Fri, Jul 03, 2015 at 08:29:30PM +0200, Sumit Bose wrote: > > > On Fri, Jul 03, 2015 at 11:59:53AM +0200, Jakub Hrozek wrote: > > > > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: > > > > > Hi, > > > > > > > > > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701 > > > > > > > > > > The first patch just adds a common function instead of copying the > > > > > same > > > > > pattern again to the new test. > > > > > > > > > > The second adds a new request krb5_auth_queue_send() that wraps > > > > > krb5_auth_send() and also uses the Kerberos authentication queue. I > > > > > hope > > > > > the unit tests cover a lot of use-cases, if not, please suggest more! > > > > > > > > > > btw I was thinking that the chaining might not always be necessary if > > > > > the ccache is of type MEMORY and I hope that the serializaton wouldn't > > > > > be perceived as performance regression for users. Shall we say that > > > > > Pavel's cached auth patches are a more systematic solution that > > > > > doesn't > > > > > rely on properties of the ccache type in that case? > > > > > > > > I'm sorry, but CI fails on Debian because of wrong linking with > > > > libraries. I'm already testing a fix. Review of the rest is appreciated > > > > :-) > > > > > > ACK to the first patch. > > > > > > The second patch has a trailing whitespace > > > > > > > +} > > > > + > > > > +static void test_krb5_wait_mock(struct test_krb5_wait_queue *test_ctx, > > > ^ > > > > + const char *username, > > > > + time_t us_delay, > > > > + int ret, > > > > + int pam_status, > > > > + int dp_err) > > > > > > Otherwise the patch looks good and works as expected. But since you > > > mentioned > > > that it needs a respin for the Debian issue I would like to ask to add > > > changes similar to the following: > > > > > > diff --git a/src/providers/krb5/krb5_wait_queue.c > > > b/src/providers/krb5/krb5_wait_queue.c > > > index d0cc0c1..1262096 100644 > > > --- a/src/providers/krb5/krb5_wait_queue.c > > > +++ b/src/providers/krb5/krb5_wait_queue.c > > > @@ -271,17 +271,17 @@ struct tevent_req *krb5_auth_queue_send(TALLOC_CTX > > > *mem_ctx, > > > ret = add_to_wait_queue(be_ctx, req, pd, krb5_ctx); > > > if (ret == EOK) { > > > DEBUG(SSSDBG_TRACE_LIBS, > > > - "Request successfully added to wait queue " > > > - "of user [%s].\n", pd->user); > > > + "Request [%p] successfully added to wait queue " > > > + "of user [%s].\n", req, pd->user); > > > ret = EOK; > > > goto immediate; > > > } else if (ret == ENOENT) { > > > DEBUG(SSSDBG_TRACE_LIBS, "Wait queue of user [%s] is empty, " > > > - "running request immediately.\n", pd->user); > > > + "running request [%p] immediately.\n", pd->user, req); > > > } else { > > > DEBUG(SSSDBG_MINOR_FAILURE, > > > "Failed to add request to wait queue of user [%s], " > > > - "running request immediately.\n", pd->user); > > > + "running request [%p] immediately.\n", pd->user, req); > > > } > > > > > > subreq = krb5_auth_send(req, ev, be_ctx, pd, krb5_ctx); > > > @@ -322,7 +322,7 @@ static void krb5_auth_queue_done(struct tevent_req > > > *subreq) > > > return; > > > } > > > > > > - DEBUG(SSSDBG_TRACE_INTERNAL, "krb5_auth_queue done\n"); > > > + DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n", > > > req); > > > tevent_req_done(req); > > > } > > > > > > @@ -346,6 +346,7 @@ static void krb5_auth_queue_finish(struct tevent_req > > > *req, > > > if (ret != EOK) { > > > tevent_req_error(req, ret); > > > } else { > > > + DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n", > > > req); > > > tevent_req_done(req); > > > } > > > } > > > > > > > > > I was a bit irritated when I verified the by following the logs that every > > > entry to the wait queue was logged but only the exit of the immediate run > > > was > > > found in the logs. This is basically the last change. > > > > > > The other changes add the address of the request pointer to the log > > > message to make it easier to find matching enter and exit entries. I > > > hope this is in agreement with the recent discussion about improving log > > > messages. > > > > > > Finally the log levels are aligned. > > > > > > Feel free to drop any changes you don't like but I would appreciate it > > > if you can at least add a don message in krb5_auth_queue_finish(). > > > > > > bye, > > > Sumit > > > > Thank you for the review, I squashed in your changes in full. It's > > really important to keep DEBUG messages helpful. > > > > I couldn't see the whitespace issue in the patch, maybe I fixed it in > > parallel.
yes, it's gone now. > > > > Attached are new patches, CI is running so far, but I've done the same > > change as you showed me on IRC, so I'm confident it would help. > > CI: http://sssd-ci.duckdns.org/logs/job/18/59/summary.html CI passes for me as well http://sssd-ci.duckdns.org/logs/job/18/60/summary.html. ACK. bye, Sumit > _______________________________________________ > sssd-devel mailing list > [email protected] > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
