Xen project Mailing List

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

On Tue, Nov 10, 2015 at 8:41 AM, Chun Yan Liu <cyliu@xxxxxxxx> wrote: >> > +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: > > Reading the codes, libxl__wait_device_connection will call aodev->callback > properly. So here, only if (rc != 0), that means error happens, then we need > to > call aodev->callback to end the process. (Refer to current > libxl__device_disk_add, > all current code does similar work.) So I think the code is not wrong (?) >> > +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 ? > > Which cast? Here, we want to get a uint_8 value, but buf is a string, > we need to do atoi. atoi() isn't a cast, it's a function call. The cast is the (char *) bit. I think probably you could get away with declaring buf as (char *). &buf should be converted to void** automatically without any warnings, I think. >> > +/* 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. > > The busid is in syntax like 3-1:2.1. It does contain '-'. But since we only > use > the single direction, never decode back, so it won't harm. So the only potential problem is that 3-1:2, 3-1-2, 3:1-2, and 3:1:2 all collapse down to 3-1-2. Is there any risk of something like that happening? (Ian: NB these are all read from sysfs.) Since this isn't really being read by anyone, maybe it would be better to replace ':' with another character, just to be safe. It could even be something like 'c' if no other punctuation is available. >> > +/* 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. Ian, I don't understand what you're saying here. The code I'm looking at does: 1. unbind + get old driver path in drvpath 2. if(drvpath) write to xenstore 3. bind to usbback If the bind fails, then we do have drvpath in xenstore. Or am I missing something? Bailing out if the above xenstore write fails seems sensible though. >> > +/* 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 ? > > Using libxl__device_usb_add and within it extracting do_usb_add is to: > * doing some initialization work and necessary check work in > libxl__device_usb_add (this part are common to PVUSB and QEMU USB) > * doing actual adding usb operations in do_usb_add (this part will diverge > for PVUSB and QEMU USB) . > > Same to libxl__device_usb_remove and do_usb_remove. > > Certainly, we could merge into one function if that is better. George may > have some opinions on this? I think it would be nice to have things broken down that way, but it's likely I'll have to do some adjustments when I come to add in devicemodel usb anyway. So making it one big function wouldn't hurt. (Nor would leaving it the way it is, as far as I'm concerned.) The big thing though is with the removal interface here, where we again have somewhat of a confusion between naming things based on the *host* name (as we do in pci pass-through) vs the *guest* names (as we do for disks and nics). If you have the controller and virtual port, there's no need for the caller to *also* go and look up the host bus and host address; or vice versa -- if you have the hostbus and hostaddr, you shouldn't also need to go look up the usbctrl and virtual port. It looks like for libxl_device_{disk,nic,vtpm}_remove(), it gets a libxl_device struct using libxl__device_from_{disk,nic,vtpm}; and in all cases, the information used to construct the "device" is the backend + either a devid (for nic and vtpm) or vdev, for disks. It looks like for those devices, any conversion from other labels (mac address for nic, uuid for vtpm) is done in xl_cmdimpli.c. I think we want to follow suit here -- libxl_device_usb_remove() should take a usbctrl and port number only, and should look up whatever information it needs to do the removal from that. All we really need for the re-assignment is the busid, which is already stored in a xenstore location we can find using domid, ctrl, and port. Something like the attached patch (compile-tested only). What do you think? (NB the patch doesn't fix the gc or aliasing issues in usb_interface_xenstore_encode() -- those still need to be addressed.) -George

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

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.