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. > > 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 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
