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
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel