Re: [PATCH V3 4/6] xl: add pvusb commands

[Juergen Gross <jgross@suse.com>]

On 04/19/2015 05:50 AM, Chunyan Liu wrote:
> Add pvusb commands: usb-ctrl-attach, usb-ctrl-detach, usb-list,
> usb-attach and usb-detach.
>
> To attach a usb device to guest through pvusb, one could follow
> following example:
>
>   #xl usb-ctrl-attach test_vm version=1 num_ports=8
>
>   #xl usb-list test_vm
>   will show the usb controllers and port usage under the domain.
>
>   #xl usb-attach test_vm 1.6
>   will find the first usable controller:port, and attach usb
>   device whose bus address is 1.6 (busnum is 1, devnum is 6)
>   to it. One could also specify which <controller> and which <port>.
>
>   #xl usb-detach test_vm 1.6
>
>   #xl usb-ctrl-detach test_vm dev_id
>   will destroy the controller with specified dev_id. Dev_id
>   can be traced in usb-list info.
>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>
> ---
> Changes to v2:
>    * use bus.addr as user interface instead of busid in usb-attach|detach
>    * remove usb-assignable-list interface

Why? While lsusb in combination with xl usb-list for each domain will
give the same information, having to iterate through all domains can be
quite annoying.

An alternative would be to accept omitting the domain for xl usb-list
and list all domains with assigned usb devices in this case.

>    * add documentation
>
>   docs/man/xl.pod.1         |  38 ++++++++
>   tools/libxl/xl.h          |   5 +
>   tools/libxl/xl_cmdimpl.c  | 238 ++++++++++++++++++++++++++++++++++++++++++++++
>   tools/libxl/xl_cmdtable.c |  25 +++++
>   4 files changed, 306 insertions(+)
>
...
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3196,6 +3196,244 @@ int main_cd_insert(int argc, char **argv)
>       return 0;
>   }
>
> +static void usbinfo_print(libxl_device_usb *usbs, int num) {
> +    int i;
> +    if ( usbs == NULL )

Coding style.

> +         return;
> +    for (i = 0; i < num; i++) {
> +        libxl_usbinfo usbinfo;
> +        libxl_usbinfo_init(&usbinfo);
> +
> +        if (usbs[i].port )

Coding style.

> +            printf("  Port %d:", usbs[i].port);
> +        if (!libxl_device_usb_getinfo(ctx, usbs[i].busid, &usbinfo)) {
> +            printf(" Bus %03x Device %03x: ID %04x:%04x %s %s\n",
> +                    usbinfo.busnum, usbinfo.devnum, usbinfo.idVendor,
> +                    usbinfo.idProduct, usbinfo.manuf, usbinfo.prod);
> +        }
> +        libxl_usbinfo_dispose(&usbinfo);
> +    }
> +}
> +
> +int main_usbctrl_attach(int argc, char **argv)
> +{
> +    uint32_t domid;
> +    int opt;
> +    char *oparg;
> +    libxl_device_usbctrl usbctrl;
> +
> +    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-ctrl-attach", 1) {
> +        /* No options */
> +    }
> +
> +    domid = find_domain(argv[optind++]);
> +
> +    libxl_device_usbctrl_init(&usbctrl);
> +
> +    while (argc > optind) {
> +        if (MATCH_OPTION("version", argv[optind], oparg)) {
> +            usbctrl.version = atoi(oparg);
> +        } else if (MATCH_OPTION("ports", argv[optind], oparg)) {
> +            usbctrl.ports = atoi(oparg);
> +        } else {
> +            fprintf(stderr, "unrecognized argument `%s'\n", argv[optind]);
> +            exit(-1);

I don't think this is the preferred way of error handling.
Returning with an appropriate error code would be better.

Same applies to all other uses of exit() below.

> +        }
> +        optind++;
> +    }
> +
> +    if (dryrun_only) {
> +       char* json = libxl_device_usbctrl_to_json(ctx, &usbctrl);
> +       printf("usb controller: %s\n", json);
> +       free(json);
> +       libxl_device_usbctrl_dispose(&usbctrl);
> +       if (ferror(stdout) || fflush(stdout)) {
> +           perror("stdout");
> +           exit(-1);
> +       }
> +       return 0;
> +    }
> +
> +    if (libxl_device_usbctrl_add(ctx, domid, &usbctrl, 0)) {
> +        fprintf(stderr, "libxl_device_usbctrl_add failed.\n");
> +        exit(-1);
> +    }
> +    libxl_device_usbctrl_dispose(&usbctrl);
> +    return 0;
> +}
> +
> +int main_usbctrl_detach(int argc, char **argv)
> +{
> +    uint32_t domid;
> +    int opt;
> +    libxl_device_usbctrl usbctrl;
> +
> +    SWITCH_FOREACH_OPT(opt, "", NULL, "usb-ctrl-detach", 2) {
> +        /* No options */
> +    }
> +
> +    domid = find_domain(argv[optind]);
> +
> +    libxl_device_usbctrl_init(&usbctrl);
> +    usbctrl.devid = atoi(argv[optind+1]);
> +
> +    if(libxl_device_usbctrl_remove(ctx, domid, &usbctrl, 0)) {

Coding style.

> +        fprintf(stderr, "libxl_device_usbctrl_add failed.\n");
> +        exit(-1);
> +    }
> +    libxl_device_usbctrl_dispose(&usbctrl);
> +    return 0;
> +
> +}

Juergen