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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel