[PATCH] RESPONDER: Refactor DP requests into tevent_req style

[Stephen Gallagher]

This patch addresses a number of code-maintenance issues and one
hard-to-spot bug that has been hiding in the responder code for a long
time.

The code has been changed so that we will now follow the tevent_req
style for requesting data from the Data Provider. We will attach
tevent_req objects to the DP request object and call tevent_req_[done|
error] on them once the processing is complete.

For those requests that don't need to wait for a reply (such as midpoint
cache refresh), the caller need only talloc_free() the returned
tevent_req. The cache update will still continue in the background, but
no notification of completion will be sent to this caller.

The major bug that this addresses was the present of two timeouts being
monitored for the same request: one was set in tevent directly by the
NSS responder and the other was created implicitly by the D-BUS
communication. Under some hard-to-reproduce situations, these could
interact negatively and result in one cancelling the other. It has now
been reduced down to using ONLY the D-BUS timeout.

[Jakub Hrozek]

On Fri, Nov 18, 2011 at 12:17:42PM -0500, Stephen Gallagher wrote:
>  This patch addresses a number of code-maintenance issues and one
>  hard-to-spot bug that has been hiding in the responder code for a long
>  time.
>  
>  The code has been changed so that we will now follow the tevent_req
>  style for requesting data from the Data Provider. We will attach
>  tevent_req objects to the DP request object and call tevent_req_[done|
>  error] on them once the processing is complete.
>  
>  For those requests that don't need to wait for a reply (such as midpoint
>  cache refresh), the caller need only talloc_free() the returned
>  tevent_req. The cache update will still continue in the background, but
>  no notification of completion will be sent to this caller.
>  
>  The major bug that this addresses was the present of two timeouts being
>  monitored for the same request: one was set in tevent directly by the
>  NSS responder and the other was created implicitly by the D-BUS
>  communication. Under some hard-to-reproduce situations, these could
>  interact negatively and result in one cancelling the other. It has now
>  been reduced down to using ONLY the D-BUS timeout. 
(I hope I don't break threading..I lost some mail and ended up importing
sssd-devel contents from public archives)

Nack,

sssd_nss segfaults when setpwent is run:

Program received signal SIGSEGV, Segmentation fault.
0xnai000000000040f7dc in nss_cmd_setpwent_step (step_ctx=0x206a290) at
src/responder/nss/nsssrv_cmd.c:1405
1405                    cb_ctx->cctx = dctx->cmdctx->cctx;
(gdb) print *dctx
$1 = {cmdctx = 0x0, domain = 0x2067620, check_provider = false, res =
0x0, netgr = 0x0}
(gdb) bt
#0  0x000000000040f7dc in nss_cmd_setpwent_step (step_ctx=0x206a290) at
src/responder/nss/nsssrv_cmd.c:1405
#1  0x000000000040f101 in nss_cmd_setpwent_send (mem_ctx=0x206d290, client=0x206ada0) at
src/responder/nss/nsssrv_cmd.c:1316
#2  0x000000000040e7fe in nss_cmd_setpwent (cctx=0x206ada0) at
src/responder/nss/nsssrv_cmd.c:1202
#3  0x0000000000421c5a in sss_cmd_execute (cctx=0x206ada0, sss_cmds=0x66ffa0) at
src/responder/common/responder_cmd.c:98
#4  0x0000000000422b7e in client_recv (cctx=0x206ada0) at
src/responder/common/responder_common.c:181
#5  0x0000000000423139 in client_fd_handler (ev=0x205d300, fde=0x20699d0, flags=1,
ptr=0x206ada0) at src/responder/common/responder_common.c:219
#6  0x0000003aa42066b8 in epoll_event_loop (tvalp=0x7fffb53d6d90, std_ev=0x205d3c0) at
../tevent_standard.c:309
#7  std_event_loop_once (ev=<optimized out>, location=<optimized out>) at
../tevent_standard.c:548
#8  0x0000003aa42039f0 in _tevent_loop_once (ev=0x205d300, location=0x468817
"src/util/server.c:570") at ../tevent.c:494
#9  0x0000003aa4203b7b in tevent_common_loop_wait (ev=0x205d300, location=0x468817
"src/util/server.c:570") at ../tevent.c:595
#10 0x00000000004492e8 in server_loop (main_ctx=0x205e470) at src/util/server.c:570
#11 0x0000000000407c91 in main (argc=1, argv=0x7fffb53d70e8) at
src/responder/nss/nsssrv.c:378

[Stephen Gallagher]

