Re: [SSSD] [PATCH] PAM: Make p11_child timeout configurable
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. 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.
Michal