From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v5 2/2] xl: Add commands for usb hot-plug Date: Fri, 19 Apr 2013 11:02:14 +0100 Message-ID: <51711626.3030702@eu.citrix.com> References: <1366296408-5660-1-git-send-email-george.dunlap@eu.citrix.com> <1366296408-5660-2-git-send-email-george.dunlap@eu.citrix.com> <20848.12375.807996.494297@mariner.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20848.12375.807996.494297@mariner.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Jackson Cc: "sstanisi@cbnco.com" , Roger Pau Monne , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org 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 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 > these... 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. :-) -George