[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v5 2/2] xl: Add commands for usb hot-plug

On 18/04/13 18:41, Ian Jackson wrote:
George Dunlap writes ("[PATCH v5 2/2] xl: Add commands for usb hot-plug"):
+=item B<usb-add> I<-d domain-id> I<-v hosbus.hostaddr>
Shouldn't these be more like `xl {disk,network}-{attach,detach}' ?

I was imagining something like
    xl usb-attach debian.guest.osstest host:046d:c014
?  (Perhaps with
    xl usb-remove --protocol hvm debian.guest.osstest tablet
coming later at some point.)

I'm not really a fan of the "use position to specify arguments" when there is more than one or two arguments; but that's what block/net/pci do, so we should probably follow suit.

Hmm, a bit strange that the libxl functions are "libxl_device_[foo]_add()", but the xl commands are "[foo]-attach". Ah well -- I'll change that around too.

One of the reasons I didn't do this was because I didn't want to have to implement a parser to figure out which kind of device to implement. But an advantage of doing that is that we could at some point move from the sort of hacky, qemu-specific way of specifying usb devices for HVM guests in the config file (which basically passes them on directly to the qemu command-line via a deprecated interface), and move to a unified way to specify USB devices for either PV or HVM (with, for example, hot-unplug of devices which were added at boot time).

Let me take a look and see if I can steal a parsing function from someone else. Any recommendations? :-)

+static int parse_usb_hostdev_specifier(libxl_device_usb *dev, const char *s)
+    const char * hostbus, *hostaddr, *p;
+    hostbus = s;
+    hostaddr=NULL;
+    /* Match [0-9]+\.[0-9] */
+    if (!CTYPE(isdigit,*hostbus))
+        return -1;
+    for(p=s; *p; p++) {
+        if(*p == '.') {
+            if ( !hostaddr )
+                hostaddr = p+1;
+            else {
+                return -1;
+            }
+        } else if (!CTYPE(isdigit,*p)) {
+            return -1;
+        }
+    }
+    if (!hostaddr || !CTYPE(isdigit,*hostaddr))
+        return -1;
+    dev->u.hostdev.hostbus  = strtoul(hostbus,  NULL, 10);
+    dev->u.hostdev.hostaddr = strtoul(hostaddr, NULL, 10);
Perhaps it would be easier to use the 2nd argument to strtoul ?
Or strchr ?

I'll see about that when looking at the parsing functions.

Also, some style quibbles: spaces should appear after the keyword in
"for (" and "if (" and not inside the parens in "if (!hostaddr)".  And
it seems odd to write an if without braces for the one-line if branch
and with braces for the one-line else branch.  You have quite a lot of

Ack. Sorry, for some reason didn't go through and check for stylistic things in this patch.

+static int usb_add(uint32_t domid, libxl_device_usb_type type,
+                            const char * device)
This indent is odd.

Ack. Didn't re-indent after removing "hvm_host_" from the function name.

+    if ( (rc = libxl_device_usb_add(ctx, domid, &usbdev, NULL)) < 0 )
+        fprintf(stderr, "libxl_device_usb_add failed.\n");
Can't we have the call and the assignment to rc on its own line ?

If you like. :-)


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.