Re: [Xen-devel] [PATCH v1] libxl: fix device_model_version related assert

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, May 16, 2019 at 02:50:00PM +0200, Olaf Hering wrote:
> In commit 3802ecbaa9 ("libxl: add helper function to set
> device_model_version") an assert was added to
> libxl__domain_build_info_setdefault to make sure callers provide
> complete info to this function. This unveiled a flaw in the way how
> libxl_domain_build_info is passed to libxl. The public API
> libxl_domain_need_memory passes an incomplete libxl_domain_build_info to
> libxl__domain_build_info_setdefault, which triggers the assert. Prior to
> the change above, device_model_version was hardcoded to QEMU_XEN which
> lead to the bugs described in that changeset.
> 
> A new libxl API would need to be created to fully populate
> libxl_domain_build_info with missing defaults. For existing, unchanged
> consumers of libxl the assumption about QEMU_XEN needs to be restored
> within this function to properly populate the amount of required memory.
> 
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> Reported-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  tools/libxl/libxl_create.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 89f99f7f44..030851fb28 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -123,6 +123,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>                                          libxl_domain_build_info *b_info)
>  {
>      int i, rc;
> +    libxl_device_model_version device_model_version;
>  
>      if (b_info->type != LIBXL_DOMAIN_TYPE_HVM &&
>          b_info->type != LIBXL_DOMAIN_TYPE_PV &&
> @@ -131,7 +132,9 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          return ERROR_INVAL;
>      }
>  
> -    assert(b_info->device_model_version);
> +    device_model_version = b_info->device_model_version;
> +    if (!device_model_version)
> +        device_model_version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;

This is a bit more code churn than I like.

Like I said, libxl_domain_need_memory is the only public API which takes
b_info. All other callers to libxl__domain_build_info_setdefault should
have d_config to hand. So changing the code here seems overkilled.

Would the following work?

diff --git a/tools/libxl/libxl_mem.c b/tools/libxl/libxl_mem.c
index 448a2af8fd..12af34f70e 100644
--- a/tools/libxl/libxl_mem.c
+++ b/tools/libxl/libxl_mem.c
@@ -457,6 +457,12 @@ 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);
 
+    /* Compatibility: if we don't set this, build_info_setdefault will
+     * try to access domain_config, which we don't have here.
+     */
+    if (!b_info->device_model_version)
+       b_info->device_model_version = LIBXL_DEVICE_MODEL_XXX;
+
     rc = libxl__domain_build_info_setdefault(gc, b_info);
     if (rc) goto out;


Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel