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

[Jakub Hrozek]

On Wed, Jun 29, 2016 at 10:39:17PM +0200, Jakub Hrozek wrote:
> On Wed, Jun 29, 2016 at 06:49:05PM +0200, Jakub Hrozek wrote:
> > On Wed, Jun 29, 2016 at 06:20:25PM +0200, Lukas Slebodnik wrote:
> > > On (24/06/16 10:16), Jakub Hrozek wrote:
> > > >On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
> > > >> On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
> > > >> > On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
> > > >> > > Given that Lukas says "http_parser can provide pkgconfig in 
> > > >> > > future", I
> > > >> > > read his mail as a preference to keep the pkg-check test. And 
> > > >> > > actually I
> > > >> > > agree, it doesn't hurt, let's keep it in.
> > > >> > 
> > > >> > I wanted to push these patches:
> > > >> > https://github.com/jhrozek/sssd/tree/secrets-review
> > > >> > but..I can't find http_parser_strict on Debian, only http-parser and 
> > > >> > the
> > > >> > patches seem to require the _strict version. I really don't know the
> > > >> > difference between the two, can we fallback to the non-strict?
> > > >> 
> > > >> If it not too hard to detect if strict is present I would try to use it
> > > >> and fallback to not strict only of not available.
> > > >
> > > >Thanks, I will prepare a fallback patch. I agree about the preference,
> > > >but was wondering if we should require strict or not.
> > > I did a very brief test of latest patches from github and I can see
> > > few errors/warnings.
> > > 
> > > src/responder/common/responder_common.c: In function 
> > > ‘activate_unix_sockets’:
> > > src/responder/common/responder_common.c:795:13: error: too many arguments 
> > > for format [-Werror=format-extra-args]
> > >  DEBUG(SSSDBG_CRIT_FAILURE,
> > >  ^
> > > src/responder/common/responder_common.c:808:17: error: too many arguments 
> > > for format [-Werror=format-extra-args]
> > >  DEBUG(SSSDBG_CRIT_FAILURE,
> > >  ^
> > > 
> > > 
> > > src/responder/secrets/proxy.c: In function ‘ph_on_status’:
> > > src/responder/secrets/proxy.c:648:29: error: passing argument 2 of 
> > > ‘ph_append_string’ from incompatible pointer type 
> > > [-Werror=incompatible-pointer-types]
> > >  ph_append_string(reply, >reason_phrase, at, length);
> > >  ^
> > > src/responder/secrets/proxy.c:624:13: note: expected ‘char **’ but 
> > > argument is of type ‘const char **’
> > >  static void ph_append_string(TALLOC_CTX *memctx, char **dest,
> > >  ^~~~
> > > 
> > >   CCLD test_cert_utils
> > > /usr/bin/ld: src/util/test_cert_utils-util.o: undefined reference to 
> > > symbol 'hash_enter@@DHASH_0.4.3'
> > > /usr/lib64/libdhash.so.1: error adding symbols: DSO missing from command 
> > > line
> > > Previous linking error is fixed by following diff
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 622b0d2..7416141 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -2895,7 +2895,6 @@ test_krb5_wait_queue_LDADD = \
> > >  
> > >  test_cert_utils_SOURCES = \
> > >  src/tests/cmocka/test_cert_utils.c \
> > > -src/util/util.c \
> > >  $(NULL)
> > >  test_cert_utils_CFLAGS = \
> > >  $(AM_CFLAGS) \
> > > 
> > > 
> > > 
> > >   CCLD responder_common-tests
> > > /usr/bin/ld: 
> > > ../../../src/responder/common/responder_common_tests-responder_common.o: 
> > > undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209'
> > > /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from 
> > > command line
> > > collect2: error: ld returned 1 exit status
> > > 
> > >   CCLD negcache-tests
> > > /usr/bin/ld: 
> > > ../../../src/responder/common/negcache_tests-responder_common.o: 
> > > undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209'
> > > /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from 
> > > command line
> > > collect2: error: ld returned 1 exit status
> > > 
> > > and previous two linking error with following diff
> > > diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am
> > > index 8005d99..d8a49f1 100644
> > > --- a/src/tests/cwrap/Makefile.am
> > > +++ b/src/tests/cwrap/Makefile.am
> > > @@ -145,6 +145,7 @@ responder_common_tests_LDADD = \
> > >  $(CMOCKA_LIBS) \
> > >  $(SSSD_LIBS) \
> > >  $(SELINUX_LIBS) \
> > > +$(SYSTEMD_DAEMON_LIBS) \
> > >  $(abs_top_builddir)/libsss_util.la \
> > >  $(abs_top_builddir)/libsss_debug.la \
> > >  $(abs_top_builddir)/libsss_test_common.la \
> > > @@ -162,6 +163,7 @@ negcache_tests_LDADD = \
> > >  $(CMOCKA_LIBS) \
> > >  $(SSSD_LIBS) \
> > >  $(SELINUX_LIBS) \
> > > +$(SYSTEMD_DAEMON_LIBS) \
> > >  $(abs_top_builddir)/libsss_util.la \
> > >  $(abs_top_builddir)/libsss_debug.la \
> > >  $(abs_top_builddir)/libsss_test_common.la \
> > 
> > OK, squashed and submitted to CI. Thanks, if the CI passes this time,
> > I'll push the patches and *finally* release the beta.
> 
> 

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

[Jakub Hrozek]

On Wed, Jun 29, 2016 at 06:20:25PM +0200, Lukas Slebodnik wrote:
> On (24/06/16 10:16), Jakub Hrozek wrote:
> >On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
> >> On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
> >> > On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
> >> > > Given that Lukas says "http_parser can provide pkgconfig in future", I
> >> > > read his mail as a preference to keep the pkg-check test. And actually 
> >> > > I
> >> > > agree, it doesn't hurt, let's keep it in.
> >> > 
> >> > I wanted to push these patches:
> >> > https://github.com/jhrozek/sssd/tree/secrets-review
> >> > but..I can't find http_parser_strict on Debian, only http-parser and the
> >> > patches seem to require the _strict version. I really don't know the
> >> > difference between the two, can we fallback to the non-strict?
> >> 
> >> If it not too hard to detect if strict is present I would try to use it
> >> and fallback to not strict only of not available.
> >
> >Thanks, I will prepare a fallback patch. I agree about the preference,
> >but was wondering if we should require strict or not.
> I did a very brief test of latest patches from github and I can see
> few errors/warnings.
> 
> src/responder/common/responder_common.c: In function ‘activate_unix_sockets’:
> src/responder/common/responder_common.c:795:13: error: too many arguments for 
> format [-Werror=format-extra-args]
>  DEBUG(SSSDBG_CRIT_FAILURE,
>  ^
> src/responder/common/responder_common.c:808:17: error: too many arguments for 
> format [-Werror=format-extra-args]
>  DEBUG(SSSDBG_CRIT_FAILURE,
>  ^
> 
> 
> src/responder/secrets/proxy.c: In function ‘ph_on_status’:
> src/responder/secrets/proxy.c:648:29: error: passing argument 2 of 
> ‘ph_append_string’ from incompatible pointer type 
> [-Werror=incompatible-pointer-types]
>  ph_append_string(reply, >reason_phrase, at, length);
>  ^
> src/responder/secrets/proxy.c:624:13: note: expected ‘char **’ but argument 
> is of type ‘const char **’
>  static void ph_append_string(TALLOC_CTX *memctx, char **dest,
>  ^~~~
> 
>   CCLD test_cert_utils
> /usr/bin/ld: src/util/test_cert_utils-util.o: undefined reference to symbol 
> 'hash_enter@@DHASH_0.4.3'
> /usr/lib64/libdhash.so.1: error adding symbols: DSO missing from command line
> Previous linking error is fixed by following diff
> diff --git a/Makefile.am b/Makefile.am
> index 622b0d2..7416141 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2895,7 +2895,6 @@ test_krb5_wait_queue_LDADD = \
>  
>  test_cert_utils_SOURCES = \
>  src/tests/cmocka/test_cert_utils.c \
> -src/util/util.c \
>  $(NULL)
>  test_cert_utils_CFLAGS = \
>  $(AM_CFLAGS) \
> 
> 
> 
>   CCLD responder_common-tests
> /usr/bin/ld: 
> ../../../src/responder/common/responder_common_tests-responder_common.o: 
> undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209'
> /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command 
> line
> collect2: error: ld returned 1 exit status
> 
>   CCLD negcache-tests
> /usr/bin/ld: ../../../src/responder/common/negcache_tests-responder_common.o: 
> undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209'
> /usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command 
> line
> collect2: error: ld returned 1 exit status
> 
> and previous two linking error with following diff
> diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am
> index 8005d99..d8a49f1 100644
> --- a/src/tests/cwrap/Makefile.am
> +++ b/src/tests/cwrap/Makefile.am
> @@ -145,6 +145,7 @@ responder_common_tests_LDADD = \
>  $(CMOCKA_LIBS) \
>  $(SSSD_LIBS) \
>  $(SELINUX_LIBS) \
> +$(SYSTEMD_DAEMON_LIBS) \
>  $(abs_top_builddir)/libsss_util.la \
>  $(abs_top_builddir)/libsss_debug.la \
>  $(abs_top_builddir)/libsss_test_common.la \
> @@ -162,6 +163,7 @@ negcache_tests_LDADD = \
>  $(CMOCKA_LIBS) \
>  $(SSSD_LIBS) \
>  $(SELINUX_LIBS) \
> +$(SYSTEMD_DAEMON_LIBS) \
>  $(abs_top_builddir)/libsss_util.la \
>  $(abs_top_builddir)/libsss_debug.la \
>  $(abs_top_builddir)/libsss_test_common.la \

OK, squashed and submitted to CI. Thanks, if the CI passes this time,
I'll push the patches and *finally* release the beta.
___
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

[Lukas Slebodnik]

On (24/06/16 10:16), Jakub Hrozek wrote:
>On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
>> On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
>> > On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
>> > > Given that Lukas says "http_parser can provide pkgconfig in future", I
>> > > read his mail as a preference to keep the pkg-check test. And actually I
>> > > agree, it doesn't hurt, let's keep it in.
>> > 
>> > I wanted to push these patches:
>> > https://github.com/jhrozek/sssd/tree/secrets-review
>> > but..I can't find http_parser_strict on Debian, only http-parser and the
>> > patches seem to require the _strict version. I really don't know the
>> > difference between the two, can we fallback to the non-strict?
>> 
>> If it not too hard to detect if strict is present I would try to use it
>> and fallback to not strict only of not available.
>
>Thanks, I will prepare a fallback patch. I agree about the preference,
>but was wondering if we should require strict or not.
I did a very brief test of latest patches from github and I can see
few errors/warnings.

src/responder/common/responder_common.c: In function ‘activate_unix_sockets’:
src/responder/common/responder_common.c:795:13: error: too many arguments for 
format [-Werror=format-extra-args]
 DEBUG(SSSDBG_CRIT_FAILURE,
 ^
src/responder/common/responder_common.c:808:17: error: too many arguments for 
format [-Werror=format-extra-args]
 DEBUG(SSSDBG_CRIT_FAILURE,
 ^


src/responder/secrets/proxy.c: In function ‘ph_on_status’:
src/responder/secrets/proxy.c:648:29: error: passing argument 2 of 
‘ph_append_string’ from incompatible pointer type 
[-Werror=incompatible-pointer-types]
 ph_append_string(reply, >reason_phrase, at, length);
 ^
src/responder/secrets/proxy.c:624:13: note: expected ‘char **’ but argument is 
of type ‘const char **’
 static void ph_append_string(TALLOC_CTX *memctx, char **dest,
 ^~~~

  CCLD test_cert_utils
/usr/bin/ld: src/util/test_cert_utils-util.o: undefined reference to symbol 
'hash_enter@@DHASH_0.4.3'
/usr/lib64/libdhash.so.1: error adding symbols: DSO missing from command line
Previous linking error is fixed by following diff
diff --git a/Makefile.am b/Makefile.am
index 622b0d2..7416141 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2895,7 +2895,6 @@ test_krb5_wait_queue_LDADD = \
 
 test_cert_utils_SOURCES = \
 src/tests/cmocka/test_cert_utils.c \
-src/util/util.c \
 $(NULL)
 test_cert_utils_CFLAGS = \
 $(AM_CFLAGS) \



  CCLD responder_common-tests
/usr/bin/ld: 
../../../src/responder/common/responder_common_tests-responder_common.o: 
undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209'
/usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

  CCLD negcache-tests
/usr/bin/ld: ../../../src/responder/common/negcache_tests-responder_common.o: 
undefined reference to symbol 'sd_listen_fds@@LIBSYSTEMD_209'
/usr/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

and previous two linking error with following diff
diff --git a/src/tests/cwrap/Makefile.am b/src/tests/cwrap/Makefile.am
index 8005d99..d8a49f1 100644
--- a/src/tests/cwrap/Makefile.am
+++ b/src/tests/cwrap/Makefile.am
@@ -145,6 +145,7 @@ responder_common_tests_LDADD = \
 $(CMOCKA_LIBS) \
 $(SSSD_LIBS) \
 $(SELINUX_LIBS) \
+$(SYSTEMD_DAEMON_LIBS) \
 $(abs_top_builddir)/libsss_util.la \
 $(abs_top_builddir)/libsss_debug.la \
 $(abs_top_builddir)/libsss_test_common.la \
@@ -162,6 +163,7 @@ negcache_tests_LDADD = \
 $(CMOCKA_LIBS) \
 $(SSSD_LIBS) \
 $(SELINUX_LIBS) \
+$(SYSTEMD_DAEMON_LIBS) \
 $(abs_top_builddir)/libsss_util.la \
 $(abs_top_builddir)/libsss_debug.la \
 $(abs_top_builddir)/libsss_test_common.la \

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

[Jakub Hrozek]

On Thu, Jun 23, 2016 at 04:23:12PM -0400, Simo Sorce wrote:
> On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
> > On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
> > > Given that Lukas says "http_parser can provide pkgconfig in future", I
> > > read his mail as a preference to keep the pkg-check test. And actually I
> > > agree, it doesn't hurt, let's keep it in.
> > 
> > I wanted to push these patches:
> > https://github.com/jhrozek/sssd/tree/secrets-review
> > but..I can't find http_parser_strict on Debian, only http-parser and the
> > patches seem to require the _strict version. I really don't know the
> > difference between the two, can we fallback to the non-strict?
> 
> If it not too hard to detect if strict is present I would try to use it
> and fallback to not strict only of not available.

Thanks, I will prepare a fallback patch. I agree about the preference,
but was wondering if we should require strict or not.
___
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

[Simo Sorce]

On Thu, 2016-06-23 at 21:36 +0200, Jakub Hrozek wrote:
> On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
> > Given that Lukas says "http_parser can provide pkgconfig in future", I
> > read his mail as a preference to keep the pkg-check test. And actually I
> > agree, it doesn't hurt, let's keep it in.
> 
> I wanted to push these patches:
> https://github.com/jhrozek/sssd/tree/secrets-review
> but..I can't find http_parser_strict on Debian, only http-parser and the
> patches seem to require the _strict version. I really don't know the
> difference between the two, can we fallback to the non-strict?

If it not too hard to detect if strict is present I would try to use it
and fallback to not strict only of not available.

Strict *seems* a safer option.

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

[Jakub Hrozek]

On Thu, Apr 21, 2016 at 09:21:17AM +0200, Jakub Hrozek wrote:
> Given that Lukas says "http_parser can provide pkgconfig in future", I
> read his mail as a preference to keep the pkg-check test. And actually I
> agree, it doesn't hurt, let's keep it in.

I wanted to push these patches:
https://github.com/jhrozek/sssd/tree/secrets-review
but..I can't find http_parser_strict on Debian, only http-parser and the
patches seem to require the _strict version. I really don't know the
difference between the two, can we fallback to the non-strict?
___
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

[Simo Sorce]

On Wed, 2016-04-20 at 09:59 +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 wasn?t intentional in the sense that i really wanted at all cost to
have this behavior, but I see nothing wrong on sssd watchdogging itself.

> 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 :)

New debugging trick, as soon as you attach or execute a process run:
p teardown_watchdog()

I have a gdb cmd file where I execute this as a matter of fact when
attached to a sssd process.

> Otherwise ack.

Thanks.

> > 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.

Please add patch on top.

> 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...

signal(7) sasys fork()/execve() are async-signal-safe functions, so it
could be run from the handler directly, the other option is to
kill(SIGXXX) the monitor, and then wait a few seconds before aborting,
and hope the monitor has time to run pstack on the child for us.

Either way I'd consider writing a new patch for this later, and would
consider internally dumping a stack trace to a debug file instead of
executing an external process.

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

Not only fragile but also caused churn and wakeups in the system, as
every watchdog beat forced at least a context switch and 2 processes to
run.

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

We can look at it, I didn't want to get too far down the systemd road
with this patchset as the goal was another.

> To be continued..

Ty,
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

[Jakub Hrozek]

On Wed, Apr 20, 2016 at 02:58:36PM -0400, Simo Sorce wrote:
> 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 

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

[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} A new Secrets service

[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 SET} A new Secrets service

[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

[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

[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

[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

[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

[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

[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

[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

[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 SET} A new Secrets service

[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

[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

[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

[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 SET} A new Secrets service

[Simo Sorce]

On Tue, 2016-04-05 at 14:54 -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.

BUMP ?

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

[Simo Sorce]

On Fri, 2016-04-01 at 13:05 +0200, Lukas Slebodnik wrote:
> On (30/03/16 12:31), Simo Sorce wrote:
> >This patchset implements a new responder like service in SSSD called
> >secrets. It uses the Custodia project API to offer a service where
> >applications/users can store secrets in a way that makes requests
> >remotizable and routable with a high degree of configurability (esp, in
> >conjunction with a Custodia proxy).
> >
> >Included are also accessory patches to change the monitor and other
> >aspects of service startup and monitoring  necessary to have this new
> >kind of service which is more independent than the pam/nss based
> >services.
> >
> >There is no testsuite for the service yet.
> >
> >The work is also not complete in that the monitor does not start the
> >service yet, I have an experimental unit file I am working on but it is
> >not fully functional and not included yet..
> >
> >I do not expect all patches to be accepted right away, but they all work
> >individually (manually tested), but I think it is a good time to start
> >review and bring in what works, as we are going to spread some of the
> >remaining work across multiple people.
> >
> >HTH,
> >Simo.
> >
> 
> I do not plan to do a full review. Jakub voluntered IIRC :-)
> But here are few comments.
> 
> >From cd731590f1830ab9686949af1fa66d2b7463eafc 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
> >
> >The watchdog uses a kernel timer to issue a signal to the process.
> >It checks if the ticker is not being reset by the main event loop, which
> >would indicate that the process got stuck.
> >At the same time it sets a tevent timer to clear the watchdog ticker, so
> >that the watchdog handler is kept happy.
> >
> >If the watchdog detects that the timer event failed to reset the watchdog for
> >three times in a row then the process is killed.
> >Normally the monitor will detect the child terminated and will rescheduled 
> >it.
> >
> >Related:
> >https://fedorahosted.org/sssd/ticket/2921
> >---
> >diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c
> >new file mode 100644
> >index 
> >..64faaaf03213319d3127fde3946124ee26c7794a
> >--- /dev/null
> >+++ b/src/util/util_watchdog.c
> //snip
> 
> >+int setup_watchdog(struct tevent_context *ev, int interval)
> >+{
> >+struct sigevent sev;
> >+struct itimerspec its;
> >+int signum = SIGRTMIN;
> >+int ret;
> >+
> >+CatchSignal(signum, watchdog_handler);
> >+
> >+sev.sigev_notify = SIGEV_SIGNAL;
> >+sev.sigev_signo = signum;
> >+sev.sigev_value.sival_ptr = _ctx.timerid;
> >+errno = 0;
> >+ret = timer_create(CLOCK_MONOTONIC, , _ctx.timerid);
> I got a valgrind error here on rhel6
> ==2376== Syscall param timer_create(evp) points to uninitialised byte(s)
> ==2376==at 0x5B0B08D: timer_create@@GLIBC_2.3.3 (in 
> /lib64/librt-2.12.so)
> ==2376==by 0x54942CD: setup_watchdog (util_watchdog.c:88)
> ==2376==by 0x40414F: server_setup (server.c:651)
> ==2376==by 0x403059: test_run_as_root_fg (test_server.c:104)
> ==2376==by 0x5243A94: ??? (in /usr/lib64/libcmocka.so.0.3.1)
> ==2376==by 0x5243DA9: _cmocka_run_group_tests (in 
> /usr/lib64/libcmocka.so.0.3.1)
> ==2376==by 0x402CA1: main (test_server.c:204)
> ==2376==  Address 0x7fefff920 is on thread 1's stack
> 
> Following diff fixed it.

Thanks for finding this!

> diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c
> index 64faaaf..d6f71ad 100644
> --- a/src/util/util_watchdog.c
> +++ b/src/util/util_watchdog.c
> @@ -74,7 +74,7 @@ static void watchdog_event_handler(struct tevent_context 
> *ev,
>  
>  int setup_watchdog(struct tevent_context *ev, int interval)
>  {
> -struct sigevent sev;
> +struct sigevent sev = { 0, };
>  struct itimerspec its;
>  int signum = SIGRTMIN;
>  int ret;

> >diff --git a/configure.ac b/configure.ac
> >index 
> >783372dd6b5d86793c218ac513a93b65e97d4c06..f7430ca71f07b1085f49a7635070f71894f1a4a9
> > 100644
> >--- a/configure.ac
> >+++ b/configure.ac
> >@@ -185,6 +185,8 @@ m4_include([src/external/libnfsidmap.m4])
> > m4_include([src/external/cwrap.m4])
> > m4_include([src/external/libresolv.m4])
> > m4_include([src/external/intgcheck.m4])
> >+m4_include([src/external/libhttp_parser.m4])
> >+m4_include([src/external/libjansson.m4])
> This patch broke build in mock and CI.
> because you added new dependencies which are not as BuildRequires in spec.
> and moreover these dependencies are strictly detected at configure time
> even though secrect service can be build optionally.
> 
> One solution would be to remove conitional build of secret service
> or another to fix detection of build dependencies at configure time.

I'll fix the detection.

> e.g.
> diff --git a/configure.ac b/configure.ac
> index 2a8329c..9ddd9da 100644
> --- 

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

[Lukas Slebodnik]

On (30/03/16 12:31), Simo Sorce wrote:
>This patchset implements a new responder like service in SSSD called
>secrets. It uses the Custodia project API to offer a service where
>applications/users can store secrets in a way that makes requests
>remotizable and routable with a high degree of configurability (esp, in
>conjunction with a Custodia proxy).
>
>Included are also accessory patches to change the monitor and other
>aspects of service startup and monitoring  necessary to have this new
>kind of service which is more independent than the pam/nss based
>services.
>
>There is no testsuite for the service yet.
>
>The work is also not complete in that the monitor does not start the
>service yet, I have an experimental unit file I am working on but it is
>not fully functional and not included yet..
>
>I do not expect all patches to be accepted right away, but they all work
>individually (manually tested), but I think it is a good time to start
>review and bring in what works, as we are going to spread some of the
>remaining work across multiple people.
>
>HTH,
>Simo.
>

I do not plan to do a full review. Jakub voluntered IIRC :-)
But here are few comments.

>From cd731590f1830ab9686949af1fa66d2b7463eafc 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
>
>The watchdog uses a kernel timer to issue a signal to the process.
>It checks if the ticker is not being reset by the main event loop, which
>would indicate that the process got stuck.
>At the same time it sets a tevent timer to clear the watchdog ticker, so
>that the watchdog handler is kept happy.
>
>If the watchdog detects that the timer event failed to reset the watchdog for
>three times in a row then the process is killed.
>Normally the monitor will detect the child terminated and will rescheduled it.
>
>Related:
>https://fedorahosted.org/sssd/ticket/2921
>---
>diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c
>new file mode 100644
>index 
>..64faaaf03213319d3127fde3946124ee26c7794a
>--- /dev/null
>+++ b/src/util/util_watchdog.c
//snip

>+int setup_watchdog(struct tevent_context *ev, int interval)
>+{
>+struct sigevent sev;
>+struct itimerspec its;
>+int signum = SIGRTMIN;
>+int ret;
>+
>+CatchSignal(signum, watchdog_handler);
>+
>+sev.sigev_notify = SIGEV_SIGNAL;
>+sev.sigev_signo = signum;
>+sev.sigev_value.sival_ptr = _ctx.timerid;
>+errno = 0;
>+ret = timer_create(CLOCK_MONOTONIC, , _ctx.timerid);
I got a valgrind error here on rhel6
==2376== Syscall param timer_create(evp) points to uninitialised byte(s)
==2376==at 0x5B0B08D: timer_create@@GLIBC_2.3.3 (in 
/lib64/librt-2.12.so)
==2376==by 0x54942CD: setup_watchdog (util_watchdog.c:88)
==2376==by 0x40414F: server_setup (server.c:651)
==2376==by 0x403059: test_run_as_root_fg (test_server.c:104)
==2376==by 0x5243A94: ??? (in /usr/lib64/libcmocka.so.0.3.1)
==2376==by 0x5243DA9: _cmocka_run_group_tests (in 
/usr/lib64/libcmocka.so.0.3.1)
==2376==by 0x402CA1: main (test_server.c:204)
==2376==  Address 0x7fefff920 is on thread 1's stack

Following diff fixed it.

diff --git a/src/util/util_watchdog.c b/src/util/util_watchdog.c
index 64faaaf..d6f71ad 100644
--- a/src/util/util_watchdog.c
+++ b/src/util/util_watchdog.c
@@ -74,7 +74,7 @@ static void watchdog_event_handler(struct tevent_context *ev,
 
 int setup_watchdog(struct tevent_context *ev, int interval)
 {
-struct sigevent sev;
+struct sigevent sev = { 0, };
 struct itimerspec its;
 int signum = SIGRTMIN;
 int ret;


>+if (ret == -1) {
>+ret = errno;
>+DEBUG(SSSDBG_FATAL_FAILURE,
>+  "Failed to create watchdog timer (%d) [%s]\n",
>+  ret, strerror(ret));
>+return ret;
>+}
>+
>+if (interval == 0) {
>+interval = WATCHDOG_DEF_INTERVAL;
>+}
>+watchdog_ctx.interval.tv_sec = interval;
>+watchdog_ctx.interval.tv_usec = 0;
>+
>+/* Start the timer */
>+/* we give 1 second head start to the watchdog event */
>+its.it_value.tv_sec = interval + 1;
>+its.it_value.tv_nsec = 0;
>+its.it_interval.tv_sec = interval;
>+its.it_interval.tv_nsec = 0;
>+errno = 0;
>+ret = timer_settime(watchdog_ctx.timerid, 0, , NULL);
>+if (ret == -1) {
>+ret = errno;
>+DEBUG(SSSDBG_FATAL_FAILURE,
>+  "Failed to create watchdog timer (%d) [%s]\n",
>+  ret, strerror(ret));
>+return ret;
>+}
>+
>+/* Add the watchdog event and make it fire as fast as the timer */
>+watchdog_event_handler(ev, NULL, tevent_timeval_zero(), NULL);
>+
>+return EOK;
>+}
>+
>+void teardown_watchdog(void)
>+{
>+int ret;
>+
>+/* Disarm the timer */
>+errno = 0;
>+ret = timer_delete(watchdog_ctx.timerid);
>+if (ret == -1) {
>+ret =