Re: [PATCH V7 7/7] domcreate: support pvusb in configuration file

[George Dunlap <george.dunlap@citrix.com>]

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