[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V3 3/6] libxl: add pvusb API



(I look at overall code structure this pass. I haven't done a line by
line review.)

On Sun, Apr 19, 2015 at 11:50:49AM +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 to v2:
>   * remove qemu emulated usb related definitions, keep the work pure pvusb.
>     In last patch, will do all refactor work to unify pvusb and qemu emulated
>     usb.
>   * use bus.addr as user interface instead of busid to do usb-attach|detach
>   * remove usb-assignable-list APIs and some other unnecessary APIs
>   * reuse libxl_read_file_contents function instead of another new function
>     to handle getting sysfs file content
>   * fix build on different platforms as pci does
>   * fix many coding style problems
>   * address other comments in last version
>   * adjust codes to let it look better
> 
>  tools/libxl/Makefile                 |    2 +-
>  tools/libxl/libxl.c                  |    2 +
>  tools/libxl/libxl.h                  |   45 ++
>  tools/libxl/libxl_internal.h         |   11 +-
>  tools/libxl/libxl_osdeps.h           |   13 +
>  tools/libxl/libxl_pvusb.c            | 1201 
> ++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl          |   41 ++
>  tools/libxl/libxl_types_internal.idl |    1 +

You also need to document the xenstore keys and values somewhere under
docs directory.

And you forgot to update libxl_retrieve_domain_configuration function.

>  8 files changed, 1314 insertions(+), 2 deletions(-)
>  create mode 100644 tools/libxl/libxl_pvusb.c
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index 1b16598..d52281f 100644
> --- a/tools/libxl/Makefile
> +++ b/tools/libxl/Makefile
> @@ -95,7 +95,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o libxl_dm.o 
> libxl_pci.o \
>                       libxl_internal.o libxl_utils.o libxl_uuid.o \
>                       libxl_json.o libxl_aoutils.o libxl_numa.o libxl_vnuma.o 
> \
>                       libxl_save_callout.o _libxl_save_msgs_callout.o \
> -                     libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y)
> +                     libxl_qmp.o libxl_event.o libxl_fork.o libxl_pvusb.o 
> $(LIBXL_OBJS-y)
>  LIBXL_OBJS += libxl_genid.o
>  LIBXL_OBJS += _libxl_types.o libxl_flask.o _libxl_types_internal.o
>  
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b05d18b..a7c81d9 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1611,6 +1611,8 @@ void libxl__destroy_domid(libxl__egc *egc, 
> libxl__destroy_domid_state *dis)
>  
>      if (libxl__device_pci_destroy_all(gc, domid) < 0)
>          LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "pci shutdown failed for domid 
> %d", domid);
> +    if (libxl__device_usb_destroy_all(gc, domid) < 0)
> +         LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "usb shutdown failed for domid 
> %d", domid);
>      rc = xc_domain_pause(ctx->xch, domid);
>      if (rc < 0) {
>          LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc, "xc_domain_pause 
> failed for %d", domid);
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6bc75c5..cbe3519 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -114,6 +114,12 @@
>  #define LIBXL_HAVE_DOMAIN_NODEAFFINITY 1
>  
>  /*
> + * LIBXL_HAVE_PVUSB indicates the functions for doing hot-plug of
> + * USB devices through pvusb.
> + */
> +#define LIBXL_HAVE_PVUSB 1
> +
> +/*
>   * LIBXL_HAVE_BUILDINFO_HVM_VENDOR_DEVICE indicates that the
>   * libxl_vendor_device field is present in the hvm sections of
>   * libxl_domain_build_info. This field tells libxl which
> @@ -1224,6 +1230,45 @@ int libxl_cdrom_insert(libxl_ctx *ctx, uint32_t domid, 
> libxl_device_disk *disk,
>                         const libxl_asyncop_how *ao_how)
>                         LIBXL_EXTERNAL_CALLERS_ONLY;
>  
> +/* USB Controllers*/
> +int libxl_device_usbctrl_add(libxl_ctx *ctx, uint32_t domid,
> +                         libxl_device_usbctrl *usbctrl,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +int libxl_device_usbctrl_remove(libxl_ctx *ctx, uint32_t domid,
> +                         libxl_device_usbctrl *usbctrl,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +int libxl_device_usbctrl_destroy(libxl_ctx *ctx, uint32_t domid,
> +                         libxl_device_usbctrl *usbctrl,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +libxl_device_usbctrl *libxl_device_usbctrl_list(libxl_ctx *ctx,
> +                            uint32_t domid, int *num);
> +
> +
> +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
> +                                libxl_device_usbctrl *usbctrl,
> +                                libxl_usbctrlinfo *usbctrlinfo)
> +                                LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +/* USB Devices */
> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid, libxl_device_usb 
> *usb,
> +                         const libxl_asyncop_how *ao_how)
> +                         LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid, libxl_device_usb 
> *usb,
> +                            const libxl_asyncop_how *ao_how)
> +                            LIBXL_EXTERNAL_CALLERS_ONLY;
> +
> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid,
> +                                        int usbctrl, int *num);
> +
> +int libxl_device_usb_getinfo(libxl_ctx *ctx, char *intf, libxl_usbinfo 
> *usbinfo)
> +                             LIBXL_EXTERNAL_CALLERS_ONLY;
>  /* 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_internal.h b/tools/libxl/libxl_internal.h
> index 42eb1b9..f426ed8 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -2422,6 +2422,13 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, 
> uint32_t domid,
>                                     libxl_device_vtpm *vtpm,
>                                     libxl__ao_device *aodev);
>  
> +/* from libxl_usb */
> +_hidden int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid,
> +                                      libxl_device_usbctrl *usbctrl);
> +_hidden int libxl__device_usb_add(libxl__gc *gc, uint32_t domid,
> +                            libxl_device_usb *usb);
> +_hidden int libxl__device_usb_destroy_all(libxl__gc *gc, uint32_t domid);
> +
>  /* Internal function to connect a vkb device */
>  _hidden int libxl__device_vkb_add(libxl__gc *gc, uint32_t domid,
>                                    libxl_device_vkb *vkb);
> @@ -3628,7 +3635,9 @@ static inline void libxl__update_config_vtpm(libxl__gc 
> *gc,
>  #define COMPARE_PCI(a, b) ((a)->func == (b)->func &&    \
>                             (a)->bus == (b)->bus &&      \
>                             (a)->dev == (b)->dev)
> -
> +#define COMPARE_USB(a, b) (!strcmp((a)->busid, (b)->busid))
> +#define COMPARE_USBCTRL(a, b) ((a)->devid == (b)->devid)
> + 
>  /* DEVICE_ADD
>   *
>   * Add a device in libxl_domain_config structure
> diff --git a/tools/libxl/libxl_osdeps.h b/tools/libxl/libxl_osdeps.h
> index 08eaf0c..55caf71 100644
> --- a/tools/libxl/libxl_osdeps.h
> +++ b/tools/libxl/libxl_osdeps.h
> @@ -24,6 +24,8 @@
>  #define _GNU_SOURCE
>  
>  #if defined(__NetBSD__)
> +#define SYSFS_USB_DEV          "/sys/bus/usb/devices"
> +#define SYSFS_USBBACK_DRIVER   "/kern/xen/usb"
>  #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
>  #define SYSFS_PCIBACK_DRIVER   "/kern/xen/pci"
>  #define NETBACK_NIC_NAME       "xvif%ui%d"
> @@ -31,6 +33,8 @@
>  #elif defined(__OpenBSD__)
>  #include <util.h>
>  #elif defined(__linux__)
> +#define SYSFS_USB_DEV          "/sys/bus/usb/devices"
> +#define SYSFS_USBBACK_DRIVER   "/sys/bus/usb/drivers/usbback"
>  #define SYSFS_PCI_DEV          "/sys/bus/pci/devices"
>  #define SYSFS_PCIBACK_DRIVER   "/sys/bus/pci/drivers/pciback"
>  #define NETBACK_NIC_NAME       "vif%u.%d"
> @@ -38,12 +42,21 @@
>  #elif defined(__sun__)
>  #include <stropts.h>
>  #elif defined(__FreeBSD__)
> +#define SYSFS_USB_DEV          "/dev/null"
> +#define SYSFS_USBBACK_DRIVER   "/dev/null"
>  #define SYSFS_PCI_DEV          "/dev/null"
>  #define SYSFS_PCIBACK_DRIVER   "/dev/null"
>  #define NETBACK_NIC_NAME       "xnb%u.%d"
>  #include <libutil.h>
>  #endif
>  
> +#ifndef SYSFS_USBBACK_DRIVER
> +#error define SYSFS_USBBACK_DRIVER for your platform
> +#endif
> +#ifndef SYSFS_USB_DEV
> +#error define SYSFS_USB_DEV for your platform
> +#endif
> +
>  #ifndef SYSFS_PCIBACK_DRIVER
>  #error define SYSFS_PCIBACK_DRIVER for your platform
>  #endif
> diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
> new file mode 100644
> index 0000000..4e4975a
> --- /dev/null
> +++ b/tools/libxl/libxl_pvusb.c
> @@ -0,0 +1,1201 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU Lesser General Public License as published
> + * by the Free Software Foundation; version 2.1 only. with the special
> + * exception on linking described in file LICENSE.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU Lesser General Public License for more details.
> + */
> +
> +#include "libxl_osdeps.h" /* must come before any other headers */
> +
> +#include "libxl_internal.h"
> +
> +#define USBBACK_INFO_PATH "/libxl/usbback"
> +
> +#define USBHUB_CLASS_CODE 0x09
> +

