On 09/07/2015 03:38 PM, Michal Židek wrote:
Hi,
patch for ticket https://fedorahosted.org/sssd/ticket/2773 is attached.
Michal
CI passed: http://sssd-ci.duckdns.org/logs/job/25/49/summary.html
Michal
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@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?
ret = confdb_get_int(pctx->rctx->cdb, CONFDB_PAM_CONF_ENTRY,CONFDB_PAM_P11_CHILD_TIMEOUT,SSS_P11_CHILD_TIMEOUT_DEFAULT,&p11_child_timeout);if (ret != EOK) {DEBUG(SSSDBG_CRIT_FAILURE,"Failed to read p11_child_timeout from confdb: [%d]: %s\n",ret, sss_strerror(ret));goto done;}req = pam_check_cert_send(cctx, cctx->ev, pctx->p11_child_debug_fd,
pctx->nss_db, SSS_P11_CHILD_TIMEOUT, pd);
pctx->nss_db, p11_child_timeout, pd); if (req == NULL) { DEBUG(SSSDBG_OP_FAILURE, "pam_check_cert_send failed.\n"); ret = ENOMEM;
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@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
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@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@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
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@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@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
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@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@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
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@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@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
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@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@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.clientr...) in the test and increase the timeout at runtime if it runs on valgrind?
bye, Sumit
LS
On 09/18/2015 12:07 PM, Sumit Bose wrote:
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@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@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.clientr...) in the test and increase the timeout at runtime if it runs on valgrind?
I am not sure about RUNNING_ON_VALGRIND. We got used to the fact that things are slow in the CI (sometimes very slow) so we may hit the timeout in CI even without Valgrind. Valgrind just makes it more likely. We currently do not have infrastructure to test performance so I would be less strict in the tests.
bye, Sumit
LS
New patch set is attached. The first patch may fail in the CI due to the timeout. The second one fixes it.
Michal
Please see nitpicks inline.
On 09/18/2015 05:07 PM, Michal Židek wrote:
0001-PAM-Make-p11_child-timeout-configurable.patch
From 1b03ae46d3ae5812c0e414d3f99abadfdabf672e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?=mzidek@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 | 1 - 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 | 65 ++++++++++++++++++++++-------------- 6 files changed, 55 insertions(+), 26 deletions(-)
diff --git a/Makefile.am b/Makefile.am index 3eaf578..777a91c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1895,7 +1895,6 @@ pam_srv_tests_SOURCES = \ pam_srv_tests_CFLAGS = \ -U SSSD_LIBEXEC_PATH -DSSSD_LIBEXEC_PATH="$(abs_builddir)" \ $(AM_CFLAGS) \
- -DSSS_P11_CHILD_TIMEOUT=30 \ $(NULL) pam_srv_tests_LDFLAGS = \ -Wl,-wrap,sss_packet_get_body \
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index 427c309..96b2444 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -118,6 +118,7 @@ #define CONFDB_PAM_ACCOUNT_EXPIRED_MESSAGE "pam_account_expired_message" #define CONFDB_PAM_CERT_AUTH "pam_cert_auth" #define CONFDB_PAM_CERT_DB_PATH "pam_cert_db_path" +#define CONFDB_PAM_P11_CHILD_TIMEOUT "p11_child_timeout"
/* SUDO */ #define CONFDB_SUDO_CONF_ENTRY "config/sudo" diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in index cb9c368..038de16 100644 --- a/src/config/SSSDConfig/__init__.py.in +++ b/src/config/SSSDConfig/__init__.py.in @@ -89,6 +89,7 @@ option_strings = { 'pam_trusted_users' : _('List of trusted uids or user's name'), 'pam_public_domains' : _('List of domains accessible even for untrusted users.'), 'pam_account_expired_message' : _('Message printed when user account is expired.'),
- 'p11_child_timeout' : _('How many seconds will pam_sss wait for p11_child to finish'),
Could you please add '.' at the end of the sentence?
+static errno_t check_cert(struct cli_ctx *cctx,
struct pam_ctx *pctx,struct tevent_req *req,struct pam_auth_req *preq,struct pam_data *pd)+{
- int p11_child_timeout;
- const int P11_CHILD_TIMEOUT_DEFAULT = 10;
- errno_t ret;
- ret = confdb_get_int(pctx->rctx->cdb, CONFDB_PAM_CONF_ENTRY,
CONFDB_PAM_P11_CHILD_TIMEOUT,P11_CHILD_TIMEOUT_DEFAULT,&p11_child_timeout);- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,"Failed to read p11_child_timeout from confdb: [%d]: %s\n",ret, sss_strerror(ret));return ret;- }
- req = pam_check_cert_send(cctx, cctx->ev, pctx->p11_child_debug_fd,
pctx->nss_db, p11_child_timeout, pd);- if (req == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "pam_check_cert_send failed.\n");return ENOMEM;- } else {
tevent_req_set_callback(req, pam_forwarder_cert_cb, preq);return EAGAIN;
Could you move the code outside of the else branch?
- }
- return EOK;
Please remove this unreachable code.
On 09/18/2015 05:07 PM, Michal Židek wrote:
+static errno_t check_cert(struct cli_ctx *cctx,
struct pam_ctx *pctx,struct tevent_req *req,
What's the point of parameter *req? You are not reading it and if you want to use it as output parameter you should use **_req, right?
struct pam_auth_req *preq,struct pam_data *pd)+{
- int p11_child_timeout;
- const int P11_CHILD_TIMEOUT_DEFAULT = 10;
- errno_t ret;
- ret = confdb_get_int(pctx->rctx->cdb, CONFDB_PAM_CONF_ENTRY,
CONFDB_PAM_P11_CHILD_TIMEOUT,P11_CHILD_TIMEOUT_DEFAULT,&p11_child_timeout);- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,"Failed to read p11_child_timeout from confdb: [%d]: %s\n",ret, sss_strerror(ret));return ret;- }
- req = pam_check_cert_send(cctx, cctx->ev, pctx->p11_child_debug_fd,
pctx->nss_db, p11_child_timeout, pd);- if (req == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "pam_check_cert_send failed.\n");return ENOMEM;- } else {
tevent_req_set_callback(req, pam_forwarder_cert_cb, preq);return EAGAIN;- }
- return EOK;
+}
On 09/20/2015 05:07 PM, Pavel Reichl wrote:
On 09/18/2015 05:07 PM, Michal Židek wrote:
+static errno_t check_cert(struct cli_ctx *cctx,
struct pam_ctx *pctx,struct tevent_req *req,What's the point of parameter *req? You are not reading it and if you want to use it as output parameter you should use **_req, right?
struct pam_auth_req *preq,struct pam_data *pd)+{
- int p11_child_timeout;
- const int P11_CHILD_TIMEOUT_DEFAULT = 10;
- errno_t ret;
- ret = confdb_get_int(pctx->rctx->cdb, CONFDB_PAM_CONF_ENTRY,
CONFDB_PAM_P11_CHILD_TIMEOUT,P11_CHILD_TIMEOUT_DEFAULT,&p11_child_timeout);- if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE,"Failed to read p11_child_timeout from confdb: [%d]:%s\n",
ret, sss_strerror(ret));return ret;- }
- req = pam_check_cert_send(cctx, cctx->ev, pctx->p11_child_debug_fd,
pctx->nss_db, p11_child_timeout, pd);- if (req == NULL) {
DEBUG(SSSDBG_OP_FAILURE, "pam_check_cert_send failed.\n");return ENOMEM;- } else {
tevent_req_set_callback(req, pam_forwarder_cert_cb, preq);return EAGAIN;- }
- return EOK;
+}
Hi,
new patches attached.
Michal
Unless other reviewers oppose I think we can ACK the patch. Code LGTM. CI passed: http://sssd-ci.duckdns.org/logs/job/26/94/summary.html
On Mon, Sep 21, 2015 at 02:41:12PM +0200, Pavel Reichl wrote:
Unless other reviewers oppose I think we can ACK the patch. Code LGTM. CI passed: http://sssd-ci.duckdns.org/logs/job/26/94/summary.html
Pushed to master: * d85be8ad409c9efa9cf9e9ab6f9c2d911b01e5c1 * ab3c0e05d18616295afbd46acad1ca243b33861c
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Michal Židek -
Pavel Reichl -
Sumit Bose