[SSSD] [PATCH] PAM: Make p11_child timeout configurable

Michal Židek mzidek at redhat.com
Fri Sep 18 09:43:26 UTC 2015


On 09/18/2015 11:27 AM, Lukas Slebodnik wrote:
> On (18/09/15 11:18), Michal Židek wrote:
>> On 09/18/2015 10:19 AM, Lukas Slebodnik wrote:
>>> On (16/09/15 13:41), Michal Židek wrote:
>>>> On 09/14/2015 05:49 PM, Jakub Hrozek wrote:
>>>>> On Mon, Sep 07, 2015 at 03:38:32PM +0200, Michal Židek wrote:
>>>>>> Hi,
>>>>>>
>>>>>> patch for ticket https://fedorahosted.org/sssd/ticket/2773
>>>>>> is attached.
>>>>>>
>>>>>> Michal
>>>>>
>>>>>>  From 96215f618f61b8b2b303f0398a41af94292ccf57 Mon Sep 17 00:00:00 2001
>>>>>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
>>>>>> Date: Mon, 7 Sep 2015 15:19:53 +0200
>>>>>> Subject: [PATCH] PAM: Make p11_child timeout configurable
>>>>>>
>>>>>> Ticket:
>>>>>> https://fedorahosted.org/sssd/ticket/2773
>>>>>> ---
>>>>>>   Makefile.am                          |  2 +-
>>>>>>   src/confdb/confdb.h                  |  1 +
>>>>>>   src/config/SSSDConfig/__init__.py.in |  1 +
>>>>>>   src/man/sssd.conf.5.xml              | 12 ++++++++++++
>>>>>>   src/responder/pam/pamsrv_cmd.c       | 35 ++++++++++++++++++++++++++++++-----
>>>>>>   5 files changed, 45 insertions(+), 6 deletions(-)
>>>>>
>>>>> Please also add the new parameter to src/config/etc/sssd.api.conf.
>>>>>
>>>>> One style-question inline (but really, more of a question..)
>>>>>
>>>>>>   static errno_t
>>>>>> @@ -1126,8 +1125,21 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
>>>>>>       }
>>>>>>
>>>>>>       if (may_do_cert_auth(pctx, pd)) {
>>>>>> +        int p11_child_timeout;
>>>>>
>>>>> I wonder if the block starts getting so big that it makes sense to add
>>>>> variables to its scope..isn't it better to just split the block into tis
>>>>> own function?
>>>>>
>>>>> Especially if the block is unit-tested?
>>>>
>>>> Yes, the block grew a little too much with the added
>>>> option. See the new version.
>>>>
>>>> Michal
>>>
>>> >From d094c0f9814b23a8d669940e4c6a7cd8c7077164 Mon Sep 17 00:00:00 2001
>>>> From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek at redhat.com>
>>>> Date: Mon, 7 Sep 2015 15:19:53 +0200
>>>> Subject: [PATCH 1/2] PAM: Make p11_child timeout configurable
>>>>
>>>> Ticket:
>>>> https://fedorahosted.org/sssd/ticket/2773
>>>> ---
>>>> Makefile.am                          |  2 +-
>>>> src/confdb/confdb.h                  |  1 +
>>>> src/config/SSSDConfig/__init__.py.in |  1 +
>>>> src/config/etc/sssd.api.conf         |  1 +
>>>> src/man/sssd.conf.5.xml              | 12 +++++++
>>>> src/responder/pam/pamsrv_cmd.c       | 64 +++++++++++++++++++++++-------------
>>>> 6 files changed, 57 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/Makefile.am b/Makefile.am
>>>> index dc0670a..1298522 100644
>>>> --- a/Makefile.am
>>>> +++ b/Makefile.am
>>>> @@ -1893,7 +1893,7 @@ pam_srv_tests_SOURCES = \
>>>> pam_srv_tests_CFLAGS = \
>>>>      -U SSSD_LIBEXEC_PATH -DSSSD_LIBEXEC_PATH=\"$(abs_builddir)\" \
>>>>      $(AM_CFLAGS) \
>>>> -    -DSSS_P11_CHILD_TIMEOUT=30 \
>>>> +    -DSSS_P11_CHILD_TIMEOUT_DEFAULT=30 \
>>> Do we need this compile time option if it is possible to
>>> change it at runtime? We should be able to use the option p11_child_timeout
>>> in test as well and not just in sssd.conf
>>>
>>> LS
>>
>> It is not necessary.
> My intention was to simplify build system part.
>
>> OTOH I do not think it is bad way
>> to do it. This will force the higher default for all
>> future tests that use p11 child globally without the
>> need to modify each test. I would prefer to keep
>> it this way, but I can change it if you insist.
>>
> This can be achieved also in cleaner way
> which will be visible directly in test.
> We can use setup/teardown functions for configuring
> p11_child timeout to 30 seconds.

Ok. Will do.

>
> BTW setting SSS_P11_CHILD_TIMEOUT to 30 seconds was
> just a temporary solution for a broken test.
> It would be good to identify why 5 seconds was not enough.
> There might be either bug in code or in test and increasing
> the timeout just hid the problem.
>
> Do we need a ticket for this?

I am not sure. We can ask Sumit (CCing), but IIRC the test
was only failing when running under Valgrind. I would investigate
only if we find out it is slow also outside Valgrind, not
sooner.

>
> LS



More information about the sssd-devel mailing list