[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Simo Sorce 
On Wed, 2016-04-20 at 19:58 +0200, Lukas Slebodnik wrote:
> On (20/04/16 17:21), Jakub Hrozek wrote:
> >On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
> >> On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
> >> > On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> >> > > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> >> > > > Thanks, IIRC the int-instead of enum use is intentional, I will look
> >> > > > at the others.
> >> > > 
> >> > > The last coverity/clang thing is a false positive, but I initialized
> >> > > reply to NULL anyway, I expect now it will start complaining of 
> >> > > possible
> >> > > NULL dereference :-)
> >> > > 
> >> > > Attached find patches that fixes all other issues (hopefully), one of
> >> > > them simply dropped an entire function as it turned out I wasn't using
> >> > > it.
> >> > > 
> >> > > Simo.
> >> > > 
> >> > > -- 
> >> > > Simo Sorce * Red Hat, Inc * New York
> >> > 
> >> > > From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001
> >> > > From: Simo Sorce 
> >> > > Date: Thu, 7 Jan 2016 10:17:38 -0500
> >> > > Subject: [PATCH 05/15] Responders: Add support for socket activation
> >> > 
> >> > ACK (visual at this point) with a question - do we want to check
> >> > that the fd we received is a UNIX socket using sd_is_socket_unix()?
> >> > 
> >> > The sd_listen_fds() manpage recommends that.
> >> 
> >> If they recommend it we should, yes.
> >
> >OK, same as with the responder issue, I will prepare a fixup patch and
> >ask you to check it before squashing into your patches..
> >
> >> 
> >> > > From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001
> >> > > From: Simo Sorce 
> >> > > Date: Wed, 20 Jan 2016 10:33:39 -0500
> >> > > Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
> >> > > 
> >> > > The secrets database will have "subsections", ie sections that are in 
> >> > > the
> >> > > "secrets" namespace and look like this: [secrets/]
> >> > > 
> >> > > This function allows to source any section under secrets/ or under any
> >> > > arbitrary sub-path.
> >> > > 
> >> > > Related:
> >> > > https://fedorahosted.org/sssd/ticket/2913
> >
> >[...]
> >
> >> > > +int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
> >> > > +struct confdb_ctx *cdb,
> >> > > +const char *section,
> >> > > +char ***sections,
> >> > > +int *num_sections)
> >> > > +{
> >> > > +TALLOC_CTX *tmp_ctx = NULL;
> >> > > +char *secdn;
> >> > > +struct ldb_dn *base = NULL;
> >> > > +struct ldb_result *res = NULL;
> >> > > +static const char *attrs[] = {"cn", NULL};
> >> > > +char **names;
> >> > > +int base_comp_num;
> >> > > +int num;
> >> > > +int i;
> >> > 
> >> > Can you use size_t here so that clang doesn't complain about "comparison
> >> > of integers of different signs: 'int' and 'unsigned int'" in the for
> >> > loop below?
> >> 
> >> meh, ok :-)
> >
> >Trivial, I can also fix this locally and ask you if it's OK to squash.
> >
> >> 
> >> > > +int ret;
> >> > > +
> >> > > +tmp_ctx = talloc_new(mem_ctx);
> >> > > +if (tmp_ctx == NULL) {
> >> > > +return ENOMEM;
> >> > > +}
> >> > > +
> >> > > +ret = parse_section(tmp_ctx, section, , NULL);
> >> > > +if (ret != EOK) {
> >> > > +goto done;
> >> > > +}
> >> > > +
> >> > > +base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
> >> > > +if (base == NULL) {
> >> > > +ret = ENOMEM;
> >> > > +goto done;
> >> > > +}
> >> > > +
> >> > > +base_comp_num = ldb_dn_get_comp_num(base);
> >> > > +
> >> > > +ret = ldb_search(cdb->ldb, tmp_ctx, , base, LDB_SCOPE_SUBTREE,
> >> > > + attrs, NULL);
> >> > > +if (ret != LDB_SUCCESS) {
> >> > > +ret = EIO;
> >> > > +goto done;
> >> > > +}
> >> > > +
> >> > > +names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
> >> > > +if (names == NULL) {
> >> > > +ret = ENOMEM;
> >> > > +goto done;
> >> > > +}
> >> > > +
> >> > > +for (num = 0, i = 0; i < res->count; i++) {
> >> > > +const struct ldb_val *val;
> >> > > +char *name;
> >> > > +int n;
> >> > > +int j;
> >> > 
> >> > Every time I see variables declared in a scope in C except loop control
> >> > variables I think "This should be a static function of its own" :-)
> >> 
> >> Should it be in this case ? 
> >
> >Not sure, I'll make up my mind when I fix the other trivial issues.
> >
> >
> >[...]
> >
> >> > > From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001
> >> > > From: Christian Heimes 
> >> > > Date: Fri, 8 Jan 2016 13:26:22 +0100
> >> > > Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
> >> > > 
> >> > > Prepares autoconf for the new Secrets Provider dependencies
>

[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache

2016-04-20 Thread Lukas Slebodnik 
On (20/04/16 10:18), Pavel Březina wrote:
>On 04/19/2016 08:18 AM, Petr Cech wrote:
>> On 04/18/2016 04:04 PM, Petr Cech wrote:
>> > On 04/15/2016 09:54 AM, Pavel Březina wrote:
>> > > Hi, CI fails on debian:
>> > > http://sssd-ci.duckdns.org/logs/job/41/53/debian_testing/ci.html
>> > 
>> > Hi,
>> > 
>> > CI tests passed locally.
>> > 
>> > Moderate CI tests passed too:
>> > http://sssd-ci.duckdns.org/logs/job/41/87/summary.html
>> > 
>> > And I have started rigorous CI tests (8e62b7b hash), so we will see.
>> 
>> CI tests on debian passed:
>> http://sssd-ci.duckdns.org/logs/job/41/94/summary.html
>> 
>> > Regads
>> 
>> Whole patch set is attached for clarity.
>> 
>> Regards
>
>I ran CI twice and the debian error is indeed gone. It failed both times
>though for reason that is unrelated to you patches, probably
>timeout-related...
>
>http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4242/
>http://sssd-ci.idm.lab.eng.brq.redhat.com:8080/job/ci/4244/
>
>Ack.

master:
* fd3cbf6bfe86a245d7e90d2a355794eb9c70d525
* be6d25ea38ddda232175aab5e297d8c6cb223551
* 27a7dedb0ee4d4b51ca4c196aa894ad30cb3e821
* e2d26e97d62f06f65e8228b28746471cc5f73fe5

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Lukas Slebodnik 
On (20/04/16 17:21), Jakub Hrozek wrote:
>On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
>> On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
>> > On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
>> > > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
>> > > > Thanks, IIRC the int-instead of enum use is intentional, I will look
>> > > > at the others.
>> > > 
>> > > The last coverity/clang thing is a false positive, but I initialized
>> > > reply to NULL anyway, I expect now it will start complaining of possible
>> > > NULL dereference :-)
>> > > 
>> > > Attached find patches that fixes all other issues (hopefully), one of
>> > > them simply dropped an entire function as it turned out I wasn't using
>> > > it.
>> > > 
>> > > Simo.
>> > > 
>> > > -- 
>> > > Simo Sorce * Red Hat, Inc * New York
>> > 
>> > > From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001
>> > > From: Simo Sorce 
>> > > Date: Thu, 7 Jan 2016 10:17:38 -0500
>> > > Subject: [PATCH 05/15] Responders: Add support for socket activation
>> > 
>> > ACK (visual at this point) with a question - do we want to check
>> > that the fd we received is a UNIX socket using sd_is_socket_unix()?
>> > 
>> > The sd_listen_fds() manpage recommends that.
>> 
>> If they recommend it we should, yes.
>
>OK, same as with the responder issue, I will prepare a fixup patch and
>ask you to check it before squashing into your patches..
>
>> 
>> > > From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001
>> > > From: Simo Sorce 
>> > > Date: Wed, 20 Jan 2016 10:33:39 -0500
>> > > Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
>> > > 
>> > > The secrets database will have "subsections", ie sections that are in the
>> > > "secrets" namespace and look like this: [secrets/]
>> > > 
>> > > This function allows to source any section under secrets/ or under any
>> > > arbitrary sub-path.
>> > > 
>> > > Related:
>> > > https://fedorahosted.org/sssd/ticket/2913
>
>[...]
>
>> > > +int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
>> > > +struct confdb_ctx *cdb,
>> > > +const char *section,
>> > > +char ***sections,
>> > > +int *num_sections)
>> > > +{
>> > > +TALLOC_CTX *tmp_ctx = NULL;
>> > > +char *secdn;
>> > > +struct ldb_dn *base = NULL;
>> > > +struct ldb_result *res = NULL;
>> > > +static const char *attrs[] = {"cn", NULL};
>> > > +char **names;
>> > > +int base_comp_num;
>> > > +int num;
>> > > +int i;
>> > 
>> > Can you use size_t here so that clang doesn't complain about "comparison
>> > of integers of different signs: 'int' and 'unsigned int'" in the for
>> > loop below?
>> 
>> meh, ok :-)
>
>Trivial, I can also fix this locally and ask you if it's OK to squash.
>
>> 
>> > > +int ret;
>> > > +
>> > > +tmp_ctx = talloc_new(mem_ctx);
>> > > +if (tmp_ctx == NULL) {
>> > > +return ENOMEM;
>> > > +}
>> > > +
>> > > +ret = parse_section(tmp_ctx, section, , NULL);
>> > > +if (ret != EOK) {
>> > > +goto done;
>> > > +}
>> > > +
>> > > +base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
>> > > +if (base == NULL) {
>> > > +ret = ENOMEM;
>> > > +goto done;
>> > > +}
>> > > +
>> > > +base_comp_num = ldb_dn_get_comp_num(base);
>> > > +
>> > > +ret = ldb_search(cdb->ldb, tmp_ctx, , base, LDB_SCOPE_SUBTREE,
>> > > + attrs, NULL);
>> > > +if (ret != LDB_SUCCESS) {
>> > > +ret = EIO;
>> > > +goto done;
>> > > +}
>> > > +
>> > > +names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
>> > > +if (names == NULL) {
>> > > +ret = ENOMEM;
>> > > +goto done;
>> > > +}
>> > > +
>> > > +for (num = 0, i = 0; i < res->count; i++) {
>> > > +const struct ldb_val *val;
>> > > +char *name;
>> > > +int n;
>> > > +int j;
>> > 
>> > Every time I see variables declared in a scope in C except loop control
>> > variables I think "This should be a static function of its own" :-)
>> 
>> Should it be in this case ? 
>
>Not sure, I'll make up my mind when I fix the other trivial issues.
>
>
>[...]
>
>> > > From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001
>> > > From: Christian Heimes 
>> > > Date: Fri, 8 Jan 2016 13:26:22 +0100
>> > > Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
>> > > 
>> > > Prepares autoconf for the new Secrets Provider dependencies
>> > > 
>> > > Related:
>> > > https://fedorahosted.org/sssd/ticket/2913
>> > > 
>> > 
>> > [...]
>> > 
>> > > +PKG_CHECK_MODULES([HTTP_PARSER], [http_parser], 
>> > > [found_http_parser=yes], [found_http_parser=no])
>> > 
>> > There is no pkgconfig for http-parser-devel, so it seems to be this line
>> > is redundant.
>> > 
>> > Otherwise ACK.

