From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH V7 7/7] domcreate: support pvusb in configuration file Date: Wed, 7 Oct 2015 16:06:32 +0100 Message-ID: <561534F8.1090603@citrix.com> References: <1443147102-6471-1-git-send-email-cyliu@suse.com> <1443147102-6471-8-git-send-email-cyliu@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1443147102-6471-8-git-send-email-cyliu@suse.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: Chunyan Liu , xen-devel@lists.xen.org Cc: jgross@suse.com, wei.liu2@citrix.com, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, jfehlig@suse.com, Simon Cao List-Id: xen-devel@lists.xenproject.org On 25/09/15 03:11, Chunyan Liu wrote: > Add code to support pvusb in domain config file. One could specify > usbctrl and usb in domain's configuration file and create domain, > then usb controllers will be created and usb device would be attached > to guest automatically. > > One could specify usb controllers and usb devices in config file > like this: > usbctrl=['version=2,ports=4', 'version=1, ports=4', ] > usbdev=['2.1,controller=0,port=1', ] I realize you're patterning this after pci, but this syntax assumes that you're always going to want to pass in a host device, which I think we don't want to do. In particular, we want to leave the door open in the future to different ways of being able to specify a particular device based on the pci + usb topology which will be invariant over reboots. So I think all parameters should have a name. Two questions; first, the name of the parameter specifying hostbus and hostaddr. 1. "hostspec=", which will automatically use 2.1 for bus.addr, and could in the future be extended to use 2-1 for bus-port, xxxx:yyyy for vendorid:productid, and potentially the udev "pci + usb topology" thing. 2. hostbus=x, hostaddr=y. This is what qemu and libvirt do. It also leaves open the door for adding double-checks or other constraints: e.g., hostbus=x, hostaddr=y, hostvid=m, hostpid=n would only assign x.y if vendorid:productid match m:n I'd go for #2. We should also have a "type" field, of which one option should be "hostdev". I think it's probably OK to assume "hostdev" if we have hostdev-only parameters. One more comment... > /* First layer; wraps libxl__spawn_spawn. */ > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 66d8f8c..c64e445 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -1253,6 +1253,79 @@ static void parse_vnuma_config(const XLU_Config *config, > free(vcpu_parsed); > } > > +static void parse_usbctrl_config(libxl_device_usbctrl *usbctrl, > + const char *buf) > +{ > + char *buf2 = strdup(buf); > + char *p, *p2; > + > + p = strtok(buf2, ","); > + if (!p) > + goto out; > + do { > + while (*p == ' ') > + p++; > + if ((p2 = strchr(p, '=')) == NULL) > + break; > + *p2 = '\0'; > + if (!strcmp(p, "type")) { > + if (!strcmp(p2 + 1, "pv")) { I'd probably do "*p2='\0'; p2++" so that then later you can just use p2 as a normal string, rather than p2+1 as you do here. But what I'd *really* do is copy the parse_nic_config() code, and use MATCH_STRING("type", ...). And as Ian said, I'd move these parsing functions into the previous patch, and then use them to parse the command-line arguments (again modelled after main_networkattach()). (I'm not confident in the multidev stuff enough to give a proper review.) -George