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

Sumit Bose sbose at redhat.com
Fri Sep 18 10:07:38 UTC 2015


On Fri, Sep 18, 2015 at 11:43:26AM +0200, Michal Židek wrote:
> 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.

I agree, the delay might be caused by the environment (valgrind +
virtual machine). Maybe we can use RUNNING_ON_VALGRIND
(http://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.clientreq)
in the test and increase the timeout at runtime if it runs on valgrind?

bye,
Sumit

> 
> >
> >LS
> 


More information about the sssd-devel mailing list