[SSSD] Re: [PATCH] NEGCACHE: Removing of condition for ttl = -1

2016-04-20 Thread Lukas Slebodnik 
On (20/04/16 08:41), Petr Cech wrote:
>On 04/19/2016 01:55 PM, Petr Cech wrote:
>> Hi,
>> 
>> I found a strange condition in the function sss_ncache_check_str().
>> 
>> This condition causes the cache is NOT checked and the result of
>> checking is automatically EEXIST.
>> 
>> I dind't find call of sss_ncache_check_str() with ttl = -1, except in
>> tests.
>>
It looks like I didn't remember it correctly an you were right.
and it does not make a sense to have two values (0,-1) for special purpose.

>> Note: We use value 0 for permanent cache, no -1.
>> 
>
>Hello,
>
>I added one another little patch to this, so there is patch set.
>
>Regards
>
>-- 
>Petr^4 Čech

>From 9b8a7a4ca917134db82fb0f8992ab933a72ebd1e Mon Sep 17 00:00:00 2001
>From: Petr Cech 
>Date: Tue, 19 Apr 2016 13:20:25 -0400
>Subject: [PATCH 1/2] NEGCACHE: Fixing typo in test_sss_ncache_gid()
>
>There were sss_ncache_*_uid() functions instead of
>sss_ncache_*_gid() functions.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2928
>---
> src/tests/cmocka/test_negcache.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/src/tests/cmocka/test_negcache.c 
>b/src/tests/cmocka/test_negcache.c
>index 
>274e1543cc842bbb6b125423a76cf665354cc059..b3118abdc76c38539d725e7924b3905638438005
> 100644
>--- a/src/tests/cmocka/test_negcache.c
>+++ b/src/tests/cmocka/test_negcache.c
>@@ -265,10 +265,10 @@ static void test_sss_ncache_gid(void **state)
> assert_int_equal(ret, EEXIST);
> 
> permanent = false;
>-ret = sss_ncache_set_uid(ts->ctx, permanent, NULL, gid);
>+ret = sss_ncache_set_gid(ts->ctx, permanent, NULL, gid);
> assert_int_equal(ret, EOK);
> 
>-ret = sss_ncache_check_uid(ts->ctx, ttl, NULL, gid);
>+ret = sss_ncache_check_gid(ts->ctx, ttl, NULL, gid);
> assert_int_equal(ret, EEXIST);

ACK

>From cd5b2217d22f1da7d64cf81e67ac31f823d40e06 Mon Sep 17 00:00:00 2001
>From: Petr Cech 
>Date: Tue, 19 Apr 2016 07:35:26 -0400
>Subject: [PATCH 2/2] NEGCACHE: Removing of condition for ttl = -1
>
>If ttl = -1 then function sss_ncache_check_str() returns EEXIST without
>checking negcache. This behaviour is out of logic.
>
>We use ttl = 0 for permanent caching.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2928
>---
> src/responder/common/negcache.c  |  6 --
> src/tests/cmocka/test_negcache.c | 46 
> 2 files changed, 52 deletions(-)
>
>diff --git a/src/responder/common/negcache.c b/src/responder/common/negcache.c
>index 
>5b0517ceba85d6e35515a935423412314c218143..1617bf8c5cf7d36e7091a000f6473d1bcfe44f3f
> 100644
>--- a/src/responder/common/negcache.c
>+++ b/src/responder/common/negcache.c
>@@ -97,12 +97,6 @@ static int sss_ncache_check_str(struct sss_nc_ctx *ctx, 
>char *str, int ttl)
> goto done;
> }
> 
>-if (ttl == -1) {
>-/* a negative ttl means: never expires */
>-ret = EEXIST;
>-goto done;
>-}
>-

ACK

http://sssd-ci.duckdns.org/logs/job/42/50/summary.html

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] NEGCACHE: Test fix

2016-04-20 Thread Lukas Slebodnik 
On (20/04/16 08:39), Petr Cech wrote:
>On 04/19/2016 07:24 PM, Petr Cech wrote:
>> Hello,
>> 
>> there is little simple patch which fix the typo.
>> 
>
>Hello,
>
>I wrote better commit message and I added to patch set with
>
OK, I marked this thread as supperseeded in patchwork

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Simo Sorce 
On Wed, 2016-04-20 at 17:18 +0200, Jakub Hrozek wrote:
> On Wed, Apr 20, 2016 at 09:43:05AM -0400, Simo Sorce wrote:
> > On Wed, 2016-04-20 at 11:12 +0200, Jakub Hrozek wrote:
> > > On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote:
> > > > > > From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 
> > > > > > 2001
> > > > > > From: Simo Sorce 
> > > > > > Date: Tue, 12 Jan 2016 20:13:28 -0500
> > > > > > Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
> > > > > > 
> > > > > > This allows the services to self monitor.
> > > > > > 
> > > > > > Related:
> > > > > > https://fedorahosted.org/sssd/ticket/2921
> > > > > 
> > > > > Is it intentional that we also enable the watchdog in monitor? I 
> > > > > haven't
> > > > > seen the sssd process being stuck and if it does, we probably have
> > > > > bigger issues, so it's probably fine, I just need to remember to not
> > > > > SIGSTOP sssd when testing anymore :)
> > > > > 
> > > > > Otherwise ack.
> > > > 
> > > > Actually, more questions...
> > > > 
> > > > Can you help me test this patch? I tried to inject sleep() into sssd_be
> > > > code and the sleep was just interrupted by the SIGRT delivery. With 
> > > > SSSD,
> > > > most of the time the process was stuck was because it was writing a lot 
> > > > of
> > > > data with fsync()/fdatasync(). I can't find any information in the Linux
> > > > fsync manpage on how fsync behaves wrt signals. openpub manpages 
> > > > indicate
> > > > that fsync would return EINTR, which worries me a bit..
> > > 
> > > Hmm, sorry, I was not being careful enough. man 7 signal also says:
> > > """
> > > The sleep(3) function is also never restarted if interrupted by a
> > > handler, but gives a success return: the number of seconds remaining to
> > > sleep.
> > > """
> > > 
> > > so the sleep testcase was wrong even though CatchSignal uses SA_RESTART.
> > > But do you know how would write() or fsync() behave here? The signal
> > > manpage is a bit unclar to me as it talks about "slow" devices..
> > > 
> > > Or can you think of some easy way to test this?
> > 
> > The fsync manpage here says:
> > "The call blocks until the device reports that the transfer has
> > completed."
> > 
> > And does not report EINTR as a possible error.
> > 
> > That said I am a bit unclear what you want to test actually ?
> 
> I want to actually test that the service can be restarted if stuck and
> reconnects fine. So far I haven't been lucky - SIGSTOP-ing the service
> stopped delivery of the signals, so did attaching gdb and waiting.

gdb it, do *not* cont, but tell gdb to not block signals, iirc you do
that with: handle SIGRTMIN nostop noprint pass

if you then wait in gdb the timers will fire and eventually the process
will be killed because you are blocking the main loop and not resetting
the watchdog.

> But most importantly I want to make sure that if tdb is writing a
> transaction and the signal is delivered, then we don't fsync() in tdb
> doesn't get interrupted and doesn't corrupt the database. From all the
> cases where users complained about a service being restarted, it was
> always about tdb writing stuff to disk...

I'm inquiring about this.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Jakub Hrozek 
On Wed, Apr 20, 2016 at 09:59:19AM -0400, Simo Sorce wrote:
> On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
> > On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> > > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > > > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > > > at the others.
> > > 
> > > The last coverity/clang thing is a false positive, but I initialized
> > > reply to NULL anyway, I expect now it will start complaining of possible
> > > NULL dereference :-)
> > > 
> > > Attached find patches that fixes all other issues (hopefully), one of
> > > them simply dropped an entire function as it turned out I wasn't using
> > > it.
> > > 
> > > Simo.
> > > 
> > > -- 
> > > Simo Sorce * Red Hat, Inc * New York
> > 
> > > From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001
> > > From: Simo Sorce 
> > > Date: Thu, 7 Jan 2016 10:17:38 -0500
> > > Subject: [PATCH 05/15] Responders: Add support for socket activation
> > 
> > ACK (visual at this point) with a question - do we want to check
> > that the fd we received is a UNIX socket using sd_is_socket_unix()?
> > 
> > The sd_listen_fds() manpage recommends that.
> 
> If they recommend it we should, yes.

OK, same as with the responder issue, I will prepare a fixup patch and
ask you to check it before squashing into your patches..

> 
> > > From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001
> > > From: Simo Sorce 
> > > Date: Wed, 20 Jan 2016 10:33:39 -0500
> > > Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
> > > 
> > > The secrets database will have "subsections", ie sections that are in the
> > > "secrets" namespace and look like this: [secrets/]
> > > 
> > > This function allows to source any section under secrets/ or under any
> > > arbitrary sub-path.
> > > 
> > > Related:
> > > https://fedorahosted.org/sssd/ticket/2913

[...]

> > > +int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
> > > +struct confdb_ctx *cdb,
> > > +const char *section,
> > > +char ***sections,
> > > +int *num_sections)
> > > +{
> > > +TALLOC_CTX *tmp_ctx = NULL;
> > > +char *secdn;
> > > +struct ldb_dn *base = NULL;
> > > +struct ldb_result *res = NULL;
> > > +static const char *attrs[] = {"cn", NULL};
> > > +char **names;
> > > +int base_comp_num;
> > > +int num;
> > > +int i;
> > 
> > Can you use size_t here so that clang doesn't complain about "comparison
> > of integers of different signs: 'int' and 'unsigned int'" in the for
> > loop below?
> 
> meh, ok :-)

Trivial, I can also fix this locally and ask you if it's OK to squash.

> 
> > > +int ret;
> > > +
> > > +tmp_ctx = talloc_new(mem_ctx);
> > > +if (tmp_ctx == NULL) {
> > > +return ENOMEM;
> > > +}
> > > +
> > > +ret = parse_section(tmp_ctx, section, , NULL);
> > > +if (ret != EOK) {
> > > +goto done;
> > > +}
> > > +
> > > +base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
> > > +if (base == NULL) {
> > > +ret = ENOMEM;
> > > +goto done;
> > > +}
> > > +
> > > +base_comp_num = ldb_dn_get_comp_num(base);
> > > +
> > > +ret = ldb_search(cdb->ldb, tmp_ctx, , base, LDB_SCOPE_SUBTREE,
> > > + attrs, NULL);
> > > +if (ret != LDB_SUCCESS) {
> > > +ret = EIO;
> > > +goto done;
> > > +}
> > > +
> > > +names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
> > > +if (names == NULL) {
> > > +ret = ENOMEM;
> > > +goto done;
> > > +}
> > > +
> > > +for (num = 0, i = 0; i < res->count; i++) {
> > > +const struct ldb_val *val;
> > > +char *name;
> > > +int n;
> > > +int j;
> > 
> > Every time I see variables declared in a scope in C except loop control
> > variables I think "This should be a static function of its own" :-)
> 
> Should it be in this case ? 

Not sure, I'll make up my mind when I fix the other trivial issues.


[...]

> > > From aa6203a0a6cb1f3ac60428887e77fe176489c3e0 Mon Sep 17 00:00:00 2001
> > > From: Christian Heimes 
> > > Date: Fri, 8 Jan 2016 13:26:22 +0100
> > > Subject: [PATCH 08/15] Secrets: m4 macros for jansson and http-parser
> > > 
> > > Prepares autoconf for the new Secrets Provider dependencies
> > > 
> > > Related:
> > > https://fedorahosted.org/sssd/ticket/2913
> > > 
> > 
> > [...]
> > 
> > > +PKG_CHECK_MODULES([HTTP_PARSER], [http_parser], [found_http_parser=yes], 
> > > [found_http_parser=no])
> > 
> > There is no pkgconfig for http-parser-devel, so it seems to be this line
> > is redundant.
> > 
> > Otherwise ACK.
> 
> I wonder why this is not failing then ?

Because we find the library using the next AC_CHECK_LIB call. Again, I
will fixup this locally and send a branch for review later.

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Jakub Hrozek 
On Wed, Apr 20, 2016 at 09:57:03AM -0400, Simo Sorce wrote:
> On Wed, 2016-04-20 at 11:55 +0200, Jakub Hrozek wrote:
> > On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> > > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > > > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > > > at the others.
> > > 
> > > The last coverity/clang thing is a false positive, but I initialized
> > > reply to NULL anyway, I expect now it will start complaining of possible
> > > NULL dereference :-)
> > > 
> > > Attached find patches that fixes all other issues (hopefully), one of
> > > them simply dropped an entire function as it turned out I wasn't using
> > > it.
> > > 
> > > Simo.
> > > 
> > > -- 
> > > Simo Sorce * Red Hat, Inc * New York
> > 
> > > From 02e259e44631d228e0ec8311392e4a1a1a661b89 Mon Sep 17 00:00:00 2001
> > > From: Simo Sorce 
> > > Date: Fri, 8 Jan 2016 17:51:06 -0500
> > > Subject: [PATCH 04/15] Responders: Make the client context more generic
> > 
> > This patch breaks the PAM responder:
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x in ?? ()
> > (gdb) bt
> > #0  0x in ?? ()
> > #1  0x00413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, 
> > flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478
> > #2  0x7f8fe25d5bf3 in epoll_event_loop (tvalp=0x7ffd4e5d41b0, 
> > epoll_ev=0x653c80) at ../tevent_epoll.c:728
> > #3  epoll_event_loop_once (ev=, location=) at 
> > ../tevent_epoll.c:926
> > #4  0x7f8fe25d4137 in std_event_loop_once (ev=0x653a40, 
> > location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
> > ../tevent_standard.c:114
> > #5  0x7f8fe25d038d in _tevent_loop_once (ev=ev@entry=0x653a40, 
> > location=location@entry=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
> > ../tevent.c:533
> > #6  0x7f8fe25d052b in tevent_common_loop_wait (ev=0x653a40, 
> > location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:637
> > #7  0x7f8fe25d40d7 in std_event_loop_wait (ev=0x653a40, 
> > location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
> > ../tevent_standard.c:140
> > #8  0x7f8fe5f67b55 in server_loop (main_ctx=0x654e90) at 
> > /sssd/src/util/server.c:689
> > #9  0x00406f57 in main (argc=6, argv=0x7ffd4e5d4598) at 
> > /sssd/src/responder/pam/pamsrv.c:426
> > (gdb) frame 1
> > #1  0x00413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, 
> > flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478
> > 478 ret = accept_ctx->connection_setup(cctx);
> > (gdb) p accept_ctx
> > $1 = (struct accept_fd_ctx *) 0x6604b0
> > (gdb) p accept_ctx->connection_setup 
> > $2 = (connection_setup_t) 0x0
> > 
> > Do you want me to fix this and proceed or will you?
> 
> If you already know what's wrong, feel free to fix it, if you have to
> spend time analyzing I can do it, should be an easy fix.

Yes, I can prepare a branch on github with fixup patches and ask you if
it's OK to squash the fixups into your patches.

> 
> > The NSS, autofs and IFP responders seem to work fine. I haven't tested
> > the others yet.
> 
> Ok, sorry for that, I did install SSSD at various times, but was more
> concentrated on testing the secrets stuff, and the tests went smoothly,
> but I now realize they fake the connection handling.
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York
> 
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Jakub Hrozek 
On Wed, Apr 20, 2016 at 09:43:05AM -0400, Simo Sorce wrote:
> On Wed, 2016-04-20 at 11:12 +0200, Jakub Hrozek wrote:
> > On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote:
> > > > > From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001
> > > > > From: Simo Sorce 
> > > > > Date: Tue, 12 Jan 2016 20:13:28 -0500
> > > > > Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
> > > > > 
> > > > > This allows the services to self monitor.
> > > > > 
> > > > > Related:
> > > > > https://fedorahosted.org/sssd/ticket/2921
> > > > 
> > > > Is it intentional that we also enable the watchdog in monitor? I haven't
> > > > seen the sssd process being stuck and if it does, we probably have
> > > > bigger issues, so it's probably fine, I just need to remember to not
> > > > SIGSTOP sssd when testing anymore :)
> > > > 
> > > > Otherwise ack.
> > > 
> > > Actually, more questions...
> > > 
> > > Can you help me test this patch? I tried to inject sleep() into sssd_be
> > > code and the sleep was just interrupted by the SIGRT delivery. With SSSD,
> > > most of the time the process was stuck was because it was writing a lot of
> > > data with fsync()/fdatasync(). I can't find any information in the Linux
> > > fsync manpage on how fsync behaves wrt signals. openpub manpages indicate
> > > that fsync would return EINTR, which worries me a bit..
> > 
> > Hmm, sorry, I was not being careful enough. man 7 signal also says:
> > """
> > The sleep(3) function is also never restarted if interrupted by a
> > handler, but gives a success return: the number of seconds remaining to
> > sleep.
> > """
> > 
> > so the sleep testcase was wrong even though CatchSignal uses SA_RESTART.
> > But do you know how would write() or fsync() behave here? The signal
> > manpage is a bit unclar to me as it talks about "slow" devices..
> > 
> > Or can you think of some easy way to test this?
> 
> The fsync manpage here says:
> "The call blocks until the device reports that the transfer has
> completed."
> 
> And does not report EINTR as a possible error.
> 
> That said I am a bit unclear what you want to test actually ?

I want to actually test that the service can be restarted if stuck and
reconnects fine. So far I haven't been lucky - SIGSTOP-ing the service
stopped delivery of the signals, so did attaching gdb and waiting.

But most importantly I want to make sure that if tdb is writing a
transaction and the signal is delivered, then we don't fsync() in tdb
doesn't get interrupted and doesn't corrupt the database. From all the
cases where users complained about a service being restarted, it was
always about tdb writing stuff to disk...

> 
> Yes interruptible calls can be interrupted by a signal, that's always
> the case, if we have code that misbehave when a syscall is interrupted
> we need to fix that code.
> 
> Afaik when we write() we always check the return and retry on EINTR.
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York
> 
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Simo Sorce 
On Wed, 2016-04-20 at 14:16 +0200, Jakub Hrozek wrote:
> On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > > at the others.
> > 
> > The last coverity/clang thing is a false positive, but I initialized
> > reply to NULL anyway, I expect now it will start complaining of possible
> > NULL dereference :-)
> > 
> > Attached find patches that fixes all other issues (hopefully), one of
> > them simply dropped an entire function as it turned out I wasn't using
> > it.
> > 
> > Simo.
> > 
> > -- 
> > Simo Sorce * Red Hat, Inc * New York
> 
> > From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Thu, 7 Jan 2016 10:17:38 -0500
> > Subject: [PATCH 05/15] Responders: Add support for socket activation
> 
> ACK (visual at this point) with a question - do we want to check
> that the fd we received is a UNIX socket using sd_is_socket_unix()?
> 
> The sd_listen_fds() manpage recommends that.

If they recommend it we should, yes.

> > From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Wed, 20 Jan 2016 10:33:39 -0500
> > Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
> > 
> > The secrets database will have "subsections", ie sections that are in the
> > "secrets" namespace and look like this: [secrets/]
> > 
> > This function allows to source any section under secrets/ or under any
> > arbitrary sub-path.
> > 
> > Related:
> > https://fedorahosted.org/sssd/ticket/2913
> > ---
> >  src/confdb/confdb.c | 92 
> > +
> >  src/confdb/confdb.h | 26 +++
> >  2 files changed, 118 insertions(+)
> > 
> > diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
> > index 
> > d409344890c869aa3e7b2dbb49c0f51cd3a20adc..03adbe593b42f556b560df77d410f80460200a67
> >  100644
> > --- a/src/confdb/confdb.c
> > +++ b/src/confdb/confdb.c
> > @@ -1531,3 +1531,95 @@ done:
> >  talloc_free(tmp_ctx);
> >  return ret;
> >  }
> > +
> > +int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
> > +struct confdb_ctx *cdb,
> > +const char *section,
> > +char ***sections,
> > +int *num_sections)
> > +{
> > +TALLOC_CTX *tmp_ctx = NULL;
> > +char *secdn;
> > +struct ldb_dn *base = NULL;
> > +struct ldb_result *res = NULL;
> > +static const char *attrs[] = {"cn", NULL};
> > +char **names;
> > +int base_comp_num;
> > +int num;
> > +int i;
> 
> Can you use size_t here so that clang doesn't complain about "comparison
> of integers of different signs: 'int' and 'unsigned int'" in the for
> loop below?

meh, ok :-)

> > +int ret;
> > +
> > +tmp_ctx = talloc_new(mem_ctx);
> > +if (tmp_ctx == NULL) {
> > +return ENOMEM;
> > +}
> > +
> > +ret = parse_section(tmp_ctx, section, , NULL);
> > +if (ret != EOK) {
> > +goto done;
> > +}
> > +
> > +base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
> > +if (base == NULL) {
> > +ret = ENOMEM;
> > +goto done;
> > +}
> > +
> > +base_comp_num = ldb_dn_get_comp_num(base);
> > +
> > +ret = ldb_search(cdb->ldb, tmp_ctx, , base, LDB_SCOPE_SUBTREE,
> > + attrs, NULL);
> > +if (ret != LDB_SUCCESS) {
> > +ret = EIO;
> > +goto done;
> > +}
> > +
> > +names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
> > +if (names == NULL) {
> > +ret = ENOMEM;
> > +goto done;
> > +}
> > +
> > +for (num = 0, i = 0; i < res->count; i++) {
> > +const struct ldb_val *val;
> > +char *name;
> > +int n;
> > +int j;
> 
> Every time I see variables declared in a scope in C except loop control
> variables I think "This should be a static function of its own" :-)

Should it be in this case ? 

> > +
> > +n = ldb_dn_get_comp_num(res->msgs[i]->dn);
> > +if (n == base_comp_num) continue;
> > +
> > +name = NULL;
> > +for (j = n - base_comp_num - 1; j >= 0; j--) {
> > +val = ldb_dn_get_component_val(res->msgs[i]->dn, j);
> > +if (name == NULL) {
> > +name = talloc_strndup(names,
> > +  (const char *)val->data, 
> > val->length);
> > +} else {
> > +name = talloc_asprintf(names, "%s/%.*s", name,
> > +   (int)val->length,
> > +   (const char *)val->data);
> > +}
> > +if (name == NULL) {
> > +ret = ENOMEM;
> > +goto done;
> > +}
> > +}
> > +
> > +names[num] = name;
> > +if

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Simo Sorce 
On Wed, 2016-04-20 at 11:55 +0200, Jakub Hrozek wrote:
> On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > > at the others.
> > 
> > The last coverity/clang thing is a false positive, but I initialized
> > reply to NULL anyway, I expect now it will start complaining of possible
> > NULL dereference :-)
> > 
> > Attached find patches that fixes all other issues (hopefully), one of
> > them simply dropped an entire function as it turned out I wasn't using
> > it.
> > 
> > Simo.
> > 
> > -- 
> > Simo Sorce * Red Hat, Inc * New York
> 
> > From 02e259e44631d228e0ec8311392e4a1a1a661b89 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Fri, 8 Jan 2016 17:51:06 -0500
> > Subject: [PATCH 04/15] Responders: Make the client context more generic
> 
> This patch breaks the PAM responder:
> Program received signal SIGSEGV, Segmentation fault.
> 0x in ?? ()
> (gdb) bt
> #0  0x in ?? ()
> #1  0x00413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, 
> flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478
> #2  0x7f8fe25d5bf3 in epoll_event_loop (tvalp=0x7ffd4e5d41b0, 
> epoll_ev=0x653c80) at ../tevent_epoll.c:728
> #3  epoll_event_loop_once (ev=, location=) at 
> ../tevent_epoll.c:926
> #4  0x7f8fe25d4137 in std_event_loop_once (ev=0x653a40, 
> location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
> ../tevent_standard.c:114
> #5  0x7f8fe25d038d in _tevent_loop_once (ev=ev@entry=0x653a40, 
> location=location@entry=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
> ../tevent.c:533
> #6  0x7f8fe25d052b in tevent_common_loop_wait (ev=0x653a40, 
> location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:637
> #7  0x7f8fe25d40d7 in std_event_loop_wait (ev=0x653a40, 
> location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
> ../tevent_standard.c:140
> #8  0x7f8fe5f67b55 in server_loop (main_ctx=0x654e90) at 
> /sssd/src/util/server.c:689
> #9  0x00406f57 in main (argc=6, argv=0x7ffd4e5d4598) at 
> /sssd/src/responder/pam/pamsrv.c:426
> (gdb) frame 1
> #1  0x00413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, 
> flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478
> 478 ret = accept_ctx->connection_setup(cctx);
> (gdb) p accept_ctx
> $1 = (struct accept_fd_ctx *) 0x6604b0
> (gdb) p accept_ctx->connection_setup 
> $2 = (connection_setup_t) 0x0
> 
> Do you want me to fix this and proceed or will you?

