Re: [Xen-devel] [PATCH V6 3/7] libxl: add pvusb API

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Aug 10, 2015 at 06:35:24PM +0800, 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: Chunyan Liu <cyliu@xxxxxxxx>
> Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx>
> 
> ---
> changes:
>   - Address George's comments:
>   * Update libxl_device_usb_getinfo to read ctrl/port only and
>     get other information.
>   * Update backend path according to xenstore frontend 'xxx/backend'
>     entry instead of using TOOLSTACK_DOMID.
>   * Use 'type' to indicate qemu/pv instead of previous naming 'protocol'.
>   * Add USB 'devtype' union, currently only includes "hostdev"
> 

I will leave this to Ian and George since they had strong opinions on
this.

I only skimmed this patch. Some comments below.

[...]
> +
> +int libxl_device_usb_getinfo(libxl_ctx *ctx, uint32_t domid,
> +                             libxl_device_usb *usb,
> +                             libxl_usbinfo *usbinfo);
> +
>  /* Network Interfaces */
>  int libxl_device_nic_add(libxl_ctx *ctx, uint32_t domid, libxl_device_nic 
> *nic,
>                           const libxl_asyncop_how *ao_how)
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index bee5ed5..935f25b 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -676,6 +676,10 @@ void libxl__devices_destroy(libxl__egc *egc, 
> libxl__devices_remove_state *drs)
>                  aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
>                  aodev->dev = dev;
>                  aodev->force = drs->force;
> +                if (dev->backend_kind == LIBXL__DEVICE_KIND_VUSB) {
> +                    libxl__initiate_device_usbctrl_remove(egc, aodev);
> +                    continue;
> +                }

Is there a risk that this races with individual device removal? I think
you get away with it because removal of individual device is idempotent?

>                  libxl__initiate_device_remove(egc, aodev);
>              }
>          }
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index f98f089..5be3b3a 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2553,6 +2553,14 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, 
> uint32_t domid,
>                                     libxl_device_vtpm *vtpm,
>                                     libxl__ao_device *aodev);
>  
> +_hidden void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
> +                                       libxl_device_usbctrl *usbctrl,
> +                                       libxl__ao_device *aodev);
> +
> +_hidden void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> +                                   libxl_device_usb *usb,
> +                                   libxl__ao_device *aodev);
> +
>  /* Internal function to connect a vkb device */
>  _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
>                                    libxl_device_vkb *vkb);
> @@ -2585,6 +2593,13 @@ _hidden void libxl__wait_device_connection(libxl__egc*,
>  _hidden void libxl__initiate_device_remove(libxl__egc *egc,
>                                             libxl__ao_device *aodev);
>  
> +_hidden int libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
[...]
> +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> +                           libxl_device_usb *usb,
> +                           libxl__ao_device *aodev)
> +{
> +    STATE_AO_GC(aodev->ao);
> +    int rc = -1;
> +    char *busid = NULL;
> +
> +    assert(usb->u.hostdev.hostbus > 0 && usb->u.hostdev.hostaddr > 0);
> +
> +    busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> +                                 usb->u.hostdev.hostaddr);
> +    if (!busid) {
> +        LOG(ERROR, "USB device doesn't exist in sysfs");
> +        goto out;
> +    }
> +
> +    if (!is_usb_assignable(gc, usb)) {
> +        LOG(ERROR, "USB device is not assignable.");
> +        goto out;
> +    }
> +
> +    /* check usb device is already assigned */
> +    if (is_usb_assigned(gc, usb)) {
> +        LOG(ERROR, "USB device is already attached to a domain.");
> +        goto out;
> +    }
> +
> +    rc = libxl__device_usb_setdefault(gc, domid, usb, aodev->update_json);
> +    if (rc) goto out;
> +
> +    rc = libxl__device_usb_add_xenstore(gc, domid, usb, aodev->update_json);
> +    if (rc) goto out;
> +
> +    rc = usbback_dev_assign(gc, usb);
> +    if (rc) {
> +        libxl__device_usb_remove_xenstore(gc, domid, usb);
> +        goto out;
> +    }
> +
> +    libxl__ao_complete(egc, ao, 0);
> +    rc = 0;
> +
> +out:

You forget to complete ao in failure path.

But I'm not very familiar with the AO machinery, I will let Ian comment
on this.

Wei.

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