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).
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
