Re: [Xen-devel] [RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill

From: Anthony PERARD <anthony.perard@citrix.com>
To: Ian Jackson <ian.jackson@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	Wei Liu <wl@xen.org>
Subject: Re: [Xen-devel] [RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill
Date: Mon, 28 Oct 2019 17:01:07 +0000	[thread overview]
Message-ID: <20191028170107.GC1162@perard.uk.xensource.com> (raw)
In-Reply-To: <23990.53361.316758.473175@mariner.uk.xensource.com>

On Mon, Oct 28, 2019 at 11:26:41AM +0000, Ian Jackson wrote:
> Hi.  Thanks.  The code here looks by and large good to me but I think
> the docs and maybe the log messages need improvement.
> 
> Anthony PERARD writes ("[RFC XEN PATCH for-4.13 1/4] libxl: Introduce libxl__ev_child_kill"):
> > Allow to kill a child and deregister the callback associated with it.
> 
> Did you read the doc comment above libxl__ev_child_fork, in
> libxl_internal.h near line 1160 ?  The user of libxl__ev_child is
> already permitted to kill the child.
> 
> In this patch are adding a layer to make this more convenient, and in
> particular to let a libxl__ev_child user transfer responsibility for
> reaping the child from its own application logic into the ao system.
> 
> Some more API documentation to make this much more explicit would be
> good - ie the main doc comment the facility needs to discuss it:
>  | * It is not possible to "deregister" the child death event source
> ^ this is no longer true after your patch; indeed that's the point.
> 
> So perhaps
> 
> > +void libxl__ev_child_kill(libxl__ao *ao, libxl__ev_child *ch)
> 
> should be called
>    libxl__ev_child_reattach_to_ao
> or
>    libxl__ev_child_kill_deregister
> or something, and maybe it should take a signal number ?

I'll rework the documentation to explain that the AO won't complete
until the child has been reaped. Adding the signal number to the
parameter and renaming the function _kill_derigister sound good.

> > +static void deregistered_child_callback(libxl__egc *egc,
> > +                                        libxl__ev_child *ch,
> > +                                        pid_t pid,
> > +                                        int status)
> > +{
> > +    ev_child_killed *ck = CONTAINER_OF(ch, *ck, ch);
> > +    EGC_GC;
> > +
> > +    if (status) {
> > +        libxl_report_child_exitstatus(CTX, XTL_ERROR,
> > +                                      "killed fork (dying as expected)",
> > +                                      pid, status);
> > +    } else {
> > +        LOG(DEBUG, "killed child exit cleanly, unexpected");
> 
> I don't think this is entirely unexpected.  Maybe the child was just
> exiting at the point where libxl__ev_child_kill was called.
> 
> And, please check log the actual whole exit status.  "status" is a
> wait status.  We want to know what signal it died from, whether it
> core dumped, the exit status, etc.  Probably, you should call
> libxl_report_child_exitstatus.

It does ;-). But I guess I could call libxl_report_child_exitstatus()
unconditionally, so even if status=0.

> > @@ -1891,7 +1891,8 @@ static bool ao_work_outstanding(libxl__ao *ao)
> >       * decrement progress_reports_outstanding, and call
> >       * libxl__ao_complete_check_progress_reports.
> >       */
> > -    return !ao->complete || ao->progress_reports_outstanding;
> > +    return !ao->complete || ao->progress_reports_outstanding
> > +        || ao->outstanding_killed_child;
> >  }
> 
> I wonder if this should gain a new debug message.  If the child gets
> lost or stuck for some reason, it will otherwise require searching the
> past log to find out why the ao doesn't return.

Do you mean adding a debug message in libxl__ev_child_kill_deregister()?
It's probably a good idea.

I'll add:
    LOG(DEBUG, "ao %p: Will wait process [%ld] death", ao, pid);

Or should we also add a debug log in libxl__ao_complete() ?

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel