Re: [Xen-devel] [PATCH V15 4/6] libxl: add pvusb API

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 01/03/16 08:09, Chunyan Liu wrote:
> Add pvusb APIs, including:
>  - attach/detach (create/destroy) virtual usb controller.
>  - attach/detach usb device
>  - list usb controller and usb devices
>  - some other helper functions
> 
> Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx>
> ---
> Changes:
>   reorder usbdev_remove to following three steps:
>   1. Unassign all interfaces from usbback, stopping and returning an
>      error as soon as one attempt fails
>   2. Remove the pvusb xenstore nodes, stopping and returning an error
>      if it fails
>   3. Attempt to re-assign all interfaces to the original drivers,
>      stopping and returning an error as soon as one attempt fails.

Thanks, Chunyan!  One minor comment about these changes...

> +static int usbdev_rebind(libxl__gc *gc, const char *busid)
> +{
> +    char **intfs = NULL;
> +    char *usbdev_encode = NULL;
> +    char *path = NULL;
> +    int i, num = 0;
> +    int rc;
> +
> +    rc = usbdev_get_all_interfaces(gc, busid, &intfs, &num);
> +    if (rc) goto out;
> +
> +    usbdev_encode = usb_interface_xenstore_encode(gc, busid);
> +
> +    for (i = 0; i < num; i++) {
> +        char *intf = intfs[i];
> +        char *usbintf_encode = NULL;
> +        const char *drvpath;
> +
> +        /* rebind USB interface to its originial driver */
> +        usbintf_encode = usb_interface_xenstore_encode(gc, intf);
> +        path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path",
> +                         usbdev_encode, usbintf_encode);
> +        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath);
> +        if (rc) goto out;
> +
> +        if (drvpath) {
> +            rc = bind_usbintf(gc, intf, drvpath);
> +            if (rc) {
> +                LOGE(ERROR, "Couldn't rebind %s to %s", intf, drvpath);
> +                goto out;
> +            }
> +        }
> +    }
> +
> +    path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode);
> +    rc = libxl__xs_rm_checked(gc, XBT_NULL, path);
> +
> +out:

So it looks like if one of the re-binds fails, then it stops where it is
and leaves the USBBACK re-bind info in xenstore.  In that case it's not
clear to me how that information would ever be removed.

I think until such time as we have a command to re-attempt the re-bind,
 if there's an error in the actual rebind, it should just break out of
the for loop, and remove the re-bind nodes, and document a way to let
the user try to clean things up.

> +static int do_usbdev_remove(libxl__gc *gc, uint32_t domid,
> +                            libxl_device_usbdev *usbdev)
> +{
> +    int rc;
> +    char *busid;
> +    libxl_device_usbctrl usbctrl;
> +    libxl_usbctrlinfo usbctrlinfo;
> +
> +    libxl_device_usbctrl_init(&usbctrl);
> +    libxl_usbctrlinfo_init(&usbctrlinfo);
> +    usbctrl.devid = usbdev->ctrl;
> +
> +    rc = libxl_device_usbctrl_getinfo(CTX, domid, &usbctrl, &usbctrlinfo);
> +    if (rc) goto out;
> +
> +    switch (usbctrlinfo.type) {
> +    case LIBXL_USBCTRL_TYPE_PV:
> +        busid = usbdev_busid_from_ctrlport(gc, domid, usbdev);
> +        if (!busid) {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +
> +        rc = usbback_dev_unassign(gc, busid);
> +        if (rc) goto out;
> +
> +        rc = libxl__device_usbdev_remove_xenstore(gc, domid, usbdev);
> +        if (rc) goto out;
> +
> +        rc = usbdev_rebind(gc, busid);
> +        if (rc) goto out;

I think we need a comment here saying why we're doing things in this
order.  Maybe:

"Things are done in this order to balance simplicity with robustness in
the case of failure:
* We unbind all interfaces before rebinding any interfaces, so that we
never get into a situation where some interfaces are assigned to usbback
and some are assigned to the original drivers.
* We also unbind the interfaces before removing the pvusb xenstore
nodes, so that if the unbind fails in the middle, the device still shows
up in xl usb-list, and the user can re-try removing it."

Other than that, I gave this patche a moderately thorough review again
today, and I think everything else looks good to me.

 -George


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