If you already know what's wrong, feel free to fix it, if you have to
spend time analyzing I can do it, should be an easy fix.

> The NSS, autofs and IFP responders seem to work fine. I haven't tested
> the others yet.

Ok, sorry for that, I did install SSSD at various times, but was more
concentrated on testing the secrets stuff, and the tests went smoothly,
but I now realize they fake the connection handling.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Simo Sorce 
On Wed, 2016-04-20 at 11:12 +0200, Jakub Hrozek wrote:
> On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote:
> > > > From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001
> > > > From: Simo Sorce 
> > > > Date: Tue, 12 Jan 2016 20:13:28 -0500
> > > > Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
> > > > 
> > > > This allows the services to self monitor.
> > > > 
> > > > Related:
> > > > https://fedorahosted.org/sssd/ticket/2921
> > > 
> > > Is it intentional that we also enable the watchdog in monitor? I haven't
> > > seen the sssd process being stuck and if it does, we probably have
> > > bigger issues, so it's probably fine, I just need to remember to not
> > > SIGSTOP sssd when testing anymore :)
> > > 
> > > Otherwise ack.
> > 
> > Actually, more questions...
> > 
> > Can you help me test this patch? I tried to inject sleep() into sssd_be
> > code and the sleep was just interrupted by the SIGRT delivery. With SSSD,
> > most of the time the process was stuck was because it was writing a lot of
> > data with fsync()/fdatasync(). I can't find any information in the Linux
> > fsync manpage on how fsync behaves wrt signals. openpub manpages indicate
> > that fsync would return EINTR, which worries me a bit..
> 
> Hmm, sorry, I was not being careful enough. man 7 signal also says:
> """
> The sleep(3) function is also never restarted if interrupted by a
> handler, but gives a success return: the number of seconds remaining to
> sleep.
> """
> 
> so the sleep testcase was wrong even though CatchSignal uses SA_RESTART.
> But do you know how would write() or fsync() behave here? The signal
> manpage is a bit unclar to me as it talks about "slow" devices..
> 
> Or can you think of some easy way to test this?

The fsync manpage here says:
"The call blocks until the device reports that the transfer has
completed."

And does not report EINTR as a possible error.

That said I am a bit unclear what you want to test actually ?

Yes interruptible calls can be interrupted by a signal, that's always
the case, if we have code that misbehave when a syscall is interrupted
we need to fix that code.

Afaik when we write() we always check the return and retry on EINTR.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Pavel Reichl 



On 04/20/2016 02:16 PM, Jakub Hrozek wrote:

+for (num = 0, i = 0; i < res->count; i++) {
>+const struct ldb_val *val;
>+char *name;
>+int n;
>+int j;

Every time I see variables declared in a scope in C except loop control
variables I think "This should be a static function of its own":-)


Yes, and I think it's great that Simo does so, instead of hiding this fact by 
placing all variables at the beginning of the function. Limiting scope of 
variables as much as possible is a good think!
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Jakub Hrozek 
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > at the others.
> 
> The last coverity/clang thing is a false positive, but I initialized
> reply to NULL anyway, I expect now it will start complaining of possible
> NULL dereference :-)
> 
> Attached find patches that fixes all other issues (hopefully), one of
> them simply dropped an entire function as it turned out I wasn't using
> it.
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

> From 4610b546cb37a150ebaee12559c19a17e422708c Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Thu, 7 Jan 2016 10:17:38 -0500
> Subject: [PATCH 05/15] Responders: Add support for socket activation

ACK (visual at this point) with a question - do we want to check
that the fd we received is a UNIX socket using sd_is_socket_unix()?

The sd_listen_fds() manpage recommends that.

> From 3755b157de1309f554a380e58c42c38dcd9cc5aa Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Wed, 20 Jan 2016 10:33:39 -0500
> Subject: [PATCH 06/15] ConfDB: Add helper function to get "subsections"
> 
> The secrets database will have "subsections", ie sections that are in the
> "secrets" namespace and look like this: [secrets/]
> 
> This function allows to source any section under secrets/ or under any
> arbitrary sub-path.
> 
> Related:
> https://fedorahosted.org/sssd/ticket/2913
> ---
>  src/confdb/confdb.c | 92 
> +
>  src/confdb/confdb.h | 26 +++
>  2 files changed, 118 insertions(+)
> 
> diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c
> index 
> d409344890c869aa3e7b2dbb49c0f51cd3a20adc..03adbe593b42f556b560df77d410f80460200a67
>  100644
> --- a/src/confdb/confdb.c
> +++ b/src/confdb/confdb.c
> @@ -1531,3 +1531,95 @@ done:
>  talloc_free(tmp_ctx);
>  return ret;
>  }
> +
> +int confdb_get_sub_sections(TALLOC_CTX *mem_ctx,
> +struct confdb_ctx *cdb,
> +const char *section,
> +char ***sections,
> +int *num_sections)
> +{
> +TALLOC_CTX *tmp_ctx = NULL;
> +char *secdn;
> +struct ldb_dn *base = NULL;
> +struct ldb_result *res = NULL;
> +static const char *attrs[] = {"cn", NULL};
> +char **names;
> +int base_comp_num;
> +int num;
> +int i;

Can you use size_t here so that clang doesn't complain about "comparison
of integers of different signs: 'int' and 'unsigned int'" in the for
loop below?

> +int ret;
> +
> +tmp_ctx = talloc_new(mem_ctx);
> +if (tmp_ctx == NULL) {
> +return ENOMEM;
> +}
> +
> +ret = parse_section(tmp_ctx, section, , NULL);
> +if (ret != EOK) {
> +goto done;
> +}
> +
> +base = ldb_dn_new(tmp_ctx, cdb->ldb, secdn);
> +if (base == NULL) {
> +ret = ENOMEM;
> +goto done;
> +}
> +
> +base_comp_num = ldb_dn_get_comp_num(base);
> +
> +ret = ldb_search(cdb->ldb, tmp_ctx, , base, LDB_SCOPE_SUBTREE,
> + attrs, NULL);
> +if (ret != LDB_SUCCESS) {
> +ret = EIO;
> +goto done;
> +}
> +
> +names = talloc_zero_array(tmp_ctx, char *, res->count + 1);
> +if (names == NULL) {
> +ret = ENOMEM;
> +goto done;
> +}
> +
> +for (num = 0, i = 0; i < res->count; i++) {
> +const struct ldb_val *val;
> +char *name;
> +int n;
> +int j;

Every time I see variables declared in a scope in C except loop control
variables I think "This should be a static function of its own" :-)

> +
> +n = ldb_dn_get_comp_num(res->msgs[i]->dn);
> +if (n == base_comp_num) continue;
> +
> +name = NULL;
> +for (j = n - base_comp_num - 1; j >= 0; j--) {
> +val = ldb_dn_get_component_val(res->msgs[i]->dn, j);
> +if (name == NULL) {
> +name = talloc_strndup(names,
> +  (const char *)val->data, val->length);
> +} else {
> +name = talloc_asprintf(names, "%s/%.*s", name,
> +   (int)val->length,
> +   (const char *)val->data);
> +}
> +if (name == NULL) {
> +ret = ENOMEM;
> +goto done;
> +}
> +}
> +
> +names[num] = name;
> +if (names[num] == NULL) {
> +ret = ENOMEM;
> +goto done;
> +}
> +
> +num++;
> +}

> From 082d09cac5919d651accda2d4163deb022fcb7f6 Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Fri, 8 Jan 2016 10:56:17 -0500
> Subject: [PATCH 07/15] Secrets: Add autoconf macros to build with secrets
> 
> Prepares autoconf for the new Secrets Provider

ACK

> From

[SSSD] Re: [PATCH] intg: Use different uid range for add_remove tests

2016-04-20 Thread Petr Cech 

On 04/20/2016 12:52 PM, Lukas Slebodnik wrote:

On (20/04/16 12:36), Petr Cech wrote:

>On 04/18/2016 10:34 AM, Lukas Slebodnik wrote:

>>ehlo,
>>
>>I use special local user for building srpms in mock
>>and it caused failures for me with running integration tests.
>>
>>Attached patch is a workaround. The proper solution would be to wrap
>>detection of active users in CWRAP enviroment.
>>
>>LS

>
>Hi Lukas,
>
>I see it is Subject: [PATCH 2/3] intg: Use different... Is this one patch all

It is a single patch.
I just generated more patches together:-)


OK :-)


LS


>what you want to attached? I just check it.
>
>The code looks good to me => I will add final ACK after CI passed.
>
>CI hash: b3ca357..eb352af
>

thank you for review


You're welcome.


LS


CI passed:
http://sssd-ci.duckdns.org/logs/job/42/49/summary.html

ACK (final)

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] Do not crash if GetUserAttrs cannot be parsed

2016-04-20 Thread Pavel Březina 

On 04/20/2016 11:56 AM, Jakub Hrozek wrote:

Hi Pavel,

can you check if this is the right thing to do for methods that parse
arguments on their own?

To reproduce, it was enough to run:
 sudo dbus-send --print-reply --system
 --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe
 org.freedesktop.sssd.infopipe.GetUserAttr string:admin
 string:gecos