I'm not very familiar with how USB is supposed to work. Can you explain
why this particular value is chosen?

> +static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
> +                                            libxl_device_usbctrl *usbctrl)
> +{
> +    int rc;
> +
> +    if (!usbctrl->version)
> +        usbctrl->version = 2;
> +

How hard would it be to implement USB 3? I assume this depends on QEMU's
support? I.e. we just need to specify the version to 3 and it should
just work? Just curious.

> +    if (!usbctrl->ports)
> +        usbctrl->ports = 8;
> +
> +    rc = libxl__resolve_domid(gc, usbctrl->backend_domname,
> +                              &usbctrl->backend_domid);
> +    return rc;
> +}
> +
> +static void libxl__device_from_usbctrl(libxl__gc *gc, uint32_t domid,
> +                                       libxl_device_usbctrl *usbctrl,
> +                                       libxl__device *device)
> +{
> +    device->backend_devid   = usbctrl->devid;
> +    device->backend_domid   = usbctrl->backend_domid;
> +    device->backend_kind    = LIBXL__DEVICE_KIND_VUSB;
> +    device->devid           = usbctrl->devid;
> +    device->domid           = domid;
> +    device->kind            = LIBXL__DEVICE_KIND_VUSB;
> +}
> +
> +static int libxl__usbport_add_xenstore(libxl__gc *gc,
> +                                       xs_transaction_t tran,
> +                                       uint32_t domid,
> +                                       libxl_device_usbctrl *usbctrl)
> +{
> +    char *path;
> +    int i;
> +
> +    path = GCSPRINTF("%s/backend/vusb/%d/%d/port",
> +                     libxl__xs_get_dompath(gc, 0), domid, usbctrl->devid);
> +
> +    libxl__xs_mkdir(gc, tran, path, NULL, 0);
> +
> +    for (i = 1; i <= usbctrl->ports; i++) {
> +        if (libxl__xs_write_checked(gc, tran, GCSPRINTF("%s/%d", path, i), 
> ""))
> +            return ERROR_FAIL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int libxl__usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
> +                                       libxl_device_usbctrl *usbctrl)
> +{
> +    flexarray_t *front;
> +    flexarray_t *back;
> +    libxl__device *device;
> +    xs_transaction_t t = XBT_NULL;
> +    int rc = 0;
> +    libxl_domain_config d_config;
> +    libxl_device_usbctrl usbctrl_saved;
> +    libxl__domain_userdata_lock *lock = NULL;
> +
> +    libxl_domain_config_init(&d_config);
> +    libxl_device_usbctrl_init(&usbctrl_saved);
> +    libxl_device_usbctrl_copy(CTX, &usbctrl_saved, usbctrl);
> +
> +    GCNEW(device);
> +    libxl__device_from_usbctrl(gc, domid, usbctrl, device);
> +
> +    front = flexarray_make(gc, 4, 1);
> +    back = flexarray_make(gc, 12, 1);
> +
> +    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", domid));
> +    flexarray_append_pair(back, "online", "1");
> +    flexarray_append_pair(back, "state", "1");
> +    flexarray_append_pair(back, "usb-ver", GCSPRINTF("%d", 
> usbctrl->version));
> +    flexarray_append_pair(back, "num-ports", GCSPRINTF("%d", 
> usbctrl->ports));
> +    flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", 
> usbctrl->backend_domid));

