[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V8 3/7] libxl: add pvusb API
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |