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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On 11/11/13 18:02, Ian Jackson wrote:
> 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 ?

Yes, sorry for that.

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

Great, if you have other comments on the series I will resend it with
the whitespace removed. Should I take the "looks good to me" as an Ack
if the whitespace is removed?


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