| Message ID | 20190514072741.11760-1-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] libxl: add helper function to set device_model_version | expand |
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.
Am Tue, 14 May 2019 12:18:56 +0200 schrieb Roger Pau Monné <roger.pau@citrix.com>: > Why is it not fine to set the device model version in > libxl__domain_build_info_setdefault? Because it receives just build_info, which lacks all the data to decide if a device type needs a device model or not. Olaf
On Tue, May 14, 2019 at 12:31:18PM +0200, Olaf Hering wrote: > Am Tue, 14 May 2019 12:18:56 +0200 > schrieb Roger Pau Monné <roger.pau@citrix.com>: > > > Why is it not fine to set the device model version in > > libxl__domain_build_info_setdefault? > > Because it receives just build_info, which lacks all the data to decide > if a device type needs a device model or not. Gah, thanks. Could you please add this to the commit message? Or else it's likely I will ask again. With that: Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Roger.
On Tue, May 14, 2019 at 12:39:07PM +0200, Roger Pau Monné wrote: > On Tue, May 14, 2019 at 12:31:18PM +0200, Olaf Hering wrote: > > Am Tue, 14 May 2019 12:18:56 +0200 > > schrieb Roger Pau Monné <roger.pau@citrix.com>: > > > > > Why is it not fine to set the device model version in > > > libxl__domain_build_info_setdefault? > > > > Because it receives just build_info, which lacks all the data to decide > > if a device type needs a device model or not. > > Gah, thanks. Could you please add this to the commit message? Or else > it's likely I will ask again. > > With that: > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Wei Liu <wei.liu2@citrix.com> Olaf, if you can provide me with an updated version of the commit message I can fold that in while committing. No need to resend this patch. Wei. > > Roger.
Am Tue, 14 May 2019 15:32:27 +0100 schrieb Wei Liu <wei.liu2@citrix.com>: > Olaf, if you can provide me with an updated version of the commit > message I can fold that in while committing. No need to resend this > patch. Maybe just append this paragraph? The upcoming change needs a full libxl_domain_config, and the existing libxl__domain_build_info_setdefault has just a libxl_domain_build_info to work with. Olaf
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 89fe80fc9c..3f0431cc84 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -27,6 +27,53 @@ #include <xen-xsm/flask/flask.h> +int libxl__domain_set_device_model(libxl__gc *gc, libxl_domain_config *d_config) +{ + libxl_domain_build_info *b_info = &d_config->b_info; + int ret; + + libxl_defbool_setdefault(&b_info->device_model_stubdomain, false); + + if (b_info->device_model_version) + return 0; + + switch (b_info->type) { + case LIBXL_DOMAIN_TYPE_HVM: + if (libxl_defbool_val(b_info->device_model_stubdomain)) { + b_info->device_model_version = + LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; + } else { + b_info->device_model_version = libxl__default_device_model(gc); + } + break; + default: + b_info->device_model_version = + LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; + break; + } + + if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) { + const char *dm; + + dm = libxl__domain_device_model(gc, b_info); + ret = access(dm, X_OK); + if (ret < 0) { + /* qemu-xen unavailable, use qemu-xen-traditional */ + if (errno == ENOENT) { + LOGE(INFO, "qemu-xen is unavailable" + ", using qemu-xen-traditional instead"); + b_info->device_model_version = + LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; + } else { + LOGE(ERROR, "qemu-xen access error"); + return ERROR_FAIL; + } + } + } + + return 0; +} + int libxl__domain_create_info_setdefault(libxl__gc *gc, libxl_domain_create_info *c_info) { @@ -73,6 +120,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, return ERROR_INVAL; } + assert(b_info->device_model_version); + /* Copy deprecated options to it's new position. */ rc = libxl__domain_build_info_copy_deprecated(CTX, b_info); if (rc) { @@ -80,45 +129,10 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, return rc; } - libxl_defbool_setdefault(&b_info->device_model_stubdomain, false); - if (libxl_defbool_val(b_info->device_model_stubdomain) && !b_info->device_model_ssidref) b_info->device_model_ssidref = SECINITSID_DOMDM; - if (!b_info->device_model_version) { - if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) { - if (libxl_defbool_val(b_info->device_model_stubdomain)) { - b_info->device_model_version = - LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; - } else { - b_info->device_model_version = libxl__default_device_model(gc); - } - } else { - b_info->device_model_version = - LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN; - } - if (b_info->device_model_version - == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) { - const char *dm; - - dm = libxl__domain_device_model(gc, b_info); - rc = access(dm, X_OK); - if (rc < 0) { - /* qemu-xen unavailable, use qemu-xen-traditional */ - if (errno == ENOENT) { - LOGE(INFO, "qemu-xen is unavailable" - ", using qemu-xen-traditional instead"); - b_info->device_model_version = - LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL; - } else { - LOGE(ERROR, "qemu-xen access error"); - return ERROR_FAIL; - } - } - } - } - if (b_info->blkdev_start == NULL) b_info->blkdev_start = libxl__strdup(NOGC, "xvda"); @@ -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; + } + 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); diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 44e0221284..25f113404b 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1374,6 +1374,8 @@ _hidden int libxl__device_nextid(libxl__gc *gc, uint32_t domid, _hidden int libxl__resolve_domid(libxl__gc *gc, const char *name, uint32_t *domid); +_hidden int libxl__domain_set_device_model(libxl__gc *gc, + libxl_domain_config *d_config); /* * For each aggregate type which can be used as an input we provide: *
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. libxl_domain_need_memory() needs no update because it does not have a d_config available anyway, and the callers provide a populated b_info. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- tools/libxl/libxl_create.c | 90 +++++++++++++++++++++++++++----------------- tools/libxl/libxl_dm.c | 2 + tools/libxl/libxl_internal.h | 2 + 3 files changed, 59 insertions(+), 35 deletions(-)