On Mon, 2011-11-28 at 15:01 +0100, Jakub Hrozek wrote:
>  On Fri, Nov 18, 2011 at 12:17:42PM -0500, Stephen Gallagher wrote:
>  > This patch addresses a number of code-maintenance issues and one
>  > hard-to-spot bug that has been hiding in the responder code for a long
>  > time.
>  > 
>  > The code has been changed so that we will now follow the tevent_req
>  > style for requesting data from the Data Provider. We will attach
>  > tevent_req objects to the DP request object and call tevent_req_[done|
>  > error] on them once the processing is complete.
>  > 
>  > For those requests that don't need to wait for a reply (such as midpoint
>  > cache refresh), the caller need only talloc_free() the returned
>  > tevent_req. The cache update will still continue in the background, but
>  > no notification of completion will be sent to this caller.
>  > 
>  > The major bug that this addresses was the present of two timeouts being
>  > monitored for the same request: one was set in tevent directly by the
>  > NSS responder and the other was created implicitly by the D-BUS
>  > communication. Under some hard-to-reproduce situations, these could
>  > interact negatively and result in one cancelling the other. It has now
>  > been reduced down to using ONLY the D-BUS timeout.
>  
>  (I hope I don't break threading..I lost some mail and ended up importing
>  sssd-devel contents from public archives)
>  
>  Nack,
>  
>  sssd_nss segfaults when setpwent is run: 
Thanks, I made a bad assumption in the enumeration code that dctx was
available. I'm now carrying the cli_ctx explicitly.

New patch attached.

[Jakub Hrozek]

On Mon, Nov 28, 2011 at 11:28:18AM -0500, Stephen Gallagher wrote:
>  On Mon, 2011-11-28 at 15:01 +0100, Jakub Hrozek wrote:
>  > On Fri, Nov 18, 2011 at 12:17:42PM -0500, Stephen Gallagher wrote:
>  > > This patch addresses a number of code-maintenance issues and one
>  > > hard-to-spot bug that has been hiding in the responder code for a long
>  > > time.
>  > > 
>  > > The code has been changed so that we will now follow the tevent_req
>  > > style for requesting data from the Data Provider. We will attach
>  > > tevent_req objects to the DP request object and call tevent_req_[done|
>  > > error] on them once the processing is complete.
>  > > 
>  > > For those requests that don't need to wait for a reply (such as midpoint
>  > > cache refresh), the caller need only talloc_free() the returned
>  > > tevent_req. The cache update will still continue in the background, but
>  > > no notification of completion will be sent to this caller.
>  > > 
>  > > The major bug that this addresses was the present of two timeouts being
>  > > monitored for the same request: one was set in tevent directly by the
>  > > NSS responder and the other was created implicitly by the D-BUS
>  > > communication. Under some hard-to-reproduce situations, these could
>  > > interact negatively and result in one cancelling the other. It has now
>  > > been reduced down to using ONLY the D-BUS timeout.
>  > 
>  > (I hope I don't break threading..I lost some mail and ended up importing
>  > sssd-devel contents from public archives)
>  > 
>  > Nack,
>  > 
>  > sssd_nss segfaults when setpwent is run:
>  
>  Thanks, I made a bad assumption in the enumeration code that dctx was
>  available. I'm now carrying the cli_ctx explicitly.
>  
>  New patch attached. 
Ack, just consider one change before pushing the "error:" label in
check_cache() always passes ENOMEM into nss_cmd_send_error. I think it
should use whatever is in ret.

[Stephen Gallagher]

On Tue, 2011-11-29 at 12:32 +0100, Jakub Hrozek wrote:
>  On Mon, Nov 28, 2011 at 11:28:18AM -0500, Stephen Gallagher wrote:
>  > On Mon, 2011-11-28 at 15:01 +0100, Jakub Hrozek wrote:
>  > > On Fri, Nov 18, 2011 at 12:17:42PM -0500, Stephen Gallagher wrote:
>  > > > This patch addresses a number of code-maintenance issues and one
>  > > > hard-to-spot bug that has been hiding in the responder code for a long
>  > > > time.
>  > > > 
>  > > > The code has been changed so that we will now follow the tevent_req
>  > > > style for requesting data from the Data Provider. We will attach
>  > > > tevent_req objects to the DP request object and call tevent_req_[done|
>  > > > error] on them once the processing is complete.
>  > > > 
>  > > > For those requests that don't need to wait for a reply (such as
> midpoint
>  > > > cache refresh), the caller need only talloc_free() the returned
>  > > > tevent_req. The cache update will still continue in the background, but
>  > > > no notification of completion will be sent to this caller.
>  > > > 
>  > > > The major bug that this addresses was the present of two timeouts being
>  > > > monitored for the same request: one was set in tevent directly by the
>  > > > NSS responder and the other was created implicitly by the D-BUS
>  > > > communication. Under some hard-to-reproduce situations, these could
>  > > > interact negatively and result in one cancelling the other. It has now
>  > > > been reduced down to using ONLY the D-BUS timeout.
>  > > 
>  > > (I hope I don't break threading..I lost some mail and ended up importing
>  > > sssd-devel contents from public archives)
>  > > 
>  > > Nack,
>  > > 
>  > > sssd_nss segfaults when setpwent is run:
>  > 
>  > Thanks, I made a bad assumption in the enumeration code that dctx was
>  > available. I'm now carrying the cli_ctx explicitly.
>  > 
>  > New patch attached.
>  
>  Ack, just consider one change before pushing the "error:" label in
>  check_cache() always passes ENOMEM into nss_cmd_send_error. I think it
>  should use whatever is in ret. 
Error code changed and pushed to master.