[SSSD] [PATCH] Add common SIGCHLD handling.

Pavel Zuna pzuna at redhat.com
Tue Oct 25 11:08:45 UTC 2011


On 10/21/2011 06:44 PM, Stephen Gallagher wrote:
> 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.
>

Ok, I'm done with all of this. It was pretty straight forward, but...

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

... this, I'm trying to find a way to make the old code use the new mechanism. I 
think it would be better to remove the old one completely and replace it with 
the new one everywhere where it's used.

Pavel



More information about the sssd-devel mailing list