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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 02/03/16 18:32, George Dunlap wrote:
> 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."

Sorry, just looked through the rest of the series, and there's one more
thing.

Neither here nor in the man page do we explain what to do if something
goes wrong with the detach.  I think the best thing to do is probably to
make the logged error messages more helpful.

What about something like this:

* On failure to unbind: "Error removing device from guest.  Try running
usbdev-detach again."

* On failure to rebind: "USB device removed from guest, but couldn't
re-bind to domain 0.  Try removing and re-inserting the USB device or
reloading the driver modules."

What do you think?

 -George

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