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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

Ian.

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