Re: [PATCH RFC V2 3/5] libxl: add pvusb API

[Ian Campbell <ian.campbell@citrix.com>]

On Mon, 2015-01-19 at 16:28 +0800, Chunyan Liu wrote:
> Add pvusb APIs, including:
>  - attach/detach (create/destroy) virtual usb controller.
>  - attach/detach usb device
>  - list assignable usb devices in host
>  - some other helper functions
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>
> ---
>  tools/libxl/Makefile         |    2 +-
>  tools/libxl/libxl.c          |    2 +
>  tools/libxl/libxl.h          |   58 ++
>  tools/libxl/libxl_internal.h |    6 +
>  tools/libxl/libxl_usb.c      | 1277 ++++++++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxlu_cfg_y.c   |  464 ++++++++-------
>  tools/libxl/libxlu_cfg_y.h   |   38 +-
>  7 files changed, 1623 insertions(+), 224 deletions(-)
>  create mode 100644 tools/libxl/libxl_usb.c
> 
> diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile
> index b417372..08cdb12 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_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_usb.o $(LIBXL_OBJS-y)

The contents of that file looks Linux specific, so I think you might
have to arrange for it to only be built there and also to provide a
libxl_nousb.c with stubs returning appropriate errors to be used on
other platforms.

Or it may be possible (even better) to refactor the linux specific bits
of libxl_usb.c into libxl_linux.c and leave the common stuff behind.

I thought libxl_pci.c would be a good example of this, but it doesn't
seem to have any conditional stuff, yet I expected it to be Linux
specific. I've no idea how that works :-(. Maybe usb can get away with
it too.

> +int libxl_intf_to_device_usb(libxl_ctx *ctx, uint32_t domid,
> +                            char *intf, libxl_device_usb *usb)
> +                            LIBXL_EXTERNAL_CALLERS_ONLY;

I wasn't sure what an "intf" was on patch #1. hopefully your answer
there will help me understand what this is for!

> diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c
> new file mode 100644
> index 0000000..830a846
> --- /dev/null
> +++ b/tools/libxl/libxl_usb.c

Apart from my not yet understanding the interface semantics and the
potential for Linux-specificness mentioned above the actual code here
looks reasonably OK to me. I have smaller and larger comments below
though.

> +static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid,
> +                                            libxl_device_usbctrl *usbctrl)
> +{
[...]
> +    if(!usbctrl->backend_domid)

Missing space before (.


> +static int libxl__usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid,
> +                                       libxl_device_usbctrl *usbctrl)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    flexarray_t *front;
> +    flexarray_t *back;
> +    libxl__device *device;
> +    xs_transaction_t tran;
> +    int rc = 0;
> +
> +    GCNEW(device);
> +    rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device);
> +    if (rc) goto out;
> +
> +    front = flexarray_make(gc, 4, 1);
> +    back = flexarray_make(gc, 12, 1);
> +
> +    flexarray_append(back, "frontend-id");
> +    flexarray_append(back, libxl__sprintf(gc, "%d", domid));

GCSPRINTF would be good for all of these (and in lots of other places
too).

flexarray_append_pair is also nice for adding key+value at the same time
since it makes it easier to see what goes together.

> +retry_transaction:
> +    tran = xs_transaction_start(ctx->xsh);

Please follow the design pattern in e.g. libxl__device_vtpm_add or
device_disk_add and use libxl__xs_transaction_start/commit/abort here
inside a for (;;).

> +static int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid,
> +                                     libxl_device_usbctrl *usbctrl)
> +{
> [...]
> +    if (libxl__usbctrl_add_xenstore(gc, domid, usbctrl) < 0){

Space before { please.

> +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)
> +{
> [...]
> +    for (i = 0; i < numusb; i++) {
> +        if (libxl__device_usb_remove_common(gc, domid, &usbs[i], 0)) {
> +            fprintf(stderr, "libxl_device_usb_remove failed.\n");

Use LOG*( please, not fprintf (this is true everywhere in libxl in case
I missed any other).

> +/* usb device functions */
> +
> +/* Following functions are to get assignable usb devices */
> +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 = libxl__sprintf(gc,"/local/domain/0/backend/vusb");

GCSPRINTF.

Also I notice a hardcoded 0, shouldn't that be backend_domid somehow?
I've seen a few /0/ hardcoded here and there in this patch, which if not
backend_domid perhaps ought to at least be LIBXL_TOOLSTACK_DOMID.

If not then is it necessary to dup the string, can't it just be a const
string literal?

> +static bool is_usb_assignable(libxl__gc *gc, char *intf)
> +{
> +    char buf[5];
> +
> +    if (get_usb_bDeviceClass(gc, intf, buf) < 0)
> +        return false;
> +
> +    if (strcmp(buf, USBHUB_CLASS_CODE))
> +        return false;


OOI are hubs inherently unassignable, or is that a short coming of the
current driver code?

> +/* get all usb devices of the domain */
> +static libxl_device_usb *
> +libxl_device_usb_list_all(libxl__gc *gc, uint32_t domid, int *num)

Should be in libxl__ namespace, or since it is static you could omit the
namespacing completely if you prefer.

> +/* xenstore usb data */
> +static int libxl__device_usb_add_xenstore(libxl__gc *gc, uint32_t domid,
> +                                          libxl_device_usb *usb)
> +{
> +    libxl_ctx *ctx = CTX;

FYI although it's not required, you can just use CTX in the code if you
prefer.

> [...]
> +    be_path = libxl__sprintf(gc, "%s/port/%d", be_path, usb->port);
> +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new usb device to xenstore");

The LOG*( macros will help shorten this line.

> +    path = libxl__sprintf(gc, "cat "SYSFS_USB_DEVS_PATH"/%s/manufacturer", intf);

Whoah, what now?

What you want here is to open the fd and use read on it. We may even
have existing helpers for doing so. Certainly using popen on a string
starting with cat isn't what is wanted (at least not without a big and
convincing comment explaining why this should be needed).

I see a bunch of this in this area.

> +int libxl_device_usb_getinfo(libxl_ctx *ctx, char *intf, libxl_usbinfo *usbinfo)
> +{
> +    GC_INIT(ctx);
> +    char buf[512];
> +
> +    if (!get_usb_devnum(gc, intf, buf) )

In consistent spacing.

> +        usbinfo->devnum = atoi(buf);
> +
> +    if ( !get_usb_busnum(gc, intf, buf))
> +        usbinfo->bus = atoi(buf);
> +
> +    if (!get_usb_idVendor(gc, intf, buf) )
> +        usbinfo->idVendor = atoi(buf);
> +
> +    if (!get_usb_idProduct(gc, intf, buf) )
> +        usbinfo->idProduct  = atoi(buf);
> +
> +    if (!get_usb_manufacturer(gc, intf, buf) )
> +        usbinfo->manuf = strdup(buf);

libxl_strdup with NOGC to get correct error handling please.

> +
> +    if (!get_usb_product(gc, intf, buf) )
> +        usbinfo->prod = strdup(buf);
> +
> +    GC_FREE;
> +    return 0;
> +}
> +