Line too long.

> +    flexarray_append_pair(front, "state", "1");
> +
> +    lock = libxl__lock_domain_userdata(gc, domid);
> +    if (!lock) {
> +        rc = ERROR_LOCK_FAIL;
> +        goto out;
> +    }
> +
> +    rc = libxl__get_domain_configuration(gc, domid, &d_config);
> +    if (rc) goto out;
> +
> +    DEVICE_ADD(usbctrl, usbctrls, domid, &usbctrl_saved, COMPARE_USBCTRL, 
> &d_config);
> +

Line too long.

> +    for (;;) {
> +        rc = libxl__xs_transaction_start(gc, &t);
> +        if (rc) goto out;
> +
> +        rc = libxl__device_exists(gc, t, device);
> +        if (rc < 0) goto out;
> +        if (rc == 1) {
> +            /* already exists in xenstore */
> +            LOG(ERROR, "device already exists in xenstore");
> +            rc = ERROR_DEVICE_EXISTS;
> +            goto out;
> +        }
> +
> +        rc = libxl__set_domain_configuration(gc, domid, &d_config);
> +        if (rc) goto out;
> +
> +        libxl__device_generic_add(gc, t, device,
> +                                  libxl__xs_kvs_of_flexarray(gc, back,
> +                                                             back->count),
> +                                  libxl__xs_kvs_of_flexarray(gc, front,
> +                                                             front->count),
> +                                  NULL);
> +        libxl__usbport_add_xenstore(gc, t, domid, usbctrl);
> +        rc = libxl__xs_transaction_commit(gc, &t);
> +        if (!rc) break;
> +        if (rc < 0) goto out;
> +    }
> +

You don't have aodev so you cannot check update_json. Maybe you need
aodev?

That field update_json is set to true when doing hotplug. It's set to
false during domain creation.

The same comment applies to other add functions as well.