instead of the proper:
 sudo dbus-send --print-reply --system
 --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe
 org.freedesktop.sssd.infopipe.GetUserAttr string:admin
 array:string:gecos


0001-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch


 From 220b9124e81961a3febe814a0cb70d4fcfc2ade2 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek
Date: Wed, 20 Apr 2016 11:54:31 +0200
Subject: [PATCH] IFP: Do not crash on invalid arguments to GetUserAttr

---
  src/responder/ifp/ifpsrv_cmd.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
index 
399e83e0255700df5a24491a3eb33b4f92040ca7..43dbce53c0395edfd7e8e6d87b609c52e835eaa0
 100644
--- a/src/responder/ifp/ifpsrv_cmd.c
+++ b/src/responder/ifp/ifpsrv_cmd.c
@@ -117,7 +117,7 @@ ifp_user_get_attr_unpack_msg(struct ifp_attr_req *attr_req)
DBUS_TYPE_INVALID);
  if (parsed == false) {
  DEBUG(SSSDBG_OP_FAILURE, "Could not parse arguments\n");
-return EOK; /* handled */
+return EINVAL;
  }

  /* Copy the attributes to maintain memory hierarchy with talloc */
-- 2.4.11



An sbus method handler returns EOK if the message was handled (reply was 
sent) even if this was an error case. See:



void
sbus_request_invoke_or_finish(struct sbus_request *dbus_req,
  sbus_msg_handler_fn handler_fn,
  void *handler_data,
  sbus_method_invoker_fn invoker_fn)
{
DBusError error;
int ret;

if (invoker_fn != NULL) {
ret = invoker_fn(dbus_req, handler_fn);
} else if (handler_fn != NULL) {
ret = handler_fn(dbus_req, handler_data);
} else {
ret = EINVAL;
}

switch(ret) {
case EOK:
return;


^ message was handled, therefore we do not do anything else here


case ENOMEM:
DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory handling DBus message\n");
sbus_request_finish(dbus_req, NULL);
break;
default:
dbus_error_init();
dbus_set_error_const(, DBUS_ERROR_FAILED, INTERNAL_ERROR);
sbus_request_fail_and_finish(dbus_req, );
break;


^ otherwise we report error


}
}


Because sbus_request is freed when you sent reply over 
sbus_request_finish/fail_and_finish or whatever (which happens 
internally when the params are invalid) you access freed memory in the 
default case if !EOK is returned.


So you can return EINVAL in ifp_user_get_attr_unpack_msg but you need to 
return EOK from ifp_user_get_attr in this case. Maybe we can create a 
custom code ERR_SBUS_MESSAGE_HANDLED? for this.

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] intg: Use different uid range for add_remove tests

2016-04-20 Thread Lukas Slebodnik 
On (20/04/16 12:36), Petr Cech wrote:
>On 04/18/2016 10:34 AM, Lukas Slebodnik wrote:
>> ehlo,
>> 
>> I use special local user for building srpms in mock
>> and it caused failures for me with running integration tests.
>> 
>> Attached patch is a workaround. The proper solution would be to wrap
>> detection of active users in CWRAP enviroment.
>> 
>> LS
>
>Hi Lukas,
>
>I see it is Subject: [PATCH 2/3] intg: Use different... Is this one patch all
It is a single patch.
I just generated more patches together :-)

LS

>what you want to attached? I just check it.
>
>The code looks good to me => I will add final ACK after CI passed.
>
>CI hash: b3ca357..eb352af
>
thank you for review

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [DESIGN] sss_confcheck tool

2016-04-20 Thread Jakub Hrozek 
On Wed, Apr 20, 2016 at 12:04:23PM +0200, Michal Židek wrote:
> Hi,
> 
> I just updated the design for the planned sss_confcheck tool.
> It can be found here:
> https://fedorahosted.org/sssd/wiki/DesignDocs/ConfigCheckTool
> 
> For convenience I paste it here as well:
> 
> 
> sss_confcheck tool
> 
> Related ticket(s):
> 
> ​https://fedorahosted.org/sssd/ticket/2269
> 
> Problem statement
> 
> There is no easy way to debug the SSSD configuration without having to look
> into the debug logs. Moreover the debug logs can be difficult to understand
> for people outside SSSD development team. Some common issues can be
> identified during static offline analysis of the config files. To find these
> issues soon we need a tool that performs this analysis and provides human
> readable report.
> 
> Use cases
> 
> - performing ad-hoc static analysis of the installed SSSD configuration
> - performing ad-hoc static analysis of SSSD configuration files retrieved
> from user with some SSSD problems
> 
> Overview of the solution - sss_confcheck tool
> 
> A new tool will be added to sss_* tools that will perform static analysis of
> SSSD configuration files. This tool can be run without any parameters in
> which case it will print a report to the standard output in the following or
> similar format:
> 
> -
> $ sss_confcheck
> Number of identified issues: 1
> [rule/allowed_nss_options]: Attribute 'foo' is not allowed in section 'nss'.
> Check for typos.

It might be nice to explicitly say what the validators will do in the
upcoming version (I know you linked the design document...)

> 
> Used configuration file:
> 
> 
> Number of used configuration override snippets: 2
> List of configuration override snippets in order of priority (lowest
> priority first):
> snippet_name_1.conf
> snippet_name_2.conf
> 
> Content of configuration override snippets:
> snippet_name_1.conf:
> 
> 
> snippet_name_2.conf:
> 

Would it make sense to also print the resulting config file (merged with
snippets) ?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] intg: Use different uid range for add_remove tests

2016-04-20 Thread Petr Cech 

On 04/18/2016 10:34 AM, Lukas Slebodnik wrote:

ehlo,

I use special local user for building srpms in mock
and it caused failures for me with running integration tests.

Attached patch is a workaround. The proper solution would be to wrap
detection of active users in CWRAP enviroment.

LS


Hi Lukas,

I see it is Subject: [PATCH 2/3] intg: Use different... Is this one 
patch all what you want to attached? I just check it.


The code looks good to me => I will add final ACK after CI passed.

CI hash: b3ca357..eb352af

Regards

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [DESIGN] sss_confcheck tool

2016-04-20 Thread Michal Židek 

Hi,

I just updated the design for the planned sss_confcheck tool.
It can be found here: 
https://fedorahosted.org/sssd/wiki/DesignDocs/ConfigCheckTool


For convenience I paste it here as well:


sss_confcheck tool

Related ticket(s):

​https://fedorahosted.org/sssd/ticket/2269

Problem statement

There is no easy way to debug the SSSD configuration without having to 
look into the debug logs. Moreover the debug logs can be difficult to 
understand for people outside SSSD development team. Some common issues 
can be identified during static offline analysis of the config files. To 
find these issues soon we need a tool that performs this analysis and 
provides human readable report.


Use cases

- performing ad-hoc static analysis of the installed SSSD   configuration
- performing ad-hoc static analysis of SSSD configuration files 
retrieved from user with some SSSD problems


Overview of the solution - sss_confcheck tool

A new tool will be added to sss_* tools that will perform static 
analysis of SSSD configuration files. This tool can be run without any 
parameters in which case it will print a report to the standard output 
in the following or similar format:


-
$ sss_confcheck
Number of identified issues: 1
[rule/allowed_nss_options]: Attribute 'foo' is not allowed in section 
'nss'. Check for typos.


Used configuration file:


Number of used configuration override snippets: 2
List of configuration override snippets in order of priority (lowest 
priority first):

snippet_name_1.conf
snippet_name_2.conf

Content of configuration override snippets:
snippet_name_1.conf:


snippet_name_2.conf:

---

Available options:

  ?, --help
  --config-file PATH_CONFIG_FILEPath to config file 
that will be checked.

  --snippets-dir PATH_TO_SNIPPETS_DIRECTORY Path to snippets directory.
  --no-validators   Do not use validators 
(no analysis will be made).
  --no-file-content Do not print config 
file or snippet contents.

  --no-snippets Ignore the snippets.
  --silent  If no errors are 
detected, do not print anything.


Implementation details

The tool will use ding-libs validators feature described in this design 
document ​ 
https://fedorahosted.org/sssd/wiki/DesignDocs/libini-config-file-checks.


Configuration changes

No configuration changes.

How To Test

Depending on the capabilities of validators used by SSSD, make an error 
in configuration and run sss_confcheck to see if it was detected.

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [PATCH] Do not crash if GetUserAttrs cannot be parsed

2016-04-20 Thread Jakub Hrozek 
Hi Pavel,

can you check if this is the right thing to do for methods that parse
arguments on their own?

To reproduce, it was enough to run:
sudo dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe
org.freedesktop.sssd.infopipe.GetUserAttr string:admin
string:gecos
instead of the proper:
sudo dbus-send --print-reply --system
--dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe
org.freedesktop.sssd.infopipe.GetUserAttr string:admin
array:string:gecos
>From 220b9124e81961a3febe814a0cb70d4fcfc2ade2 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek 
Date: Wed, 20 Apr 2016 11:54:31 +0200
Subject: [PATCH] IFP: Do not crash on invalid arguments to GetUserAttr

---
 src/responder/ifp/ifpsrv_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
index 
399e83e0255700df5a24491a3eb33b4f92040ca7..43dbce53c0395edfd7e8e6d87b609c52e835eaa0
 100644
