Re: [PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest

[George Dunlap <george.dunlap@eu.citrix.com>]

On 18/04/13 18:51, Ian Jackson wrote:
> George Dunlap writes ("[PATCH v5 1/2] libxl: Introduce functions to add and remove USB devices to an HVM guest"):
>> +    /* Helpfully, libxl__xs_rm_checked() returns 0 on success... */
>> +    rc = libxl__xs_rm_checked(gc, t, libxl_usb_path);
>> +    if (rc) goto out;
>> +    /* ...but libxl__xs_mkdir() returns 1 on success, 0 on failure. */
>> +    rc = libxl__xs_mkdir(gc, t, libxl_usb_path, noperm, ARRAY_SIZE(noperm));
>> +    if (!rc) goto out;
> Is this really right ?  If mkdir fails, you cause libxl__domain_make
> to immediately return success.  The out label expects the error code
> to be in rc.  I think it's a stylistic error to assign the
> (deprecated) boolean return value from libxl__xs_mkdir to rc; rc would
> normally contain a libxl error code.
>
>> +    id = GCSPRINTF(HOST_USB_QDEV_ID,
>> +                   (uint16_t) dev->u.hostdev.hostbus,
>> +                   (uint16_t) dev->u.hostdev.hostaddr);
> I think the style in libxl is to cuddle casts.
>
>> +    switch(usbdev->type) {
> Missing space.  (etc.)  And some overly long lines.
>
> Most of the rest of this looks plausible although I wish you wouldn't
> write things like this:
>
>> +    switch(usbdev->type) {
>> +    case LIBXL_DEVICE_USB_TYPE_HOSTDEV:
>> +        if ((rc = read_hostdev_xenstore_entry(gc, path, usbdev)) < 0)
>> +            goto out;
> I guess old habits are hard to break :-).
>
> I haven't checked the semantics and timing of the xenstore/qmp
> protocol in detail.  But let me ask a question:
>
> Suppose attempted attach or remove of a usb device fails halfway
> through (eg, the user presses ^C and xl just stops since it doesn't
> handle SIGINT).  Is the situation now (a) reported in the usb device
> listing (b) capable of being cleaned up with a remove operation (c)
> tidied up on domain destroy ?  (Is this true at all times - ie does it
> never go through a state where things won't be cleaned up?)

Re domain destroy: AFAIK everything is completely cleaned up on domain 
destroy.  All lf the xenstore directories are removed, and qemu will 
release the device when it shuts down.  This actually has been tested 
when I messed up something in storing the xenstore stuff; I didn't have 
to reboot the host, just shutdown the guest, re-build the library, start 
the guest again and re-test.

  -George