On Tue, Nov 10, 2015 at 8:41 AM, Chun Yan Liu <cy...@suse.com> 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
From 92a4725d9f669cbd63ec19d85424f47fa4eb96b6 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dun...@eu.citrix.com>
Date: Tue, 10 Nov 2015 17:22:39 +0000
Subject: [PATCH] tools: Don't use usb hostbus and hostaddr during removal
The busid is already stored at a xenstore location we can find with
<ctrl,port>. Read this value before removing the entry; then use it
to re-assign the host device back to the original drivers.
Change usb_get_all_interfaces() to take busid, rather than looking for
it itself. In the remove case, use the busid value we got from the
backend; in the add() case, just reorder the calls so that
busaddr_to_busid happens first. (This actually avoids a duplicate
busaddr_to_busid conversion in the add case.)
Signed-off-by: George Dunlap <george.dun...@citrix.com>
---
tools/libxl/libxl_pvusb.c | 57 +++++++++++++++++++++++------------------------
1 file changed, 28 insertions(+), 29 deletions(-)
diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c
index ab57371..ead3e50 100644
--- a/tools/libxl/libxl_pvusb.c
+++ b/tools/libxl/libxl_pvusb.c
@@ -971,6 +971,15 @@ static int libxl__device_usb_remove_xenstore(libxl__gc *gc, uint32_t domid,
return 0;
}
+static char * usbback_busid_from_ctrlport(libxl__gc *gc, uint32_t domid,
+ libxl_device_usb *usb)
+{
+ return libxl__xs_read(gc, XBT_NULL,
+ GCSPRINTF("%s/backend/vusb/%d/%d/port/%d",
+ libxl__xs_get_dompath(gc, LIBXL_TOOLSTACK_DOMID),
+ domid, usb->ctrl, usb->port));
+}
+
/* bind/unbind usb device interface */
static int unbind_usb_intf(libxl__gc *gc, char *intf, char **drvpath)
{
@@ -1044,25 +1053,17 @@ static int usb_intf_is_assigned(libxl__gc *gc, char *intf)
return -1;
}
-static int usb_get_all_interfaces(libxl__gc *gc, libxl_device_usb *usb,
+static int usb_get_all_interfaces(libxl__gc *gc, char *busid,
char ***intfs, int *num)
{
DIR *dir;
struct dirent *entry;
char *buf;
- char *busid;
int rc;
*intfs = NULL;
*num = 0;
- busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
- usb->u.hostdev.hostaddr);
- if (!busid) {
- rc = ERROR_FAIL;
- goto out;
- }
-
buf = GCSPRINTF("%s:", busid);
if (!(dir = opendir(SYSFS_USB_DEV))) {
@@ -1114,20 +1115,17 @@ static char *usb_interface_xenstore_encode(char *busid)
* If there are many interfaces under USB device, check each interface,
* unbind from "usbback" driver and rebind to its original driver.
*/
-static int usbback_dev_unassign(libxl__gc *gc, libxl_device_usb *usb)
+static int usbback_dev_unassign(libxl__gc *gc, char *busid)
{
char **intfs = NULL;
char *path;
int num = 0, i;
int rc;
- char *busid;
char *usb_encode = NULL;
- if (usb_get_all_interfaces(gc, usb, &intfs, &num) < 0)
+ if (usb_get_all_interfaces(gc, busid, &intfs, &num) < 0)
return ERROR_FAIL;
- busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
- usb->u.hostdev.hostaddr);
usb_encode = usb_interface_xenstore_encode(busid);
for (i = 0; i < num; i++) {
@@ -1174,11 +1172,15 @@ static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb)
char *busid;
char *usb_encode = NULL;
- if (usb_get_all_interfaces(gc, usb, &intfs, &num) < 0)
- return ERROR_FAIL;
-
busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus,
usb->u.hostdev.hostaddr);
+
+ if (!busid)
+ return ERROR_FAIL;
+
+ if (usb_get_all_interfaces(gc, busid, &intfs, &num) < 0)
+ return ERROR_FAIL;
+
usb_encode = usb_interface_xenstore_encode(busid);
for (i = 0; i < num; i++) {
@@ -1200,9 +1202,8 @@ static int usbback_dev_assign(libxl__gc *gc, libxl_device_usb *usb)
/* write driver path to xenstore for later rebinding */
path = GCSPRINTF(USBBACK_INFO_PATH"/%s/%s/driver_path",
usb_encode, usb_interface_xenstore_encode(intf));
- if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0) {
- LOG(WARN, "Write of %s to node %s failed", drvpath, path);
- }
+ if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0)
+ goto out_rebind;
}
/* bind interface to usbback */
@@ -1220,7 +1221,7 @@ out_rebind:
/* some interfaces might be bound to usbback, unbind it then and
* rebind to its original driver
*/
- usbback_dev_unassign(gc, usb);
+ usbback_dev_unassign(gc, busid);
out:
free(usb_encode);
return rc;
@@ -1376,6 +1377,7 @@ static int do_usb_remove(libxl__gc *gc, uint32_t domid,
libxl_usbctrlinfo usbctrlinfo;
libxl_device_usbctrl usbctrl;
int rc;
+ char *busid;
libxl_device_usbctrl_init(&usbctrl);
libxl_usbctrlinfo_init(&usbctrlinfo);
@@ -1395,10 +1397,14 @@ static int do_usb_remove(libxl__gc *gc, uint32_t domid,
LOG(ERROR, "Not supported");
break;
case LIBXL_USBCTRL_TYPE_PV:
+ busid = usbback_busid_from_ctrlport(gc, domid, usbdev);
+
+ if (!busid) goto out;
+
rc = libxl__device_usb_remove_xenstore(gc, domid, usbdev);
if (rc) goto out;
- usbback_dev_unassign(gc, usbdev);
+ usbback_dev_unassign(gc, busid);
break;
default:
rc = ERROR_FAIL;
@@ -1432,13 +1438,6 @@ static int libxl__device_usb_remove(libxl__gc *gc, uint32_t domid,
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;
- }
-
/* Do the remove */
rc = do_usb_remove(gc, domid, usb);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
