Re: [PATCH v1] libxl: add helper function to set device_model_version

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Olaf Hering <olaf@aepfle.de>
Cc: xen-devel@lists.xenproject.org, Wei Liu <wei.liu2@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>
Subject: Re: [PATCH v1] libxl: add helper function to set device_model_version
Date: Tue, 14 May 2019 12:18:56 +0200	[thread overview]
Message-ID: <20190514101856.6otetd56n72t42bm@Air-de-Roger> (raw)
In-Reply-To: <20190514072741.11760-1-olaf@aepfle.de>

On Tue, May 14, 2019 at 09:27:41AM +0200, Olaf Hering wrote:
> An upcoming change will set the value of device_model_version properly
> also for the non-HVM case.
> 
> Move existing code to new function libxl__domain_set_device_model.
> Move also initialization for device_model_stubdomain to that function.
> Make sure libxl__domain_build_info_setdefault is called with
> device_model_version set.
> 
> Update libxl__spawn_stub_dm() and initiate_domain_create() to call the
> new function prior libxl__domain_build_info_setdefault() because
> device_mode_version is expected to be initialzed.

That's all fine, but this just describes the changes performed below
without providing a reasoning on why those changes are needed. Why is
it not fine to set the device model version in
libxl__domain_build_info_setdefault?

> @@ -938,6 +952,12 @@ static void initiate_domain_create(libxl__egc *egc,
>          goto error_out;
>      }
>  
> +    ret = libxl__domain_set_device_model(gc, d_config);
> +    if (ret) {
> +        LOGD(ERROR, domid, "Unable to set domain device model");
> +        goto error_out;
> +    }

Can you place the call to libxl__domain_set_device_model at the top
(or a suitable place) of libxl__domain_build_info_setdefault instead
of adding a call in initiate_domain_create?

> +
>      ret = libxl__domain_create_info_setdefault(gc, &d_config->c_info);
>      if (ret) {
>          LOGD(ERROR, domid, "Unable to set domain create info defaults");
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 2f19786bdd..086e566311 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -2168,6 +2168,8 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
>      dm_config->c_info.run_hotplug_scripts =
>          guest_config->c_info.run_hotplug_scripts;
>  
> +    ret = libxl__domain_set_device_model(gc, dm_config);
> +    if (ret) goto out;
>      ret = libxl__domain_create_info_setdefault(gc, &dm_config->c_info);
>      if (ret) goto out;
>      ret = libxl__domain_build_info_setdefault(gc, &dm_config->b_info);

Same here, AFAICT the call to libxl__domain_set_device_model could be
placed inside of libxl__domain_build_info_setdefault?

Thanks, Roger.

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