| Message ID | 20190517170555.17423-1-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | libxl: fix libxl_domain_need_memory after 899433f149d | expand |
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.
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
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
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.
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 >
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;
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(+)