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

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

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
> 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