On Dec 14, 2011, at 9:09 AM, Jakub Hrozek <jhro...@redhat.com> wrote:
> On Tue, Dec 13, 2011 at 11:14:31AM -0500, Stephen Gallagher wrote: >> On Tue, 2011-11-22 at 08:58 -0500, Stephen Gallagher wrote: >>> On Wed, 2011-11-09 at 15:31 +0100, Pavel Zuna wrote: >>>> Updated patch attached. >>> >>> >>> >>> Nack. >>> >>> Please make "struct sss_sigchild_ctx" an opaque structure and provide an >>> init routine to create the hash and set up the signal handler. This way >>> we only have to call >>> sss_sigchild_init(ctx, &ctx->sigchld_ctx); >>> in be_process_init() (or any other executable that needs child control). >>> >>> In sss_child_handler(), don't consider it fatal if the PID isn't found >>> in the hash. It could mean that something we're linked against did a >>> fork and was trying to maintain itself. This is bad, but not fatal. >>> Lower the debug level a bit (I think SSSDBG_OP_FAILURE is fine) and >>> allow the waitpid() loop to continue, rather than exiting immediately. >>> >>> sss_child_ctx should be moved to child_common.c. Its internals should >>> not be visible elsewhere. >>> >>> Please reorder sss_child_invoke_cb() to remove the entry from the hash >>> before invoking the callback (you cannot assume that the callback won't >>> do something funny to the hash structure or child_ctx, though it should >>> be safe). Also, please raise the debug level for this failure to >>> SSSDBG_OP_FAILURE. >> >> I've taken over the development of this patch. I'm sending my changes to >> address the above issues except for a slight modification to my comments >> about the sss_child_handler loop - it was already ignoring unknown >> errors, but it was exiting on unexpected failures. I added a comment and >> will allow it to try to continue looping through in the hopes that it >> will minimize the potential zombies. >> >> This patch does not currently consume the functions that it creates. I >> am working on converting the existing child handlers to use this code, >> but I want to get this patch looked at and acked in the meantime. > > Are you going to fix the two comments you had originally in the later > patch? Those were don't consider it fatal if the PID isn't found > in the hash in sss_child_handler() and reordering sss_child_invoke_cb() > > If so, then ack The first part of that I already described above as being an incorrect reading of the source. I missed the second part and I'll fix that shortly. > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
