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.