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.

Yes, you're right on both points.  Good catch.

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

Ack.  I've never thought of variables as cuddly before...

>
>> +    switch(usbdev->type) {
> Missing space.  (etc.)  And some overly long lines.

Ack.

>
> 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 :-).

Well I prefer that way -- having the 'if' on a separate line actually 
annoys me (mildly).  "Do this and handle the error if any" seems like 
one operation in my mind, not two.  But I don't mind changing it.

> 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?)

As far as I could determine, there is absolutely no way to query qemu to 
find out what devices are actually attached at the moment; that's the 
whole reason for storing stuff in xenstore in the first place.  So yes 
-- if you kill the process after the qmp command has been issued but 
before writing something in xenstore, then usb-list will not show the 
device, usb-remove will return an error if you attempt to remove the 
device.  usb-add will issue the qmp command, which will then fail.

I think this is probably true of most of the add/remove functions.

We could add an option that says "attempt to do the remove even if you 
don't see the device in xenstore" I suppose.  But I'm not really sure 
how to document that in a useful way without basically describing the 
internal mechanics of the command.

There's nothing we can really do about usb-list though.

On the whole I think I'd rather just say, "Don't do that."

  -George