> +out:
> +    if (lock) libxl__unlock_domain_userdata(lock);
> +    libxl_device_usbctrl_dispose(&usbctrl_saved);
> +    libxl_domain_config_dispose(&d_config);
> +    return rc;
> +}
> +
> +int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid,
> +                              libxl_device_usbctrl *usbctrl)
> +{
> +    int rc = 0;
> +
> +    rc = libxl__device_usbctrl_setdefault(gc, domid, usbctrl);
> +    if (rc < 0) goto out;
> +
> +    if (usbctrl->devid == -1) {
> +        usbctrl->devid = libxl__device_nextid(gc, domid, "vusb");
> +        if (usbctrl->devid < 0) {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +    }
> +
> +    if (libxl__usbctrl_add_xenstore(gc, domid, usbctrl) < 0){
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +out:
> +    return rc;
> +}
> +
> +int libxl_device_usbctrl_add(libxl_ctx *ctx, uint32_t domid,
> +                             libxl_device_usbctrl *usbctrl,
> +                             const libxl_asyncop_how *ao_how)
> +{
> +    AO_CREATE(ctx, domid, ao_how);
> +    int rc;
> +
> +    rc = libxl__device_usbctrl_add(gc, domid, usbctrl);

Hmm... Your remove function is async while this one is sync, why?

> +    libxl__ao_complete(egc, ao, rc);
> +    return AO_INPROGRESS;
> +}
> +
> +static int libxl__device_usb_list(libxl__gc *gc, uint32_t domid, int usbctrl,
> +                                  libxl_device_usb **usbs, int *num);
> +
> +static int do_usb_remove(libxl__gc *gc, uint32_t domid,
> +                         libxl_device_usb *usb);
> +
> +static int
> +libxl__device_usbctrl_remove_common(libxl_ctx *ctx, uint32_t domid,
> +                                    libxl_device_usbctrl *usbctrl,
> +                                    const libxl_asyncop_how *ao_how,
> +                                    int force)
> +{
> +    AO_CREATE(ctx, domid, ao_how);
> +    libxl__device *device;
> +    libxl__ao_device *aodev;
> +    libxl_device_usbctrl *usbctrls = NULL;
> +    libxl_device_usbctrl *usbctrl_find = NULL;
> +    int numctrl = 0;
> +    libxl_device_usb *usbs = NULL;
> +    int numusb = 0;
> +    int i, rc;
> +
> +    assert(usbctrl->devid != -1);
> +
> +    usbctrls = libxl_device_usbctrl_list(ctx, domid, &numctrl);
> +    if (!numctrl) {
> +        LOG(ERROR, "No USB controller exists in this domain");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    for (i = 0; i < numctrl; i++) {
> +        if (usbctrl->devid == usbctrls[i].devid) {
> +            usbctrl_find = usbctrls + i;
> +            break;
> +        }
> +    }
> +
> +    if (!usbctrl_find) {
> +        LOG(ERROR, "USB controller %d is not attached to this domain",
> +            usbctrl->devid);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    usbctrl = usbctrl_find;
> +
> +    GCNEW(device);
> +    libxl__device_from_usbctrl(gc, domid, usbctrl, device);
> +
> +    /* Remove usb devives first */
> +    rc  = libxl__device_usb_list(gc, domid, usbctrl->devid, &usbs, &numusb);
> +    if (rc) goto out;
> +    for (i = 0; i < numusb; i++) {
> +        if (do_usb_remove(gc, domid, &usbs[i])) {
> +            LOG(ERROR, "do_usb_remove failed");
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +    }
> +    /* remove usbctrl */
> +    GCNEW(aodev);
> +    libxl__prepare_ao_device(ao, aodev);
> +    aodev->action = LIBXL__DEVICE_ACTION_REMOVE;
> +    aodev->dev = device;
> +    aodev->callback = device_addrm_aocomplete;
> +    aodev->force = force;
> +    libxl__initiate_device_remove(egc, aodev);
> +
> +out:
> +    free(usbctrls);
> +    free(usbs);
> +    if(rc) return AO_ABORT(rc);
> +    return AO_INPROGRESS;
> +}
> +
> +int libxl_device_usbctrl_remove(libxl_ctx *ctx, uint32_t domid,
> +                                libxl_device_usbctrl *usbctrl,
> +                                const libxl_asyncop_how *ao_how)
> +{
> +    return libxl__device_usbctrl_remove_common(ctx, domid, usbctrl, ao_how, 
> 0);
> +}
> +
> +int libxl_device_usbctrl_destroy(libxl_ctx *ctx, uint32_t domid,
> +                                 libxl_device_usbctrl *usbctrl,
> +                                 const libxl_asyncop_how *ao_how)
> +{
> +    return libxl__device_usbctrl_remove_common(ctx, domid, usbctrl, ao_how, 
> 1);
> +}
> +
> +libxl_device_usbctrl *
> +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
> +{
> +    GC_INIT(ctx);
> +    libxl_device_usbctrl *usbctrls = NULL;
> +    char *fe_path = NULL;
> +    char **dir = NULL;
> +    unsigned int ndirs = 0;
> +
> +    *num = 0;
> +
> +    fe_path = GCSPRINTF("%s/device/vusb",
> +                        libxl__xs_get_dompath(gc, domid));
> +    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
> +
> +    if (dir && ndirs) {
> +        usbctrls = malloc(sizeof(*usbctrls) * ndirs);
> +        libxl_device_usbctrl* usbctrl;
> +        libxl_device_usbctrl* end = usbctrls + ndirs;
> +        for(usbctrl = usbctrls; usbctrl < end; usbctrl++, dir++, (*num)++) {
> +            char *tmp;
> +            const char *be_path = libxl__xs_read(gc, XBT_NULL,
> +                                    GCSPRINTF("%s/%s/backend", fe_path, 
> *dir));
> +
> +            libxl_device_usbctrl_init(usbctrl);
> +            usbctrl->devid = atoi(*dir);
> +
> +            tmp = libxl__xs_read(gc, XBT_NULL,
> +                                 GCSPRINTF("%s/%s/backend-id", fe_path, 
> *dir));
> +            if (!tmp) goto outerr;
> +            usbctrl->backend_domid = atoi(tmp);
> +
> +            tmp = libxl__xs_read(gc, XBT_NULL,
> +                                 GCSPRINTF("%s/usb-ver", be_path));
> +            if (!tmp) goto outerr;
> +            usbctrl->version = atoi(tmp);
> +
> +            tmp = libxl__xs_read(gc, XBT_NULL,
> +                                 GCSPRINTF("%s/num-ports", be_path));
> +            if (!tmp) goto outerr;
> +            usbctrl->ports = atoi(tmp);
> +       }
> +    }
> +
> +    return usbctrls;
> +
> +outerr:
> +    LOG(ERROR, "Unable to list USB Controllers");
> +    for (int i = 0; i < *num; i++) {
> +        libxl_device_usbctrl_dispose(usbctrls + i);
> +    }
> +    free(usbctrls);

It might be useful to provide a function to free the whole list of
device. There are similar functions in libxl.

> +    *num = 0;
> +    return NULL;
> +}
> +
> +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
> +                                libxl_device_usbctrl *usbctrl,
> +                                libxl_usbctrlinfo *usbctrlinfo)
> +{
> +    GC_INIT(ctx);
> +    char *dompath, *usbctrlpath;
> +    char *val;
> +    int rc = 0;
> +
> +    usbctrlinfo->devid = usbctrl->devid;
> +    usbctrlinfo->ports = usbctrl->ports;
> +    usbctrlinfo->version = usbctrl->version;
> +
> +    dompath = libxl__xs_get_dompath(gc, domid);
> +    usbctrlpath = GCSPRINTF("%s/device/vusb/%d", dompath, 
> usbctrlinfo->devid);
> +    val = libxl__xs_read(gc, XBT_NULL,
> +                         GCSPRINTF("%s/backend", usbctrlpath));
> +    usbctrlinfo->backend = libxl__strdup(NOGC, val);
> +    if (!usbctrlinfo->backend) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    val = libxl__xs_read(gc, XBT_NULL,
> +                         GCSPRINTF("%s/backend-id", usbctrlpath));
> +    usbctrlinfo->backend_id = val ? strtoul(val, NULL, 10) : -1;
> +
> +    val = libxl__xs_read(gc, XBT_NULL,
> +                         GCSPRINTF("%s/state", usbctrlpath));
> +    usbctrlinfo->state = val ? strtoul(val, NULL, 10) : -1;
> +
> +    val = libxl__xs_read(gc, XBT_NULL,
> +                         GCSPRINTF("%s/event-channel", usbctrlpath));
> +    usbctrlinfo->evtch = val ? strtoul(val, NULL, 10) : -1;
> +
> +    val = libxl__xs_read(gc, XBT_NULL,
> +                         GCSPRINTF("%s/urb-ring-ref", usbctrlpath));
> +    usbctrlinfo->ref_urb = val ? strtoul(val, NULL, 10) : -1;
> +
> +    val = libxl__xs_read(gc, XBT_NULL,
> +                         GCSPRINTF("%s/conn-ring-ref", usbctrlpath));
> +    usbctrlinfo->ref_conn= val ? strtoul(val, NULL, 10) : -1;
> +
> +    val = libxl__xs_read(gc, XBT_NULL,
> +                         GCSPRINTF("%s/frontend", usbctrlinfo->backend));
> +    usbctrlinfo->frontend = libxl__strdup(NOGC, val);
> +
> +    val = libxl__xs_read(gc, XBT_NULL,
> +                         GCSPRINTF("%s/frontend-id", usbctrlinfo->backend));
> +    usbctrlinfo->frontend_id = val ? strtoul(val, NULL, 10) : -1;
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}
> +
> +/* usb device functions */
> +
> +static int
> +libxl__device_usb_assigned_list(libxl__gc *gc,
> +                                libxl_device_usb **list, int *num)
> +{
> +    char **domlist;
> +    unsigned int nd = 0, i, j;
> +    char *be_path;
> +    libxl_device_usb *usb;
> +
> +    *list = NULL;
> +    *num = 0;
> +
> +    domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &nd);
> +    be_path = GCSPRINTF("/local/domain/%d/backend/vusb", 
> LIBXL_TOOLSTACK_DOMID);
> +    for (i = 0; i < nd; i++) {
> +        char *path, *num_ports, **ctrl_list;
> +        unsigned int nc = 0;
> +        path = GCSPRINTF("%s/%s", be_path, domlist[i]);
> +        ctrl_list = libxl__xs_directory(gc, XBT_NULL, path , &nc);
> +
> +        for (j = 0; j < nc; j++) {
> +            path = GCSPRINTF("%s/%s/%s/num-ports", be_path,
> +                             domlist[i], ctrl_list[j]);
> +            num_ports = libxl__xs_read(gc, XBT_NULL, path);
> +            if (num_ports) {
> +                int nport = atoi(num_ports), k;
> +                char *devpath, *busid;
> +
> +                for (k = 1; k <= nport; k++) {
> +                    devpath = GCSPRINTF("%s/%s/%s/port/%u", be_path,
> +                                        domlist[i], ctrl_list[j], k);
> +                    busid = libxl__xs_read(gc, XBT_NULL, devpath);
> +                    /* If there are USB device attached, add it to list */
> +                    if (busid && strcmp(busid, "") ) {
> +                        *list = realloc(*list,
> +                                  sizeof(libxl_device_usb) * ((*num) + 1));
> +                        if (*list == NULL)
> +                            return ERROR_NOMEM;

Use GCREALLOC_ARRAY.

> +                        usb = *list + *num;
> +                        usb->ctrl = atoi(ctrl_list[j]);
> +                        usb->port = k;
> +                        usb->busid = strdup(busid);
> +                        (*num)++;
> +                    }
> +                }
> +            }
> +        }
> +    }
> +    libxl__ptr_add(gc, *list);
> +

No need to do this if you use GCREALLOC_ARRAY.

> +    return 0;
> +}
> +
> +static bool is_usb_in_array(libxl_device_usb *usbs, int num,
> +                            libxl_device_usb *usb)
> +{
> +    int i;
> +
> +    for (i = 0; i < num; i++) {
> +        if (!strcmp(usbs[i].busid, usb->busid) )
> +            return true;
> +    }
> +
> +    return false;
> +}
> +
> +/* check if USB device is already assigned to a domain */
> +static bool is_usb_assigned(libxl__gc *gc, libxl_device_usb *usb)
> +{
> +    libxl_device_usb *usbs;
> +    int rc, num;
> +
> +    rc = libxl__device_usb_assigned_list(gc, &usbs, &num);
> +    if (rc) {
> +        LOG(ERROR, "Fail to get assigned usb list");
> +        return true;
> +    }
> +
> +    if (is_usb_in_array(usbs, num, usb))
> +        return true;
> +
> +    return false;
> +}
> +
> +/* check if USB device type is assignable */
> +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    int classcode;
> +    char *filename;
> +    void *buf;
> +
> +    assert(usb->busid);
> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", usb->busid);
> +    if (libxl_read_file_contents(ctx, filename, &buf, NULL) < 0)
> +        return false;
> +
> +    sscanf(buf, "%x", &classcode);
> +    if (classcode == USBHUB_CLASS_CODE)
> +        return false;
> +
> +    return true;
> +}
> +
> +/* get usb devices under certain usb controller */
> +static int libxl__device_usb_list(libxl__gc *gc, uint32_t domid, int usbctrl,
> +                                  libxl_device_usb **usbs, int *num)
> +{
> +    char *be_path, *num_devs;
> +    int n, i;
> +
> +    *usbs = NULL;
> +    *num = 0;
> +
> +    be_path = GCSPRINTF("%s/backend/vusb/%d/%d",
> +                        libxl__xs_get_dompath(gc, 0), domid, usbctrl);
> +    num_devs = libxl__xs_read(gc, XBT_NULL,
> +                              GCSPRINTF("%s/num-ports", be_path));
> +    if (!num_devs)
> +        return 0;
> +
> +    n = atoi(num_devs);
> +    *usbs = calloc(n, sizeof(libxl_device_usb));
> +

libxl__calloc(NOGC, ...)

> +    for (i = 0; i < n; i++) {
> +        char *busid;
> +        libxl_device_usb *usb = NULL;
> +
> +        busid = libxl__xs_read(gc, XBT_NULL,
> +                               GCSPRINTF("%s/port/%d", be_path, i + 1));
> +        if (busid && strcmp(busid, "")) {
> +            usb = *usbs + *num;
> +            usb->ctrl = usbctrl;
> +            usb->port = i + 1;
> +            usb->busid = strdup(busid);
> +            (*num)++;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +/* get all usb devices of the domain */
> +static libxl_device_usb *
> +libxl_device_usb_list_all(libxl__gc *gc, uint32_t domid, int *num)
> +{
> +    char **usbctrls;
> +    unsigned int nd, i, j;
> +    char *be_path;
> +    int rc;
> +    libxl_device_usb *usbs = NULL;
> +
> +    *num = 0;
> +
> +    be_path = GCSPRINTF("/local/domain/%d/backend/vusb/%d",
> +                        LIBXL_TOOLSTACK_DOMID, domid);
> +    usbctrls = libxl__xs_directory(gc, XBT_NULL, be_path, &nd);
> +
> +    for (i = 0; i < nd; i++) {
> +        int nc = 0;
> +        libxl_device_usb *tmp = NULL;
> +        rc = libxl__device_usb_list(gc, domid, atoi(usbctrls[i]), &tmp, &nc);
> +        if (!nc) continue;
> +
> +        usbs = realloc(usbs, sizeof(libxl_device_usb)*((*num) + nc));

libxl__realloc

> +        for(j = 0; j < nc; j++) {
> +            usbs[*num].ctrl = tmp[j].ctrl;
> +            usbs[*num].port = tmp[j].port;
> +            usbs[*num].busid = strdup(tmp[j].busid);
> +            (*num)++;
> +        }
> +        free(tmp);
> +    }
> +    return usbs;
> +}
> +
> +libxl_device_usb *libxl_device_usb_list(libxl_ctx *ctx, uint32_t domid,
> +                                        int usbctrl, int *num)
> +{
> +    GC_INIT(ctx);
> +    libxl_device_usb *usbs = NULL;
> +
> +    libxl__device_usb_list(gc, domid, usbctrl, &usbs, num);
> +
> +    GC_FREE;
> +    return usbs;
> +}
> +
> +/* set default value */
> +static char *usb_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    struct dirent *de;
> +    DIR *dir;
> +    char *busid = NULL;
> +
> +    if (bus < 1 || addr < 1)
> +        return NULL;
> +
> +    if (!(dir = opendir(SYSFS_USB_DEV)))
> +        return NULL;
> +
> +    while((de = readdir(dir))) {
> +        char *filename;
> +        void *buf;
> +        int busnum = -1;
> +        int devnum = -1;
> +
> +        if (!de->d_name)
> +            continue;
> +
> +        filename = GCSPRINTF(SYSFS_USB_DEV"/%s/devnum", de->d_name);
> +        if (!libxl_read_file_contents(ctx, filename, &buf, NULL))
> +            sscanf(buf, "%x", &devnum);
> +
> +        filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", de->d_name);
> +        if (!libxl_read_file_contents(ctx, filename, &buf, NULL))
> +            sscanf(buf, "%x", &busnum);
> +
> +        if (bus == busnum && addr == devnum) {
> +            busid = strdup(de->d_name);
> +            break;
> +        }
> +    }
> +
> +    closedir(dir);
> +    return busid;
> +}
> +
> +/* find first unused controller:port and give that to usb device */
> +static int
> +libxl__device_usb_set_default_usbctrl(libxl__gc *gc, uint32_t domid,
> +                                      libxl_device_usb *usb)
> +{
> +    libxl_ctx *ctx = CTX;
> +    libxl_device_usbctrl *usbctrls;
> +    libxl_device_usb *usbs = NULL;
> +    int numctrl, numusb, i, j, rc = -1;
> +    char *be_path, *tmp;
> +
> +    usbctrls = libxl_device_usbctrl_list(ctx, domid, &numctrl);
> +    if ( !numctrl)
> +        goto out;
> +
> +    for (i = 0; i < numctrl; i++) {
> +        rc = libxl__device_usb_list(gc, domid, usbctrls[i].devid,
> +                                    &usbs, &numusb);
> +        if (rc) continue;
> +
> +        if (!usbctrls[i].ports || numusb == usbctrls[i].ports)
> +            continue;
> +
> +        for (j = 1; i <= numusb; j++) {
> +            be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
> +                                libxl__xs_get_dompath(gc, 0), domid,
> +                                usbctrls[i].devid, j);
> +            tmp = libxl__xs_read(gc, XBT_NULL, be_path);
> +            if (tmp && !strcmp( tmp, "")) {
> +                usb->ctrl = usbctrls[i].devid;
> +                usb->port = j;
> +                break;
> +            }
> +        }
> +    }
> +
> +    rc = 0;
> +
> +out:
> +    if (usbctrls)
> +        free(usbctrls);
> +    if (usbs)
> +        free(usbs);
> +    return rc;
> +}
> +
> +static int libxl__device_usb_setdefault(libxl__gc *gc, uint32_t domid,
> +                                        libxl_device_usb *usb)
> +{
> +    char *be_path, *tmp;
> +
> +    if (usb->ctrl == -1) {
> +        int ret = libxl__device_usb_set_default_usbctrl(gc, domid, usb);
> +        /* If no existing ctrl to host this usb device, setup a new one */
> +        if (ret) {
> +            libxl_device_usbctrl usbctrl;
> +            libxl_device_usbctrl_init(&usbctrl);
> +            libxl__device_usbctrl_add(gc, domid, &usbctrl);
> +            usb->ctrl = usbctrl.devid;
> +            usb->port = 1;
> +            libxl_device_usbctrl_dispose(&usbctrl);
> +        }
> +    }
> +
> +    be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
> +                        libxl__xs_get_dompath(gc, 0),
> +                        domid, usb->ctrl, usb->port);
> +    tmp = libxl__xs_read(gc, XBT_NULL, be_path);
> +    if (!tmp || strcmp(tmp, "") ){
> +        LOG(ERROR, "The controller port isn't available");
> +        return ERROR_INVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +/* xenstore usb data */
> +static int libxl__device_usb_add_xenstore(libxl__gc *gc, uint32_t domid,
> +                                          libxl_device_usb *usb)
> +{
> +    char *be_path;
> +    int rc;
> +    libxl_domain_config d_config;
> +    libxl_device_usb usb_saved;
> +    libxl__domain_userdata_lock *lock = NULL;
> +
> +    libxl_domain_config_init(&d_config);
> +    libxl_device_usb_init(&usb_saved);
> +    libxl_device_usb_copy(CTX, &usb_saved, usb);
> +
> +    lock = libxl__lock_domain_userdata(gc, domid);
> +    if (!lock) {
> +        rc = ERROR_LOCK_FAIL;
> +        goto out;
> +    }
> +
> +    rc = libxl__get_domain_configuration(gc, domid, &d_config);
> +    if (rc) goto out;
> +
> +    DEVICE_ADD(usb, usbs, domid, &usb_saved, COMPARE_USB, &d_config);
> +
> +    rc = libxl__set_domain_configuration(gc, domid, &d_config);
> +    if (rc) goto out;
> +
> +    be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
> +                  libxl__xs_get_dompath(gc, 0), domid, usb->ctrl, usb->port);
> +    LOG(DEBUG, "Adding new usb device to xenstore");
> +    if (libxl__xs_write_checked(gc, XBT_NULL, be_path, usb->busid)) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +

This is not right. See libxl_internal.h, search for "lock json
config" to see the pattern. In short, you should use a xenstore
transaction instead of libxl__xs_write_checked with XBT_NULL.

Also you need to use aodev->update_json.

> +    rc = 0;
> +
> +out:
> +    if (lock) libxl__unlock_domain_userdata(lock);
> +    libxl_device_usb_dispose(&usb_saved);
> +    libxl_domain_config_dispose(&d_config);
> +    return rc;
> +}
> +
> +static int libxl__device_usb_remove_xenstore(libxl__gc *gc, uint32_t domid,
> +                                             libxl_device_usb *usb)
> +{
> +    char *be_path;
> +
> +    be_path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
> +                        libxl__xs_get_dompath(gc, 0),
> +                        domid, usb->ctrl, usb->port);
> +    LOG(DEBUG, "Removing USB device from xenstore");
> +    if (libxl__xs_write_checked(gc,XBT_NULL, be_path, ""))
> +        return ERROR_FAIL;
> +
> +    return 0;
> +}
> +
> +/* bind/unbind usb device interface */
> +static int unbind_usb_intf(libxl__gc *gc, char *intf, char **drvpath)
> +{
> +    char *path, *spath, *dp = NULL;
> +    int fd = -1;
> +    int rc = 0;
> +    struct stat st;
> +
> +    spath = GCSPRINTF(SYSFS_USB_DEV"/%s/driver", intf);
> +    if (!lstat(spath, &st)) {
> +        /* Find the canonical path to the driver. */
> +        dp = libxl__zalloc(gc, PATH_MAX);
> +        dp = realpath(spath, dp);
> +
> +        path = GCSPRINTF("%s/unbind", spath);
> +        fd = open(path, O_WRONLY);
> +        if (fd < 0)
> +            return ERROR_FAIL;
> +        rc = write(fd, intf, strlen(intf));
> +        close(fd);
> +        if (rc < 0)
> +            return ERROR_FAIL;
> +    }
> +
> +    if (drvpath)
> +        *drvpath = dp;
> +
> +    return 0;
> +}
> +
> +static int bind_usb_intf(libxl__gc *gc, char *intf, char *drvpath)
> +{
> +    char *path;
> +    struct stat st;
> +    int fd, rc = 0;
> +
> +    path = GCSPRINTF("%s/%s", drvpath, intf);
> +    rc = lstat(path, &st);
> +    /* already bind, return */
> +    if(rc == 0)
> +        return 0;
> +
> +    path = GCSPRINTF("%s/bind", drvpath);
> +    fd = open(path, O_WRONLY);
> +    if (fd < 0)
> +        return ERROR_FAIL;
> +
> +    rc = write(fd, intf, strlen(intf));
> +    close(fd);
> +    if (rc < 0)
> +        return ERROR_FAIL;
> +
> +    return 0;
> +}
> +
> +/* Is usb interface bound to usbback? */
> +static int usb_intf_is_assigned(libxl__gc *gc, char *intf)
> +{
> +    char *spath;
> +    int rc;
> +    struct stat st;
> +
> +    spath = GCSPRINTF(SYSFS_USBBACK_DRIVER"/%s", intf);
> +    rc = lstat(spath, &st);
> +
> +    if(rc == 0)
> +        return 1;
> +    if (rc < 0 && errno == ENOENT)
> +        return 0;
> +    LOGE(ERROR, "Accessing %s", spath);
> +    return -1;
> +}
> +
> +static int usb_get_all_interfaces(libxl__gc *gc, libxl_device_usb *usb,
> +                                  char ***intfs, int *num)
> +{
> +    DIR *dir;
> +    struct dirent *entry;
> +    char *buf;
> +    int rc = 0;
> +
> +    *intfs = NULL;
> +    *num = 0;
> +
> +    buf = GCSPRINTF("%s:", usb->busid);
> +
> +    if (!(dir = opendir(SYSFS_USB_DEV))) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    while ((entry = readdir(dir)) != NULL) {
> +        if (!strncmp(entry->d_name, buf, strlen(buf))){
> +            *intfs = realloc(*intfs, sizeof(char *) * (*num + 1));
> +            if (*intfs == NULL) {
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +            (*intfs)[*num] = strdup(entry->d_name);
> +            (*num)++;
> +        }
> +    }
> +
> +    closedir(dir);
> +
> +out:
> +    return rc;
> +}
> +
> +/* Cann't write '.' or ':' into Xenstore as key. So, change '.' to '_',

"Can't"

And this behaviour needs to be documented.

> + * change ':' to '-'.
> + */
> +static char *usb_interface_encode(char *busid)
> +{
> +    char *str = strdup(busid);
> +    int i, len = strlen(str);
> +
> +    for (i = 0; i < len; i++) {
> +        if (str[i] == '.')
> +            str[i] = '_';
> +         if (str[i] == ':')

Indentation.

> +            str[i] = '-';
> +    }
> +    return str;
> +}
> +
> +/* unbind usb device from usbback driver, if there are many interfaces
> + * under the usb device, then check each interface, unbind from usbback
> + * driver and rebind to original driver
> + */
> +static int usbback_dev_unassign(libxl__gc *gc, libxl_device_usb *usb)
> +{
> +    char **intfs = NULL;
> +    char *path;
> +    int num = 0, i;
> +    int rc = 0;
> +    char *usb_encode = NULL;
> +
> +    if (usb_get_all_interfaces(gc, usb, &intfs, &num) < 0)
> +        return ERROR_FAIL;
> +
> +    usb_encode = usb_interface_encode(usb->busid);
> +
> +    for (i = 0; i < num; i++){
> +        char *intf = intfs[i];
> +        char *drvpath = NULL;
> +
> +        if (usb_intf_is_assigned(gc, intf) > 0) {
> +            /* unbind interface from usbback driver */
> +            if (unbind_usb_intf(gc, intf, NULL) < 0) {
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +        }
> +
> +        /* bind interface to its originial driver */
> +        drvpath = libxl__xs_read(gc, XBT_NULL,
> +                  GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path",
> +                  usb_encode, usb_interface_encode(intf)));
> +        if (drvpath) {
> +            if (bind_usb_intf(gc, intf, drvpath) < 0) {
> +                LOGE(ERROR, "Couldn't bind %s to %s", intf, drvpath);
> +                rc = ERROR_FAIL;
> +                goto out;
> +            }
> +        }
> +    }
> +
> +    /* finally, remove xs driver path */
> +    path = GCSPRINTF(USBBACK_INFO_PATH"/%s", usb_encode);
> +    libxl__xs_rm_checked(gc, XBT_NULL, path);
> +
> +out:
> +    if (intfs) {
> +        for (i = 0; i < num; i++)
> +            free(intfs[i]);
> +        free(intfs);
> +    }
> +    free(usb_encode);
> +    return rc;
> +}
> +
> +/* bind usb device to "usbback" driver, if there are many interfaces
> + * under the usb device, check each interface, unbind from original
> + * driver and bind to usbback driver.
> + */
> +static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb)
> +{
> +    char **intfs = NULL;
> +    int num = 0, i;
> +    int rc = 0;
> +    char *usb_encode = NULL;
> +
> +    if (usb_get_all_interfaces(gc, usb, &intfs, &num) < 0)
> +        return ERROR_FAIL;
> +
> +    usb_encode = usb_interface_encode(usb->busid);
> +
> +    for (i = 0; i < num; i++){
> +        char *intf = intfs[i];
> +        char *path = NULL;
> +        char *drvpath = NULL;
> +
> +        /* already assigned to usbback */
> +        if (usb_intf_is_assigned(gc, intf) > 0)
> +            continue;
> +
> +        /* unbind interface from original driver */
> +        if (unbind_usb_intf(gc, intf, &drvpath) < 0) {
> +            rc = ERROR_FAIL;
> +            goto out_rebind;
> +        }
> +
> +        if (drvpath) {
> +            /* write driver path to xenstore for later rebinding */
> +            path = GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path",
> +                             usb_encode, usb_interface_encode(intf));
> +            if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0) {
> +                LOG(WARN, "Write of %s to node %s failed", drvpath, path);
> +            }
> +        }
> +
> +        /* bind interface to usbback */
> +        if (bind_usb_intf(gc, intf, SYSFS_USBBACK_DRIVER) < 0){
> +            LOGE(ERROR, "Couldn't bind %s to %s", intf, 
> SYSFS_USBBACK_DRIVER);
> +            rc = ERROR_FAIL;
> +            goto out_rebind;
> +        }
> +    }
> +
> +    goto out;
> +
> +out_rebind:
> +    /* some interfaces might be bound to usbback, unbind it then and
> +     * rebind to its original driver
> +     */
> +    usbback_dev_unassign(gc, usb);
> +out:
> +    if (intfs) {
> +        for (i = 0; i < num; i++)
> +            free(intfs[i]);
> +        free(intfs);
> +    }
> +    free(usb_encode);
> +    return rc;
> +}
> +
> +static int do_usb_add(libxl__gc *gc, uint32_t domid, libxl_device_usb *usb)
> +{
> +    int rc = 0;
> +
> +    rc = libxl__device_usb_add_xenstore(gc, domid, usb);
> +    if (rc) goto out;
> +
> +    rc = usbback_dev_assign(gc, usb);
> +    if (rc)
> +        libxl__device_usb_remove_xenstore(gc, domid, usb);
> +
> +out:
> +    return rc;
> +}
> +
> +int libxl__device_usb_add(libxl__gc *gc, uint32_t domid, libxl_device_usb 
> *usb)
> +{
> +    int rc;
> +
> +    usb->busid = usb_busaddr_to_busid(gc, usb->hostbus, usb->hostaddr);
> +    if (!usb->busid) {
> +        LOG(ERROR, "USB device doesn't exist in sysfs");
> +        return ERROR_INVAL;
> +    }
> +
> +    if (!is_usb_assignable(gc, usb)) {
> +        LOG(ERROR, "USB device is not assignable.");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    /* check usb device is already assigned by pvusb */
> +    if (is_usb_assigned(gc, usb)) {
> +        LOG(ERROR, "USB device is already attached to a domain.");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    rc = libxl__device_usb_setdefault(gc, domid, usb);
> +    if (rc) goto out;
> +
> +    rc = do_usb_add(gc, domid, usb);
> +
> +out:
> +    return rc;
> +}
> +
> +int libxl_device_usb_add(libxl_ctx *ctx, uint32_t domid,
> +                         libxl_device_usb *usb,
> +                         const libxl_asyncop_how *ao_how)
> +{
> +    AO_CREATE(ctx, domid, ao_how);
> +    int rc;
> +
> +    rc = libxl__device_usb_add(gc, domid, usb);

Hmm... Your remove function is async and this one is sync. See comment
of usbctrl_add.

> +    libxl__ao_complete(egc, ao, rc);
> +    return AO_INPROGRESS;
> +}
> +
> +static int do_usb_remove(libxl__gc *gc, uint32_t domid,
> +                         libxl_device_usb *usb)
> +{
> +    if (libxl__device_usb_remove_xenstore(gc, domid, usb))
> +        return -1;
> +
> +    usbback_dev_unassign(gc, usb);
> +
> +    return 0;
> +}
> +
> +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid,
> +                                    libxl_device_usb *usb)
> +{
> +    libxl_device_usb *usbs = NULL;
> +    libxl_device_usb *usb_find = NULL;
> +    int i, num, rc;
> +
> +    usb->busid = usb_busaddr_to_busid(gc, usb->hostbus, usb->hostaddr);
> +    if (!usb->busid) {
> +        LOG(ERROR, "USB device doesn't exist in sysfs");
> +        return ERROR_INVAL;
> +    }
> +
> +    usbs = libxl_device_usb_list_all(gc, domid, &num);
> +    if (!usbs) {
> +       LOG(ERROR, "No USB device attached to this domain");
> +       return ERROR_FAIL;
> +    }
> +
> +    for (i = 0; i < num; i++) {
> +        if (!strcmp(usb->busid, usbs[i].busid)) {
> +            usb_find = usbs + i;
> +            break;
> +        }
> +    }
> +
> +    /* doesn't find the usb device in domain's usb device list*/
> +    if (!usb_find) {
> +        LOG(ERROR, "USB device %x.%x is not attached to this domain",
> +            usb->hostbus, usb->hostaddr);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    rc = do_usb_remove(gc, domid, usb_find);
> +
> +out:
> +    free(usbs);
> +    return rc;
> +}
> +
> +int libxl_device_usb_remove(libxl_ctx *ctx, uint32_t domid,
> +                            libxl_device_usb *usb,
> +                            const libxl_asyncop_how *ao_how)
> +
> +{
> +    AO_CREATE(ctx, domid, ao_how);
> +    int rc;
> +
> +    rc = libxl__device_usb_remove(gc, domid, usb);
> +

Same here.

Wei.

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.