[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V12 3/5] libxl: add pvusb API
>>> On 1/13/2016 at 02:31 AM, in message <22165.18064.452737.543799@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote: > Chunyan Liu writes ("[PATCH V12 3/5] 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 > > > > Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx> > > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx> > > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx> > > Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx> > > Thanks. I have re-reviewed this and found a few issues, I'm afraid, > mostly in the error handling. > > > +/* find first unused controller:port and give that to usb device */ > > +static int > > +libxl__device_usbdev_set_default_usbctrl(libxl__gc *gc, uint32_t domid, > > + libxl_device_usbdev *usbdev) > > +{ > ... > > + path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d", > > + libxl__xs_get_dompath(gc, > LIBXL_TOOLSTACK_DOMID), > > + domid, usbctrls[i].devid, j + 1); > > + tmp = libxl__xs_read(gc, XBT_NULL, path); > > I think this probably ought to be libxl__xs_read_checked ? (With > corresponding change to handling of return value.) OK. > > > +/* get original driver path of usb interface, stored in @drvpath */ > > +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char > **drvpath) > > +{ > > + char *spath, *dp = NULL; > > + struct stat st; > > + int rc; > > + > > + spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf); > > + > > + rc = lstat(spath, &st); > > + if (rc == 0) { > > + /* Find the canonical path to the driver. */ > > + dp = libxl__zalloc(gc, PATH_MAX); > > + dp = realpath(spath, dp); > > This call to realpath seems to lack error checking/handling. Will update. > > > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char > *path) > > +{ > > What is `intf' here ? Maybe `interface' ? But there is nothing usb > specific about this function. Looking for similar code elsewhere this > function seems very similar to libxl_pci.c:sysfs_write_bdf, but I > won't ask you to try to refactor these two functions together. Yes, it means 'interface'. It is referring to old xend/xm naming. For example, a USB device in Linux sysfs is 3-11, under it, there might be 3-11:1.0 and 3-11:1.1 (these are two interfaces). To assign this USB device to guest, both 3-11:1.0 and 3-11:1.1 should be unbind from original driver and bind to usbback. > > > + rc = write(fd, intf, strlen(intf)); > > rc ought not be used for a syscall return value. (CODING_STYLE) Will update. > > > + close(fd); > > This is a pretty ad-hoc error handling approach. Can you please use > the CODYING_STYLE-recommended pattern ? Will update. > > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char > *drvpath) > > +{ > > + char *path; > > + struct stat st; > > + > > + path = GCSPRINTF("%s/%s", drvpath, intf); > > + /* if already bound, return */ > > + if (!lstat(path, &st)) > > + return 0; > > If lstat fails, you need to check the error return to see why. Will update. > > > +/* 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. > > + */ > > +static char *usb_interface_xenstore_encode(libxl__gc *gc, const char > *busid) > > +{ > > + char *str = libxl__strdup(gc, busid); > > + int i, len = strlen(str); > > + > > + for (i = 0; i < len; i++) { > > + if (str[i] == '.') > > + str[i] = '_'; > > + if (str[i] == ':') > > + str[i] = '@'; > > I know I'm late with this comment and it's trivial and my > comaintainers may disagree, but I would find this > > + if (str[i] == '.') str[i] = '_'; > + if (str[i] == ':') str[i] = '@'; > > clearer because the layout reflects the regular structure of the code. > But please don't change this until another maintainer has commented > and said whether they agree with me. Certainly this is observation of > mine does not block this patch. > > > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid) > > +{ > > + char **intfs = NULL; > > + char *usbdev_encode = NULL; > > + char *path = NULL; > > + int i, num = 0; > > + int rc; > > + > > + if (usbdev_get_all_interfaces(gc, busid, &intfs, &num) < 0) > > + return ERROR_FAIL; > > usbdev_get_all_interfaces returns a libxl error code, which you should > preserved. So assign the result to rc, and `if (rc) goto out;'. Same > goes for unbind_usbintf (and maybe other calls elsewhere in this > file). Will update. > > > + path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", > > + usbdev_encode, usbintf_encode); > > + rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); > > + if (rc) continue; > > This anomalous error handling deserves a comment I think. (See also > below.) > > > + if (drvpath && bind_usbintf(gc, intf, drvpath)) > > + LOGE(WARN, "Couldn't rebind %s to %s", intf, drvpath); > > + } > > + > > + /* finally, remove xenstore driver path */ > > + path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); > > + libxl__xs_rm_checked(gc, XBT_NULL, path); > > If you are deliberately throwing away errors (which I think maybe you > are), please say so in a comment. Got it. > > Ought this function to really report success if these calls fail ? I think so. Till here, the USB device has already been unbound from usbback and removed from xenstore. usb-list cannot list it any more. > > > +/* 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, const char *busid) > > +{ > > Many calls here throw away the rc and invent ERROR_FAIL instead. > > And this one: > > > + if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0) > > + goto out; > > fails to set rc at all. Thanks. Will update. > > > +/* AO operation to add a usb device. > > + * > > + * Generally, it does: > > + * 1) check if the usb device type is assignable > > + * 2) check if the usb device is already assigned to a domain > > + * 3) add 'busid' of the usb device to xenstore contoller/port/. > > + * (PVUSB driver watches the xenstore changes and will detect that.) > > + * 4) unbind usb device from original driver and bind to usbback. > > + * If usb device has many interfaces, then: > > + * - unbind each interface from its original driver and bind to > > usbback. > > + * - store the original driver to xenstore for later rebinding when > > + * detaching the device. > > + * > > + * Before calling this function, aodev should be properly filled: > > + * aodev->ao, aodev->callback, aodev->update_json, ... > > + */ > > +void libxl__device_usbdev_add(libxl__egc *egc, uint32_t domid, > > + libxl_device_usbdev *usbdev, > > + libxl__ao_device *aodev) > > +{ > ... > > + if (is_usbdev_in_array(assigned, num_assigned, usbdev)) { > > + LOG(ERROR, "USB device already attached to a domain"); > > + rc = ERROR_FAIL; > > Maybe this should be ERROR_INVAL. OK. Will update. - Chunyan > > Thanks, > Ian. > > _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |