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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Chunyan Liu writes ("[PATCH V8 3/7] libxl: add pvusb API"):
> 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

Thanks for this.


I have reviewed it in detail (not just the ao handling aspects) and
(I'm afraid) produced a large number of comments, relating to style,
error handling, etc., as well as ao handling.

Please let me know if anything I've said is unclear.  I've been rather
brief in my comments, rather than writing a paragraph for each one.
Thanks.


> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index dacfaae..a050e8b 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4218,11 +4218,54 @@ DEFINE_DEVICE_REMOVE(vtpm, destroy, 1)
...
> +/* Macro for defining device remove/destroy functions for usbctrl */
> +/* Following functions are defined:
> + * libxl_device_usbctrl_remove
> + * libxl_device_usbctrl_destroy
> + */
> +
> +#define DEFINE_DEVICE_REMOVE_EXT(type, removedestroy, f)                \

As George has said, I would prefer to avoid committing this
duplication in-tree, even with a promise to de-duplicated it later.


> +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid,
> +                               libxl_device_usbctrl *usbctrl,
> +                               libxl__ao_device *aodev)
> +{

Thanks for adjusting the error-handling patterns in these functions.
The new way is good, except that:

> +out:
> +    aodev->rc = rc;
> +    if (rc) aodev->callback(egc, aodev);

Here, rc is always set, and indeed the code would be wrong if it were
not.  So can you remove the conditional please ?  Ie:

  +   aodev->callback(egc, aodev);

The same goes for:

> +static int
> +libxl__device_usb_remove(libxl__gc *gc, uint32_t domid, libxl_device_usb 
> *usb);
...

> +    /* Remove usb devices first */
> +    rc  = libxl__device_usb_list_for_usbctrl(gc, domid, usbctrl_devid,
> +                                             &usbs, &numusb);
         ^^
While you're fixing other things, you could remove one of these space.s

> +libxl_device_usbctrl *
> +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num)
> +{
...
> +        for (usbctrl = usbctrls; usbctrl < end;
> +             usbctrl++, entry++, (*num)++) {

I think this would be clearer if there were a line break at the first
semicolon too.  Ie, I like
    for (A; B; C) {
or
    for (A;
         B;
         C) {

But I don't like very much
    for (A; B;
         C) {
or
    for (A;
         B; C) {

> +#define READ_SUBPATH(path, subpath) ({                                  \
> +        rc = libxl__xs_read_checked(gc, XBT_NULL,                       \
> +                                    GCSPRINTF("%s/" subpath, path),     \
> +                                    &tmp);                              \
> +        if (rc) goto outerr;                                            \
> +        (char *)tmp;                                                    \
> +    })

Thanks, I'm pleased with how you have done this.

> +            be_path = READ_SUBPATH(fe_path, "backend");
> +            usbctrl->backend_domid = atoi(READ_SUBPATH(fe_path, 
> "backend-id"));
> +            usbctrl->version = atoi(READ_SUBPATH(be_path, "usb-ver"));
> +            usbctrl->ports = atoi(READ_SUBPATH(be_path, "num-ports"));

However, I have a question:

Why do you use atoi here, but strtoul in libxl_device_usbctrl_getinfo ?

> +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid,
> +                                libxl_device_usbctrl *usbctrl,
> +                                libxl_usbctrlinfo *usbctrlinfo)
> +{
...
> +    tmp = READ_SUBPATH(fe_path, "backend-id");
> +    usbctrlinfo->backend_id = tmp ? strtoul(tmp, NULL, 10) : -1;

There are ten copies of this pattern with tmp and strtoul.  I really
think this needs to be refactored somehow.  Can you please make a
macro which returns the return value from strtoul ?

> +static char *usb_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
> +{
...
> +    while ((de = readdir(dir))) {

You need to use readdir_r, I'm afraid.  Ordinary readdir is not
threadsafe.

> +    }
> +
> +    closedir(dir);

This assumes that all errors from readdir are EOF.  But that's not the
case.  Luckily readdir_r has more sensible error behaviour.  But you
do need to check separately for errors and EOF.

All in all this probably means that it would be clearer to write this
as:
     for (;;) {
         r = readdir_r etc.
         if (r) ....
         if (!de) break;

There is at least one more call to readdir in this patch which also
needs to be fixed.

> +static int usb_busaddr_from_busid(libxl__gc *gc, const char *busid,
> +                                  uint8_t *bus, uint8_t *addr)
> +{
> +    char *filename;
> +    void *buf;
> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid);
> +    if (!libxl__read_sysfs_file_contents(gc, filename, &buf, NULL))
> +        *bus = atoi((char *)buf);

I don't think this cast (and the one for addr) are necessary ?

> +static int
> +get_assigned_devices(libxl__gc *gc,
> +                     libxl_device_usb **list, int *num)
> +{
...
> +out:
> +    if (rc) {
> +        *list = NULL;
> +        *num = 0;
> +    }

Is this necessary ?  Given that get_assigned_devices only has internal
callers which expect gc'd memory, there is no memory cleanup issue.
And a caller who gets a nonzero return value ought not to then look at
the results.

The only existing call site seems not to care.

However, it would probably be better style to print a log message
here, rather than in the caller.


> +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
> +{
...
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", busid);
                                         ^
Our usual coding style would have a space here.

> +static int
> +libxl__device_usb_list_for_usbctrl(libxl__gc *gc, uint32_t domid,
> +                                   libxl_devid usbctrl,
> +                                   libxl_device_usb **usbs, int *num)
...
> +    for (i = 0; i < n; i++) {
> +        char *busid;
> +        libxl_device_usb *usb;
> +
> +        busid = libxl__xs_read(gc, XBT_NULL,
> +                               GCSPRINTF("%s/port/%d", be_path, i + 1));

You should be using libxl__xs_read_checked here I think.

This is true throughout.  There are more calls to libxl__xs_read which
probably need to be replaced.

...
> +out:
> +    if (rc) {
> +        *usbs = NULL;
> +        *num = 0;
> +    }

As comment as before.  However, the one call site for this ignores
errors, which is a bug.


However.  I think that get_assigned_devices has too much code that
looks like the middle of libxl__device_usb_list_for_usbctrl and
libxl_device_usb_list.

I think you could write get_assigned_devices in terms of
libxl_device_usb_list or maybe libxl__device_usb_list_for_usbctrl.


> +        fe_path = GCSPRINTF("%s/device/vusb/%d",
> +                         libxl__xs_get_dompath(gc, domid), usb->ctrl);
> +
> +        rc = libxl__xs_read_checked(gc, XBT_NULL,
> +                                    GCSPRINTF("%s/backend", fe_path), 
> &be_path);
> +        if (rc) goto out;

After reading the backend path out of the frontend, you need to check
that it's plausible.  Otherwise there perhaps possible attacks where
the frontend writes a backend path pointing somewhere else in
xenstore.

> +        rc = libxl__xs_read_checked(gc, XBT_NULL,
> +                                    GCSPRINTF("%s/port/%d", be_path, 
> usb->port),
> +                                    &tmp);
> +        if (rc) goto out;
> +
> +        if (strcmp(tmp, "")) {

This is rather odd.  What happens if the path doesn't exist ?  AFAICT
this will segfault.  (Another case in libxl_ctrlport_to_device_usb, at
least.)

....
> +/* 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;
> +    struct stat st;
> +
> +    spath = GCSPRINTF(SYSFS_USB_DEV"/%s/driver", intf);
> +    if (!lstat(spath, &st)) {

The code should not assume that all lstate failures are ENOENT.

> +        /* Find the canonical path to the driver. */
> +        dp = libxl__zalloc(gc, PATH_MAX);
> +        dp = realpath(spath, dp);

Why is this call to realpath needed ?

> +        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;

All of these system calls should log errors (errno) appropriately.
The write should check that the expected amount was written.

rc should only be used for a libxl error code.  See CODING_STYLE.
(Multiple occurrences throughout this patch.)

Do we know that writes to sysfs don't ever return short or EINTR ?

> +static int bind_usb_intf(libxl__gc *gc, char *intf, char *drvpath)
> +{

This function has a remarkable resemblance to much of the unbind
function.  The common code should be factored out.

> +static int usb_get_all_interfaces(libxl__gc *gc, libxl_device_usb *usb,
> +                                  char ***intfs, int *num)
> +{
> +    DIR *dir;

This (and the earlier function which also uses opendir) uses the wrong
resource / error-handling pattern for dir.  See ERROR HANDLING in
CODING_SYTLE.

> +    busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
> +                                 usb->u.hostdev.hostaddr);
> +    if (!busid) {
> +        rc = ERROR_FAIL;

I think these internal functions such as usb_busaddr_to_busid, _bind,
_is_assignable, etc., ought to return rc values, rather than all the
callers inventing ERROR_FAIL.


> +    if (!(dir = opendir(SYSFS_USB_DEV))) {
> +        rc = ERROR_FAIL;

Again, you need to log errno (and the path, too).

> +/* Encode usb interface so that it could be written to xenstore as a key.
> + *
> + * Since xenstore key cannot include '.' or ':', we'll change '.' to '_',
> + * change ':' to '-'. For example, 3-1:2.1 will be encoded to 3-1-2_1.
> + * This will be used to save original driver of USB device to xenstore.
> + */

What is the syntax of the incoming busid ?  Could it contain _ or - ?
You should perhaps spot them and reject if it does.

> +static char *usb_interface_xenstore_encode(char *busid)
> +{
> +    char *str = strdup(busid);

This function should either update the value in place, or be
const-correct (ie, take a const char*).

Do not use bare strdup.  This means if it's going to make a copy, it
needs to take a gc.


> +static int usbback_dev_unassign(libxl__gc *gc, libxl_device_usb *usb)
> +{
...
> +        /* 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_xenstore_encode(intf)));
> +        if (drvpath && bind_usb_intf(gc, intf, drvpath))
> +            LOGE(WARN, "Couldn't bind %s to %s", intf, drvpath);

This error message could be clearer.

Also it would be worth a comment in the code to explain why this is a
warning (merely logged), rather than an error (logged and reported
with nonzero status to caller).

> +/* Bind USB device to "usbback" driver.
> + *
> + * If there are many interfaces under 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)
> +{
...
> +            if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0) {
> +                LOG(WARN, "Write of %s to node %s failed", drvpath, path);
> +            }

One of the advantages of libxl__xs_write_checked is that it logs
errors.  So you can leave that log message out.

However, if the xs write fails, you should presumably stop, rather
than carrying on.

I think you probably do things in the wrong order here.  You should
write the old driver info to xenstore first, so that if the bind to
usbback fails, you have the old driver info.

> +    free(usb_encode);

This came from the gc, I think.


> +/*
> + * USB add
> + */

Many of these comments often don't add much.  OTOH I won't insist you
remove them.

> +static int do_usb_add(libxl__gc *gc, uint32_t domid,
> +                      libxl_device_usb *usbdev,
> +                      libxl__ao_device *aodev)
> +{
...
> +    switch (usbctrlinfo.type) {
> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> +        LOG(ERROR, "Not supported");
> +        break;

Needs to set rc.

> +    case LIBXL_USBCTRL_TYPE_PV:
> +        rc = libxl__device_usb_add_xenstore(gc, domid, usbdev,
> +                                            aodev->update_json);
> +        if (rc) goto out;
> +
> +        rc = usbback_dev_assign(gc, usbdev);
> +        if (rc) {
> +            libxl__device_usb_remove_xenstore(gc, domid, usbdev);
> +            goto out;
> +        }
> +        break;
> +    default:
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +out:

Before out, rc should be explicitly set to 0, rather than relying on
the happenstance of the previous setting.

> +void libxl__device_usb_add(libxl__egc *egc, uint32_t domid,
> +                           libxl_device_usb *usb,
> +                           libxl__ao_device *aodev)
> +{
...
> +        if (usbctrlinfo.backend_id != 0) {

LIBXL_TOOLSTACK_DOMID please.  (And in remove, too.)

Why is this check done in libxl__device_usb_add and do_usb_remove,
rather than in a symmetrical way ?

> +    /* Do the add */
> +    if (do_usb_add(gc, domid, usb, aodev)) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    libxl__ao_complete(egc, ao, 0);
> +    rc = 0;
> +
> +out:
> +    libxl_device_usbctrl_dispose(&usbctrl);
> +    libxl_usbctrlinfo_dispose(&usbctrlinfo);
> +    aodev->rc = rc;
> +    if (rc) aodev->callback(egc, aodev);
> +    return;

Calling libxl__ao_complete here is quite wrong.  I think you should
call aodev->callback in both success and failure cases.  And it must
be the last thing you do.

> +static int do_usb_remove(libxl__gc *gc, uint32_t domid,
> +                         libxl_device_usb *usbdev)
> +{
> +    libxl_ctx *ctx = CTX;

What is this for ?  I think, instead, you can just refer to CTX where
needed.


...
> +    switch (usbctrlinfo.type) {
> +    case LIBXL_USBCTRL_TYPE_DEVICEMODEL:
> +        LOG(ERROR, "Not supported");
> +        break;
> +    case LIBXL_USBCTRL_TYPE_PV:
> +        rc = libxl__device_usb_remove_xenstore(gc, domid, usbdev);
> +        if (rc) goto out;
> +
> +        usbback_dev_unassign(gc, usbdev);
> +        break;
> +    default:
> +        rc = ERROR_FAIL;
> +        goto out;

This `default' case should do something more sensible than simply
returning ERROR_FAIL with no log message (for `add', too).

> +/* Operation to remove usb device.
> + *
> + * Generally, it does:
> + * 1) check if the usb device is assigned to the domain
> + * 2) remove the usb device from xenstore controller/port.
> + * 3) unbind usb device from usbback and rebind to its original driver.
> + *    If usb device has many interfaces, do it to each interface.
> + */
> +static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid,
> +                                    libxl_device_usb *usb)
> +{
> +    int rc;
> +
> +    if (usb->ctrl < 0 || usb->port < 1) {
> +        LOG(ERROR, "Invalid USB device");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    if (usb->devtype == LIBXL_USBDEV_TYPE_HOSTDEV &&
> +        (usb->u.hostdev.hostbus < 1 || usb->u.hostdev.hostaddr < 1)) {
> +        LOG(ERROR, "Invalid USB device of hostdev");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }

Why are these checks here rather than in do_usb_remove ?

For that matter, why is this function not the same as do_usb_remove ?

I might ask the same question about do_usb_add and
libxl__device_usb_add ?


> +void libxl_device_usbctrl_list_free(libxl_device_usbctrl *list, int nr)
> +{
> +   int i;
> +   for (i = 0; i < nr; i++)
> +       libxl_device_usbctrl_dispose(&list[i]);
> +   free(list);
> +}
> +
> +void libxl_device_usb_list_free(libxl_device_usb *list, int nr)
> +{
> +   int i;
> +   for (i = 0; i < nr; i++)
> +       libxl_device_usb_dispose(&list[i]);
> +   free(list);
> +}

It is a shame that these aren't autogenerated, but I won't ask you to
arrange that (as there are already too many instances of this
in-tree).


Regards,
Ian.

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