George Dunlap
2012-Nov-28 12:16 UTC
[PATCH 1 of 2] libxl: Allow multiple USB devices on HVM domain creation
# HG changeset patch
# User George Dunlap <george.dunlap@eu.citrix.com>
# Date 1354101445 0
# Node ID 538d9ffbd71b41e8cf6d7da0ded9e0a0b07f3c0d
# Parent ae6fb202b233af815466055d9f1a635802a50855
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 b_info->hvm.usbdevice is non-NULL, then it will be used exclusively;
otherwise, libxl will check for b_info->hvm.usbdevice_list instead.
Each device listed will cause an extra "-usbdevice [foo]" to be
appended
to the qemu command line.
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 present, the caller should use b_info->hvm.usbdevice_list; otherwise
they should use b_info->hvm.usbdevice.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
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,23 @@
#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, only
+ * hvm.usbdevice will be used.
+ *
+ * If this is not defined, callers should 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
@@ -191,6 +191,15 @@ static char ** libxl__build_device_model
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++) {
+ 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),
])),
("pv", Struct(None, [("kernel", string),
("slack_memkb", MemKB),
Ian Campbell
2012-Nov-29 11:16 UTC
Re: [PATCH 1 of 2] libxl: Allow multiple USB devices on HVM domain creation
On Wed, 2012-11-28 at 12:16 +0000, George Dunlap wrote:> # HG changeset patch > # User George Dunlap <george.dunlap@eu.citrix.com> > # Date 1354101445 0 > # Node ID 538d9ffbd71b41e8cf6d7da0ded9e0a0b07f3c0d > # Parent ae6fb202b233af815466055d9f1a635802a50855 > 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 b_info->hvm.usbdevice is non-NULL, then it will be used exclusively; > otherwise, libxl will check for b_info->hvm.usbdevice_list instead.Is this the right way round? If the caller knows about usbdevice_list enough to have set it to something then surely that''s the one it wanted?> Each device listed will cause an extra "-usbdevice [foo]" to be appended > to the qemu command line. > > 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 present, the caller should use b_info->hvm.usbdevice_list; otherwise > they should use b_info->hvm.usbdevice.Actually it''s a both stricter and looser than that, if LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST is defined then the caller can choose to use usbdevice_list but can also use usbdevice if they wish (but not both). If it is not present then they must not use usbdevice_list at all. I wonder if this LIBXL_HAVE should also be contained in a #ifdef LIBXL_API_VERSION >= 0x040300?> > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>> > 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,23 @@ > #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, only > + * hvm.usbdevice will be used. > + * > + * If this is not defined, callers should 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 > @@ -191,6 +191,15 @@ static char ** libxl__build_device_model > 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++) { > + 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), > ])), > ("pv", Struct(None, [("kernel", string), > ("slack_memkb", MemKB), > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
George Dunlap
2012-Nov-29 12:01 UTC
Re: [PATCH 1 of 2] libxl: Allow multiple USB devices on HVM domain creation
On 29/11/12 11:16, Ian Campbell wrote:> On Wed, 2012-11-28 at 12:16 +0000, George Dunlap wrote: >> # HG changeset patch >> # User George Dunlap <george.dunlap@eu.citrix.com> >> # Date 1354101445 0 >> # Node ID 538d9ffbd71b41e8cf6d7da0ded9e0a0b07f3c0d >> # Parent ae6fb202b233af815466055d9f1a635802a50855 >> 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 b_info->hvm.usbdevice is non-NULL, then it will be used exclusively; >> otherwise, libxl will check for b_info->hvm.usbdevice_list instead. > Is this the right way round? If the caller knows about usbdevice_list > enough to have set it to something then surely that''s the one it wanted?Well, I had considered returning an error if both were set. :-) I suppose actually that''s probably the way that is likely to flag up bugs in the toolstack the best -- only doing one or the other will cause weird buggy behavior that will be hard to track down. Does that sound good to you?>> Each device listed will cause an extra "-usbdevice [foo]" to be appended >> to the qemu command line. >> >> 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 present, the caller should use b_info->hvm.usbdevice_list; otherwise >> they should use b_info->hvm.usbdevice. > Actually it''s a both stricter and looser than that, if > LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST is defined then the caller can > choose to use usbdevice_list but can also use usbdevice if they wish > (but not both). If it is not present then they must not use > usbdevice_list at all.Hmm, I think I wrote this first and then changed it and forgot to update it. :-) I also forgot that in this kind of "API spec" document, "should" and "must" are technical terms with very specific definitions. I''ll revise it when I re-spin.> > I wonder if this LIBXL_HAVE should also be contained in a #ifdef > LIBXL_API_VERSION >= 0x040300?What would be the purpose of that? -George
Ian Campbell
2012-Nov-29 12:12 UTC
Re: [PATCH 1 of 2] libxl: Allow multiple USB devices on HVM domain creation
On Thu, 2012-11-29 at 12:01 +0000, George Dunlap wrote:> On 29/11/12 11:16, Ian Campbell wrote: > > On Wed, 2012-11-28 at 12:16 +0000, George Dunlap wrote: > >> # HG changeset patch > >> # User George Dunlap <george.dunlap@eu.citrix.com> > >> # Date 1354101445 0 > >> # Node ID 538d9ffbd71b41e8cf6d7da0ded9e0a0b07f3c0d > >> # Parent ae6fb202b233af815466055d9f1a635802a50855 > >> 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 b_info->hvm.usbdevice is non-NULL, then it will be used exclusively; > >> otherwise, libxl will check for b_info->hvm.usbdevice_list instead. > > Is this the right way round? If the caller knows about usbdevice_list > > enough to have set it to something then surely that''s the one it wanted? > > Well, I had considered returning an error if both were set. :-) I > suppose actually that''s probably the way that is likely to flag up bugs > in the toolstack the best -- only doing one or the other will cause > weird buggy behavior that will be hard to track down. Does that sound > good to you? > > >> Each device listed will cause an extra "-usbdevice [foo]" to be appended > >> to the qemu command line. > >> > >> 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 present, the caller should use b_info->hvm.usbdevice_list; otherwise > >> they should use b_info->hvm.usbdevice. > > Actually it''s a both stricter and looser than that, if > > LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST is defined then the caller can > > choose to use usbdevice_list but can also use usbdevice if they wish > > (but not both). If it is not present then they must not use > > usbdevice_list at all. > > Hmm, I think I wrote this first and then changed it and forgot to update > it. :-) I also forgot that in this kind of "API spec" document, > "should" and "must" are technical terms with very specific definitions. > I''ll revise it when I re-spin.I don''t think we actually use the RFC-whichever definitions of should and must in general (and they would be SHOULD and MUST anyhow), it''s probably me being picky.> > I wonder if this LIBXL_HAVE should also be contained in a #ifdef > > LIBXL_API_VERSION >= 0x040300? > > What would be the purpose of that?LIBXL_API_VERSION and LIBXL_HAVE_* are two different mechanisms for helping us to do API compatibility, I hadn''t properly thought about the interaction between them until now. Adding the ifdef would expose the new feature only to those claiming compatibility with the newer API. More generally the purpose if LIBXL_API_VERSION generally is to allow us to deprecate interfaces over time, which isn''t quite the case here I suppose. Ian.