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(a)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(a)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.
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?
LS