[PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Wei Liu]

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

[Xen-devel] [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Wei Liu]

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

Re: [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Roger Pau Monné]

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

Re: [Xen-devel] [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Roger Pau Monné]

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

Re: [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Andrew Cooper]

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

Re: [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Wei Liu]

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

Re: [Xen-devel] [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Wei Liu]

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

Re: [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Ian Jackson]

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

Re: [Xen-devel] [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Ian Jackson]

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

Re: [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Wei Liu]

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

Re: [Xen-devel] [PATCH] libxl: fix libxl_domain_need_memory after 899433f149d

[Wei Liu]

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