Re: [PATCH V7 4/7] libxl: add libxl_device_usb_assignable_list API

[George Dunlap <george.dunlap@citrix.com>]

On 25/09/15 03:11, Chunyan Liu wrote:
> Add API for listing assignable USB devices info.
> Assignable USB device means the USB device type is assignable and
> it's not assigned to any guest yet.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> ---
> This could be squashed with previous patch. Split because there is
> some dispute on this. If this is acceptable, could be squashed,
> otherwise could be removed.
> 
>  tools/libxl/libxl.h       |  3 +++
>  tools/libxl/libxl_pvusb.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 633bfc1..c41b9de 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1450,6 +1450,9 @@ int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
>                                   libxl_usbctrlinfo *usbctrlinfo);
>  
>  /* USB Devices */
> +libxl_device_usb *
> +libxl_device_usb_assignable_list(libxl_ctx *ctx, int *num);
> +
>  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;
> diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
> index d4b997f..b683e60 100644
> --- a/tools/libxl/libxl_pvusb.c
> +++ b/tools/libxl/libxl_pvusb.c
> @@ -573,6 +573,61 @@ static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
>      return classcode != USBHUB_CLASS_CODE;
>  }
>  
> +libxl_device_usb *
> +libxl_device_usb_assignable_list(libxl_ctx *ctx, int *num)
> +{
> +    GC_INIT(ctx);
> +    libxl_device_usb *usbs = NULL;
> +    libxl_device_usb *assigned;
> +    int num_assigned;
> +    struct dirent *de;
> +    DIR *dir;
> +
> +    *num = 0;
> +
> +    if (get_assigned_devices(gc, &assigned, &num_assigned) < 0) {
> +        LOG(ERROR, "cannot determine if device is assigned");
> +        goto out;
> +    }
> +
> +    if (!(dir = opendir(SYSFS_USB_DEV)))
> +        goto out;
> +
> +    while ((de = readdir(dir))) {
> +        libxl_device_usb *usb;
> +        uint8_t bus = -1, addr = -1;
> +
> +        if (!de->d_name)
> +            continue;
> +
> +        usb_busaddr_from_busid(gc, de->d_name, &bus, &addr);
> +        if (bus < 1 || addr < 1)
> +            continue;
> +
> +        GCNEW(usb);
> +        usb->u.hostdev.hostbus = bus;
> +        usb->u.hostdev.hostaddr = addr;
> +
> +        if (!is_usb_assignable(gc, usb))
> +            continue;
> +
> +        if (is_usbdev_in_array(assigned, num_assigned, usb))
> +            continue;

Why are we allocating a new usb struct every loop, and then never using
it again?  Why not just put it on the stack and re-use it?

Other than that, looks OK.

 -George