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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

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 ?

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

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

Ian.

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