--- a/src/responder/ifp/ifpsrv_cmd.c
+++ b/src/responder/ifp/ifpsrv_cmd.c
@@ -117,7 +117,7 @@ ifp_user_get_attr_unpack_msg(struct ifp_attr_req *attr_req)
   DBUS_TYPE_INVALID);
 if (parsed == false) {
 DEBUG(SSSDBG_OP_FAILURE, "Could not parse arguments\n");
-return EOK; /* handled */
+return EINVAL;
 }
 
 /* Copy the attributes to maintain memory hierarchy with talloc */
-- 
2.4.11

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Jakub Hrozek 
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > at the others.
> 
> The last coverity/clang thing is a false positive, but I initialized
> reply to NULL anyway, I expect now it will start complaining of possible
> NULL dereference :-)
> 
> Attached find patches that fixes all other issues (hopefully), one of
> them simply dropped an entire function as it turned out I wasn't using
> it.
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

> From 02e259e44631d228e0ec8311392e4a1a1a661b89 Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Fri, 8 Jan 2016 17:51:06 -0500
> Subject: [PATCH 04/15] Responders: Make the client context more generic

This patch breaks the PAM responder:
Program received signal SIGSEGV, Segmentation fault.
0x in ?? ()
(gdb) bt
#0  0x in ?? ()
#1  0x00413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, 
flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478
#2  0x7f8fe25d5bf3 in epoll_event_loop (tvalp=0x7ffd4e5d41b0, 
epoll_ev=0x653c80) at ../tevent_epoll.c:728
#3  epoll_event_loop_once (ev=, location=) at 
../tevent_epoll.c:926
#4  0x7f8fe25d4137 in std_event_loop_once (ev=0x653a40, 
location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
../tevent_standard.c:114
#5  0x7f8fe25d038d in _tevent_loop_once (ev=ev@entry=0x653a40, 
location=location@entry=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
../tevent.c:533
#6  0x7f8fe25d052b in tevent_common_loop_wait (ev=0x653a40, 
location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at ../tevent.c:637
#7  0x7f8fe25d40d7 in std_event_loop_wait (ev=0x653a40, 
location=0x7f8fe5f88aed "/sssd/src/util/server.c:689") at 
../tevent_standard.c:140
#8  0x7f8fe5f67b55 in server_loop (main_ctx=0x654e90) at 
/sssd/src/util/server.c:689
#9  0x00406f57 in main (argc=6, argv=0x7ffd4e5d4598) at 
/sssd/src/responder/pam/pamsrv.c:426
(gdb) frame 1
#1  0x00413512 in accept_fd_handler (ev=0x653a40, fde=0x668970, 
flags=1, ptr=0x6604b0) at /sssd/src/responder/common/responder_common.c:478
478 ret = accept_ctx->connection_setup(cctx);
(gdb) p accept_ctx
$1 = (struct accept_fd_ctx *) 0x6604b0
(gdb) p accept_ctx->connection_setup 
$2 = (connection_setup_t) 0x0

Do you want me to fix this and proceed or will you?

The NSS, autofs and IFP responders seem to work fine. I haven't tested
the others yet.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Jakub Hrozek 
On Wed, Apr 20, 2016 at 10:32:59AM +0200, Jakub Hrozek wrote:
> > > From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001
> > > From: Simo Sorce 
> > > Date: Tue, 12 Jan 2016 20:13:28 -0500
> > > Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
> > > 
> > > This allows the services to self monitor.
> > > 
> > > Related:
> > > https://fedorahosted.org/sssd/ticket/2921
> > 
> > Is it intentional that we also enable the watchdog in monitor? I haven't
> > seen the sssd process being stuck and if it does, we probably have
> > bigger issues, so it's probably fine, I just need to remember to not
> > SIGSTOP sssd when testing anymore :)
> > 
> > Otherwise ack.
> 
> Actually, more questions...
> 
> Can you help me test this patch? I tried to inject sleep() into sssd_be
> code and the sleep was just interrupted by the SIGRT delivery. With SSSD,
> most of the time the process was stuck was because it was writing a lot of
> data with fsync()/fdatasync(). I can't find any information in the Linux
> fsync manpage on how fsync behaves wrt signals. openpub manpages indicate
> that fsync would return EINTR, which worries me a bit..

Hmm, sorry, I was not being careful enough. man 7 signal also says:
"""
The sleep(3) function is also never restarted if interrupted by a
handler, but gives a success return: the number of seconds remaining to
sleep.
"""

so the sleep testcase was wrong even though CatchSignal uses SA_RESTART.
But do you know how would write() or fsync() behave here? The signal
manpage is a bit unclar to me as it talks about "slow" devices..

Or can you think of some easy way to test this?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Jakub Hrozek 
On Wed, Apr 20, 2016 at 09:59:57AM +0200, Jakub Hrozek wrote:
> On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> > On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > > at the others.
> > 
> > The last coverity/clang thing is a false positive, but I initialized
> > reply to NULL anyway, I expect now it will start complaining of possible
> > NULL dereference :-)
> > 
> > Attached find patches that fixes all other issues (hopefully), one of
> > them simply dropped an entire function as it turned out I wasn't using
> > it.
> > 
> > Simo.
> > 
> > -- 
> > Simo Sorce * Red Hat, Inc * New York
> 
> Ugh, it took me too long to get to this review properly. Sorry about
> this..
> 
> Since this patchset is large, I will send replies in batches.
> 
> > From e215e5534bb56f3887521443ce6c77d13ea3518d Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Tue, 12 Jan 2016 20:07:59 -0500
> > Subject: [PATCH 01/15] Util: Add watchdog helper
> 
> ACK
> 
> I know Pavel Brezina also reviewed this patch earlier in a separate thread,
> so note to self: Add his RB :-)
> 
> > From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Tue, 12 Jan 2016 20:13:28 -0500
> > Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
> > 
> > This allows the services to self monitor.
> > 
> > Related:
> > https://fedorahosted.org/sssd/ticket/2921
> 
> Is it intentional that we also enable the watchdog in monitor? I haven't
> seen the sssd process being stuck and if it does, we probably have
> bigger issues, so it's probably fine, I just need to remember to not
> SIGSTOP sssd when testing anymore :)
> 
> Otherwise ack.

Actually, more questions...

Can you help me test this patch? I tried to inject sleep() into sssd_be
code and the sleep was just interrupted by the SIGRT delivery. With SSSD,
most of the time the process was stuck was because it was writing a lot of
data with fsync()/fdatasync(). I can't find any information in the Linux
fsync manpage on how fsync behaves wrt signals. openpub manpages indicate
that fsync would return EINTR, which worries me a bit..

> 
> > From c576e37c188ded932996c9714b18a71251ef1d63 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Wed, 13 Jan 2016 11:51:09 -0500
> > Subject: [PATCH 03/15] Monitor: Remove ping infrastructure
> 
> Can we also remove the force_timeout option and the functions that use
> it? Looking at monitor.c, they are only called from a single failure
> situation which is ENOMEM, so I think it's actually a dead code, the
> probability we ever call it is very low.
> 
> If you agree with removing the force_timeout but you're busy, let me know
> and I can prepare a patch atop yours.
> 
> The only code that proved to be useful in the field of all this we are
> about to remove is the diag_cmd(). We could use it to run pstack on the
> 'stuck' child to learn where it was actually stuck. But I'm not sure
> it's possible to implement it since the watchdog handler is a "plain"
> signal handler and the diag_cmd() forks and execs...

After reading man 7 signal, it seems that fork() and exec() are OK to be
called in a signal handler, but the question above is more urgent IMO..

Also, is there a reason to just deprecate the ping() method and not
remove it right away?

> 
> That said, I still think it's worth removing the pings in favor of the
> watchdog, the pings proved to be too fragile..
> 
> Maybe it would be better to file a ticket to check again if systemd
> watchdog can be used in the future?
> 
> To be continued..
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET} A new Secrets service

2016-04-20 Thread Jakub Hrozek 
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > at the others.
> 
> The last coverity/clang thing is a false positive, but I initialized
> reply to NULL anyway, I expect now it will start complaining of possible
> NULL dereference :-)
> 
> Attached find patches that fixes all other issues (hopefully), one of
> them simply dropped an entire function as it turned out I wasn't using
> it.
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

Ugh, it took me too long to get to this review properly. Sorry about
this..

Since this patchset is large, I will send replies in batches.

> From e215e5534bb56f3887521443ce6c77d13ea3518d Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Tue, 12 Jan 2016 20:07:59 -0500
> Subject: [PATCH 01/15] Util: Add watchdog helper

ACK

I know Pavel Brezina also reviewed this patch earlier in a separate thread,
so note to self: Add his RB :-)

> From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Tue, 12 Jan 2016 20:13:28 -0500
> Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
> 
> This allows the services to self monitor.
> 
> Related:
> https://fedorahosted.org/sssd/ticket/2921

Is it intentional that we also enable the watchdog in monitor? I haven't
seen the sssd process being stuck and if it does, we probably have
bigger issues, so it's probably fine, I just need to remember to not
SIGSTOP sssd when testing anymore :)

Otherwise ack.

> From c576e37c188ded932996c9714b18a71251ef1d63 Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Wed, 13 Jan 2016 11:51:09 -0500
> Subject: [PATCH 03/15] Monitor: Remove ping infrastructure

Can we also remove the force_timeout option and the functions that use
it? Looking at monitor.c, they are only called from a single failure
situation which is ENOMEM, so I think it's actually a dead code, the
probability we ever call it is very low.

If you agree with removing the force_timeout but you're busy, let me know
and I can prepare a patch atop yours.

The only code that proved to be useful in the field of all this we are
about to remove is the diag_cmd(). We could use it to run pstack on the
'stuck' child to learn where it was actually stuck. But I'm not sure
it's possible to implement it since the watchdog handler is a "plain"
signal handler and the diag_cmd() forks and execs...

That said, I still think it's worth removing the pings in favor of the
watchdog, the pings proved to be too fragile..

Maybe it would be better to file a ticket to check again if systemd
watchdog can be used in the future?

To be continued..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] ldap_test.py: Modify enum cache timeouts

2016-04-20 Thread Michal Židek 

On 04/18/2016 12:39 PM, Michal Židek wrote:

On 04/18/2016 12:22 PM, Lukas Slebodnik wrote:

On (18/04/16 11:14), Michal Židek wrote:

On 04/18/2016 10:39 AM, Lukas Slebodnik wrote:

On (02/12/15 17:10), Michal Židek wrote:

Hi!

I saw some integration tests failures recently,
and I think there is a race condition between the
enumeration refresh timeout and the sleeps
after some operations that wait for this timeout.
SSSD fails to populate changes from LDAP in time
and some asserts can fail because of this.

So far I saw 4 tests to fail like this, which
is already quite a lot.

The attached patch modifies the timeout values
and hopefully removes the issue.

Michal


I think I found alternative solution for intermittent
failures of ADD REMOVE test with enumeration.


Not sure if this is safer than my patch. I made the
timeouts bigger on purpose, so that we avoid
problems with machines under heavy load.

You decided to go the opposite direction by
making one of the timeouts shorter.

That being said, maybe your patch is better,
because if it does not fail even under heavy
load it will make the tests shorter.

Once the CI is up we can try 20 test
runs with your patch and if they do not
fail I will give you an ACK.

If they fail, we can still fallback to my patch.


No,
We will need to find another solution.
Increasing timeout is not acceptable from long term perspective.
We need to have faster tests and not slower.

LS


Then lets hope your patch will work :D


Your patch did not fix the issue.
It still fails cca 1 out of 5 times because
in the enumeration tests.

Maybe the timeout magic needs more mana
or there is some other issue which we do not see.

Michal
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] NEGCACHE: Removing of condition for ttl = -1

2016-04-20 Thread Petr Cech 

On 04/19/2016 01:55 PM, Petr Cech wrote:

Hi,

I found a strange condition in the function sss_ncache_check_str().

This condition causes the cache is NOT checked and the result of
checking is automatically EEXIST.

I dind't find call of sss_ncache_check_str() with ttl = -1, except in
tests.

Note: We use value 0 for permanent cache, no -1.



Hello,

I added one another little patch to this, so there is patch set.

Regards

--
Petr^4 Čech
>From 9b8a7a4ca917134db82fb0f8992ab933a72ebd1e Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 19 Apr 2016 13:20:25 -0400
Subject: [PATCH 1/2] NEGCACHE: Fixing typo in test_sss_ncache_gid()

There were sss_ncache_*_uid() functions instead of
sss_ncache_*_gid() functions.

Resolves:
https://fedorahosted.org/sssd/ticket/2928
---
 src/tests/cmocka/test_negcache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/tests/cmocka/test_negcache.c b/src/tests/cmocka/test_negcache.c
index 274e1543cc842bbb6b125423a76cf665354cc059..b3118abdc76c38539d725e7924b3905638438005 100644
--- a/src/tests/cmocka/test_negcache.c
+++ b/src/tests/cmocka/test_negcache.c
@@ -265,10 +265,10 @@ static void test_sss_ncache_gid(void **state)
 assert_int_equal(ret, EEXIST);
 
 permanent = false;
-ret = sss_ncache_set_uid(ts->ctx, permanent, NULL, gid);
+ret = sss_ncache_set_gid(ts->ctx, permanent, NULL, gid);
 assert_int_equal(ret, EOK);
 
-ret = sss_ncache_check_uid(ts->ctx, ttl, NULL, gid);
+ret = sss_ncache_check_gid(ts->ctx, ttl, NULL, gid);
 assert_int_equal(ret, EEXIST);
 
 /* test when ttl is -1 with gid present in database*/
-- 
2.5.5

>From cd5b2217d22f1da7d64cf81e67ac31f823d40e06 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 19 Apr 2016 07:35:26 -0400
Subject: [PATCH 2/2] NEGCACHE: Removing of condition for ttl = -1

If ttl = -1 then function sss_ncache_check_str() returns EEXIST without
checking negcache. This behaviour is out of logic.

We use ttl = 0 for permanent caching.

Resolves:
https://fedorahosted.org/sssd/ticket/2928
---
 src/responder/common/negcache.c  |  6 --
 src/tests/cmocka/test_negcache.c | 46 
 2 files changed, 52 deletions(-)

diff --git a/src/responder/common/negcache.c b/src/responder/common/negcache.c
index 5b0517ceba85d6e35515a935423412314c218143..1617bf8c5cf7d36e7091a000f6473d1bcfe44f3f 100644
--- a/src/responder/common/negcache.c
+++ b/src/responder/common/negcache.c
@@ -97,12 +97,6 @@ static int sss_ncache_check_str(struct sss_nc_ctx *ctx, char *str, int ttl)
 goto done;
 }
 
-if (ttl == -1) {
-/* a negative ttl means: never expires */
-ret = EEXIST;
-goto done;
-}
-
 errno = 0;
 timestamp = strtoull((const char *)data.dptr, , 10);
 if (errno != 0 || *ep != '\0') {
diff --git a/src/tests/cmocka/test_negcache.c b/src/tests/cmocka/test_negcache.c
index b3118abdc76c38539d725e7924b3905638438005..45dcd43c89f036aa758b0669f319ad13431efa9c 100644
--- a/src/tests/cmocka/test_negcache.c
+++ b/src/tests/cmocka/test_negcache.c
@@ -230,11 +230,6 @@ static void test_sss_ncache_uid(void **state)
 
 ret = sss_ncache_set_uid(ts->ctx, permanent, NULL, uid);
 assert_int_equal(ret, EOK);
-
-/* test when ttl is -1 with uid present in database*/
-ttl = -1;
-ret = sss_ncache_check_uid(ts->ctx, ttl, NULL, uid);
-assert_int_equal(ret, EEXIST);
 }
 
 /* @test_sss_ncache_gid : test following functions
@@ -270,11 +265,6 @@ static void test_sss_ncache_gid(void **state)
 
 ret = sss_ncache_check_gid(ts->ctx, ttl, NULL, gid);
 assert_int_equal(ret, EEXIST);
-
-/* test when ttl is -1 with gid present in database*/
-ttl = -1;
-ret = sss_ncache_check_gid(ts->ctx, ttl, NULL, gid);
-assert_int_equal(ret, EEXIST);
 }
 
 
@@ -311,11 +301,6 @@ static void test_sss_ncache_sid(void **state)
 
 ret = sss_ncache_check_sid(ts->ctx, ttl, sid);
 assert_int_equal(ret, EEXIST);
-
-/* test when ttl is -1 with sid present in database*/
-ttl = -1;
-ret = sss_ncache_check_sid(ts->ctx, ttl, sid);
-assert_int_equal(ret, EEXIST);
 }
 
 /* @test_sss_ncache_cert : test following functions
@@ -351,11 +336,6 @@ static void test_sss_ncache_cert(void **state)
 
 ret = sss_ncache_check_cert(ts->ctx, ttl, cert);
 assert_int_equal(ret, EEXIST);
-
-/* test when ttl is -1 with cert present in database*/
-ttl = -1;
-ret = sss_ncache_check_cert(ts->ctx, ttl, cert);
-assert_int_equal(ret, EEXIST);
 }
 
 /* @test_sss_ncache_user : test following functions
@@ -398,11 +378,6 @@ static void test_sss_ncache_user(void **state)
 
 ret = sss_ncache_check_user(ts->ctx, ttl, dom, name);
 assert_int_equal(ret, EEXIST);
-
-/* test when ttl is -1 with domain name present in database */
-ttl = -1;
-ret = sss_ncache_check_user(ts->ctx, ttl, dom, name);
-assert_int_equal(ret, EEXIST);
 }
 
 /* @test_sss_ncache_group : test

[SSSD] Re: [PATCH] NEGCACHE: Test fix

2016-04-20 Thread Petr Cech 

On 04/19/2016 07:24 PM, Petr Cech wrote:

Hello,

there is little simple patch which fix the typo.



Hello,

I wrote better commit message and I added to patch set with

NEGCACHE: Removing of condition for ttl = -1

https://lists.fedorahosted.org/archives/list/sssd-devel@lists.fedorahosted.org/thread/LE6SDCTXJGL3QFD4RFP5YBWLLACD3JYG/

Regards

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org