[SSSD] [PATCH] Add common SIGCHLD handling.

Stephen Gallagher sgallagh at redhat.com
Fri Oct 21 16:44:35 UTC 2011


On Fri, 2011-10-21 at 15:28 +0200, Pavel Zuna wrote:
> Base on the second proposal:
> 
> https://fedorahosted.org/sssd/wiki/DesignDocs/SigChld
> 
> There is some old SIGCHLD handling code in src/providers/child_common.[ch], that 
> should probably go away if this gets accepted. There was also a naming conflict 
> with the sss_child_ctx structure. This structure is only used internally by 
> functions defined src/providers/child_common.c. I renamed the original structure 
> to sss_child_ctx_old for now.


Nack (but a good start)

sss_child_destructor() cannot return the result of hash_delete(). Talloc
destructors may only return 0 on success or -1 on failure. However,
because destructor failure results in cancelling the free(), we should
just log an error here and return 0.

sss_child_register:
Don't initialize sss_child_ctx *tmp to the child_ctx passed in. We want
to leave child_ctx untouched until successful completion. Also, please
don't use the variable name "tmp". It's not descriptive. Call it
"child".

Instead of allocating *child_ctx at the beginning, allocate "child" on
the mem_ctx.

child = talloc_zero(mem_ctx, struct sss_child_ctx);

Then, only at the end of execution, right before returning EOK, do
*child_ctx = child;


sss_child_invoke_cb():
I'm wary of having the callback invocation free the child_ctx. It needs
to be documented well (so that consumers know they must not free it once
the callback has fired or they'll get a double-free).

sss_child_handler():
You need to check whether waitpid() returned an error and handle that
appropriately. waitpid() will return -1 on error and set errno. If the
errno value is EINTR, you should retry (because waitpid() was
interrupted by a signal at the time). Other errors should generate a
DEBUG message.



Please separate any changes you make to the existing sss_child_ctx into
its own patch. I don't like just renaming it to _old, either. Please
find a more appropriate way to replace this old object (preferably by
converting it to use your new mechanism, also in a new patch).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111021/04e3a7af/attachment.sig>


More information about the sssd-devel mailing list