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