After 899433f149d libxl needs to know the content of d_config to
determine which QEMU is used. The code is changed such that
libxl__domain_set_device_model needs to be called before
libxl__domain_build_info_setdefault.
This is fine for libxl code, but it is problematic for
libxl_domain_need_memory, which is the only public API that takes a
build_info. To avoid break its users, provide a compatibility setting
inside that function.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Olaf Hering <olaf@aepfle.de>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Juergen Gross <jgross@suse.com>
Please test.
---
tools/libxl/libxl_mem.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index 448a2af8fd..fe1f9c2ff8 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -457,6 +457,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx,
libxl_domain_build_info_init(b_info);
libxl_domain_build_info_copy(ctx, b_info, b_info_in);
+ /*
+ * It has become a requirement that to figure out which QEMU to
+ * use, libxl will need to peek d_config's content. The code has
+ * been changed such that one will need to call
+ * libxl__domain_set_device_model before calling
+ * libxl__domain_build_info_setdefault inside libxl.
+ *
+ * This (problematic) public API is the only one which takes a
+ * b_info, imposing the same requirement on the users of this
+ * public API will break them.
+ *
+ * Provide a compatibility setting for this function. The
+ * calculation doesn't really care which QEMU is set here, so we
+ * go with the upstream default.
+ */
+ if (!b_info->device_model_version)
+ b_info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+
rc = libxl__domain_build_info_setdefault(gc, b_info);
if (rc) goto out;
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Wei Liu writes ("[PATCH] libxl: fix libxl_domain_need_memory after 899433f149d"): > After 899433f149d libxl needs to know the content of d_config to > determine which QEMU is used. The code is changed such that > libxl__domain_set_device_model needs to be called before > libxl__domain_build_info_setdefault. > > This is fine for libxl code, but it is problematic for > libxl_domain_need_memory, which is the only public API that takes a > build_info. To avoid break its users, provide a compatibility setting > inside that function. This looks plausible to me. I see that this function already makes a copy of the b_info, so we do not end up accidentally leaking this new default setting to the caller. Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20/05/2019 14:50, Ian Jackson wrote: > Wei Liu writes ("[PATCH] libxl: fix libxl_domain_need_memory after 899433f149d"): >> After 899433f149d libxl needs to know the content of d_config to >> determine which QEMU is used. The code is changed such that >> libxl__domain_set_device_model needs to be called before >> libxl__domain_build_info_setdefault. >> >> This is fine for libxl code, but it is problematic for >> libxl_domain_need_memory, which is the only public API that takes a >> build_info. To avoid break its users, provide a compatibility setting >> inside that function. > > This looks plausible to me. I see that this function already makes a > copy of the b_info, so we do not end up accidentally leaking this new > default setting to the caller. > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> > Thanks. I have fixed up the issues Roger and Andy found and pushed this patch. Wei. > Thanks, > Ian. > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Fri, May 17, 2019 at 06:05:55PM +0100, Wei Liu wrote: > After 899433f149d libxl needs to know the content of d_config to > determine which QEMU is used. The code is changed such that > libxl__domain_set_device_model needs to be called before > libxl__domain_build_info_setdefault. > > This is fine for libxl code, but it is problematic for > libxl_domain_need_memory, which is the only public API that takes a > build_info. To avoid break its users, provide a compatibility setting > inside that function. > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> With one nit below. > --- > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Olaf Hering <olaf@aepfle.de> > Cc: Roger Pau Monné <roger.pau@citrix.com> > Cc: Juergen Gross <jgross@suse.com> > > Please test. > --- > tools/libxl/libxl_mem.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c > index 448a2af8fd..fe1f9c2ff8 100644 > --- a/tools/libxl/libxl_mem.c > +++ b/tools/libxl/libxl_mem.c > @@ -457,6 +457,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, > libxl_domain_build_info_init(b_info); > libxl_domain_build_info_copy(ctx, b_info, b_info_in); > > + /* > + * It has become a requirement that to figure out which QEMU to > + * use, libxl will need to peek d_config's content. The code has > + * been changed such that one will need to call > + * libxl__domain_set_device_model before calling > + * libxl__domain_build_info_setdefault inside libxl. > + * > + * This (problematic) public API is the only one which takes a > + * b_info, imposing the same requirement on the users of this > + * public API will break them. > + * > + * Provide a compatibility setting for this function. The > + * calculation doesn't really care which QEMU is set here, so we > + * go with the upstream default. > + */ > + if (!b_info->device_model_version) > + b_info->device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; There's a hard tab in the line above. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20/05/2019 10:14, Roger Pau Monné wrote: > >> diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c >> index 448a2af8fd..fe1f9c2ff8 100644 >> --- a/tools/libxl/libxl_mem.c >> +++ b/tools/libxl/libxl_mem.c >> @@ -457,6 +457,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, >> libxl_domain_build_info_init(b_info); >> libxl_domain_build_info_copy(ctx, b_info, b_info_in); >> >> + /* >> + * It has become a requirement that to figure out which QEMU to >> + * use, libxl will need to peek d_config's content. The code has >> + * been changed such that one will need to call >> + * libxl__domain_set_device_model before calling >> + * libxl__domain_build_info_setdefault inside libxl. >> + * >> + * This (problematic) public API is the only one which takes a >> + * b_info, imposing the same requirement on the users of this >> + * public API will break them. This sentence doesn't quite parse. You either want "b_info. Imposing", or something like "b_info, and imposing". ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
On Mon, May 20, 2019 at 10:19:26AM +0100, Andrew Cooper wrote: > On 20/05/2019 10:14, Roger Pau Monné wrote: > > > >> diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c > >> index 448a2af8fd..fe1f9c2ff8 100644 > >> --- a/tools/libxl/libxl_mem.c > >> +++ b/tools/libxl/libxl_mem.c > >> @@ -457,6 +457,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, > >> libxl_domain_build_info_init(b_info); > >> libxl_domain_build_info_copy(ctx, b_info, b_info_in); > >> > >> + /* > >> + * It has become a requirement that to figure out which QEMU to > >> + * use, libxl will need to peek d_config's content. The code has > >> + * been changed such that one will need to call > >> + * libxl__domain_set_device_model before calling > >> + * libxl__domain_build_info_setdefault inside libxl. > >> + * > >> + * This (problematic) public API is the only one which takes a > >> + * b_info, imposing the same requirement on the users of this > >> + * public API will break them. > > This sentence doesn't quite parse. You either want "b_info. Imposing", > or something like "b_info, and imposing". Yep. Already fixed in my local copy. I sent an email via xen.org address to that effect but it didn't come through. Wei. > > ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.