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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

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

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

> +static int usb_add(uint32_t domid, libxl_device_usb_type type,
> +                            const char * device)
                      ^
This indent is odd.

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

Ian.

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