Re: [Xen-devel] [PATCH v2 04/10] libxl: synchronize device removal when using driver domains

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v2 04/10] libxl: synchronize device removal 
when using driver domains"):
> Synchronize the clean up of the backend from the toolstack domain when
> the driver domain has actually finished closing the backend for the
> device.
> 
> This is accomplished by waiting for the driver domain to  remove the
> directory containing the backend keys, then the toolstack domain will
> finish the cleanup by removing the empty folders on the backend path.
...
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index d726f0b..8987434 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -443,6 +443,11 @@ void libxl__prepare_ao_device(libxl__ao *ao, 
> libxl__ao_device *aodev)
>      aodev->num_exec = 0;
>      /* Initialize timer for QEMU Bodge and hotplug execution */
>      libxl__ev_time_init(&aodev->timeout);
> +    /*
> +     * Initialize xs_watch, because it's not used on all possible
> +     * execution paths, but it's unconditionally destroyed when finished.
> +     */
> +    libxl__ev_xswatch_init(&aodev->xs_watch);

FWIW I think this is entirely unremarkable and does not deserve a
comment.  Every field in the newly created aodev should be
initialised.  The fact that it wasn't was a latent bug.  So I would
mention that in the commit message instead.

But I don't feel strongly about this so keep it as it is if you like.
And there are several other similar comments in the same function.

>  static void device_hotplug_clean(libxl__gc *gc, libxl__ao_device *aodev);
> @@ -781,12 +794,14 @@ void libxl__initiate_device_remove(libxl__egc *egc,
>          LOG(ERROR, "unable to get info for domain %d", domid);
>          goto out;
>      }
> +
>      if (QEMU_BACKEND(aodev->dev) &&
>          (info.paused || info.dying || info.shutdown)) {
>          /*
>           * TODO: 4.2 Bodge due to QEMU, see comment on top of
>           * libxl__initiate_device_remove in libxl_internal.h
>           */
> +
>          rc = libxl__ev_time_register_rel(gc, &aodev->timeout,
>                                           device_qemu_timeout,
>                                           LIBXL_QEMU_BODGE_TIMEOUT * 1000);

Unintentional whitespace changes ?

Aside from that, this patch all looks good to me.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel