Re: [Xen-devel] [PATCH 3 of 4 v2] libxl: Allow multiple USB devices on HVM domain creation

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2012-11-30 at 16:24 +0000, George Dunlap wrote:
> # HG changeset patch
> # User George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> # Date 1354291988 0
> # Node ID af47b01d2dbe0e916e1014a264845c48d6ef8108
> # Parent  fa37bd276212a01e3c898b54a7f2385454c406a7
> libxl: Allow multiple USB devices on HVM domain creation
> 
> This patch allows an HVM domain to be created with multiple USB
> devices.
> 
> Since the previous interface only allowed the passing of a single
> device, this requires us to add a new element to the hvm struct of
> libxl_domain_build_info -- usbdevice_list.  For API compatibility, the
> old element, usbdevice, remains.
> 
> If hvm.usbdevice_list is set, each device listed will cause an extra
> "-usbdevice [foo]" to be appended to the qemu command line.
> 
> Callers may set either hvm.usbdevice or hvm.usbdevice_list, but not
> both; libxl will throw an error if both are set.
> 
> In order to allow users of libxl to write software compatible with
> older versions of libxl, also define LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST.
> If this is defined, callers may use either hvm.usbdevice or
> hvm.usbdevice_list; otherwise, only hvm.usbdevice will be available.
> 
> v2:
>  - Throw an error if both usbdevice and usbdevice_list are set
>  - Update and clarify definition based on feedback
>  - Previous patches means this works for both traditional and upstream

Is this a requirement? Did this work with qemu-xen-trad with xend?

Unless it did I'm not especially in favour of adding new features to
qemu-xen-trad just because it looks easy to do so.

> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -266,6 +266,22 @@
>  #endif
>  #endif
>  
> +/* 
> + * LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
> + * 
> + * If this is defined, then the libxl_domain_build_info structure will
> + * contain hvm.usbdevice_list, a libxl_string_list type that contains
> + * a list of USB devices to specify on the qemu command-line.
> + *
> + * If it is set, callers may use either hvm.usbdevice or
> + * hvm.usbdevice_list, but not both; if both are set, libxl will
> + * throw an error.
> + *
> + * If this is not defined, callers can only use hvm.usbdevice.  Note
> + * that this means only one device can be added at domain build time.
> + */
> +#define LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST 1
> +
>  /* Functions annotated with LIBXL_EXTERNAL_CALLERS_ONLY may not be
>   * called from within libxl itself. Callers outside libxl, who
>   * do not #include libxl_internal.h, are fine. */
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -322,11 +322,28 @@ static char ** libxl__build_device_model
>  
>          }
>  
> -        if (libxl_defbool_val(b_info->u.hvm.usb) || b_info->u.hvm.usbdevice) 
> {
> +        if (libxl_defbool_val(b_info->u.hvm.usb)
> +            || b_info->u.hvm.usbdevice
> +            || b_info->u.hvm.usbdevice_list) {
> +            if ( b_info->u.hvm.usbdevice && b_info->u.hvm.usbdevice_list )
> +            {
> +                LOG(ERROR, "%s: Both usbdevice and usbdevice_list set",
> +                    __func__);
> +                return NULL;
> +            }
>              flexarray_append(dm_args, "-usb");
>              if (b_info->u.hvm.usbdevice) {
>                  flexarray_vappend(dm_args,
>                                    "-usbdevice", b_info->u.hvm.usbdevice, 
> NULL);
> +            } else if (b_info->u.hvm.usbdevice_list) {
> +                char **p;
> +                for (p = b_info->u.hvm.usbdevice_list;
> +                     *p;
> +                     p++) {

Is the line too long if you unfold this? It doesn't look to me like it
should be.

> +                    flexarray_vappend(dm_args,
> +                                      "-usbdevice",
> +                                      *p, NULL);
> +                }
>              }
>          }
>          if (b_info->u.hvm.soundhw) {
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -326,6 +326,7 @@ libxl_domain_build_info = Struct("domain
>                                         ("usbdevice",        string),
>                                         ("soundhw",          string),
>                                         ("xen_platform_pci", libxl_defbool),
> +                                       ("usbdevice_list",   
> libxl_string_list),

Is there any chance we might want something more structured that a
string in the interface in the future? e.g. libxl_device_usb?

>                                         ])),
>                   ("pv", Struct(None, [("kernel", string),
>                                        ("slack_memkb", MemKB),
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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