| Message ID | 1460051129-20817-2-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Apr 07, 2016 at 07:45:26PM +0200, Roger Pau Monne wrote: > So libxl doesn't have to pass the build info around just to get the device > model used by the guest. This allows to simplify > libxl__device_nic_setdefault. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Wei Liu <wei.liu2@citrix.com> I have some reservation on this approach. I would rather passing around a struct than accessing xenstore. The latter is much more expensive. Wei.
On Fri, 8 Apr 2016, Wei Liu wrote: > On Thu, Apr 07, 2016 at 07:45:26PM +0200, Roger Pau Monne wrote: > > So libxl doesn't have to pass the build info around just to get the device > > model used by the guest. This allows to simplify > > libxl__device_nic_setdefault. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > Cc: Wei Liu <wei.liu2@citrix.com> > > I have some reservation on this approach. I would rather passing around > a struct than accessing xenstore. The latter is much more expensive. Sorry, my commit log wasn't very clear. The struct can't always be passed around, since it's only available at domain creation time, so we also need to xenstore way for hotplug. Due to that, I think it's simpler to always use it, instead of having two different approaches depending on whether the build info struct is provided or not. TBH, this is only one xenstore read, so I would rather prefer to do it always this way in order to have simpler code. Roger.
On Fri, Apr 08, 2016 at 04:16:19PM +0200, Roger Pau Monné wrote: > On Fri, 8 Apr 2016, Wei Liu wrote: > > On Thu, Apr 07, 2016 at 07:45:26PM +0200, Roger Pau Monne wrote: > > > So libxl doesn't have to pass the build info around just to get the device > > > model used by the guest. This allows to simplify > > > libxl__device_nic_setdefault. > > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > > > Cc: Wei Liu <wei.liu2@citrix.com> > > > > I have some reservation on this approach. I would rather passing around > > a struct than accessing xenstore. The latter is much more expensive. > > Sorry, my commit log wasn't very clear. The struct can't always be passed > around, since it's only available at domain creation time, so we also need > to xenstore way for hotplug. > > Due to that, I think it's simpler to always use it, instead of having two > different approaches depending on whether the build info struct is > provided or not. TBH, this is only one xenstore read, so I would rather > prefer to do it always this way in order to have simpler code. > In this case: Acked-by: Wei Liu <wei.liu2@citrix.com> Wei. > Roger.
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 5cdc09e..d35fc33 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -3292,7 +3292,7 @@ out: /******************************************************************************/ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, - uint32_t domid, libxl_domain_build_info *info) + uint32_t domid) { int rc; @@ -3330,21 +3330,11 @@ int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, switch (libxl__domain_type(gc, domid)) { case LIBXL_DOMAIN_TYPE_HVM: if (!nic->nictype) { - if (info != NULL) { - /* Path taken at creation time. */ - if (info->device_model_version == - LIBXL_DEVICE_MODEL_VERSION_NONE) - nic->nictype = LIBXL_NIC_TYPE_VIF; - else - nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU; - } else { - /* Path taken when hot-adding a nic. */ - if (libxl__device_model_version_running(gc, domid) == - LIBXL_DEVICE_MODEL_VERSION_NONE) - nic->nictype = LIBXL_NIC_TYPE_VIF; - else - nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU; - } + if (libxl__device_model_version_running(gc, domid) == + LIBXL_DEVICE_MODEL_VERSION_NONE) + nic->nictype = LIBXL_NIC_TYPE_VIF; + else + nic->nictype = LIBXL_NIC_TYPE_VIF_IOEMU; } break; case LIBXL_DOMAIN_TYPE_PV: @@ -3394,7 +3384,7 @@ void libxl__device_nic_add(libxl__egc *egc, uint32_t domid, libxl_device_nic_init(&nic_saved); libxl_device_nic_copy(CTX, &nic_saved, nic); - rc = libxl__device_nic_setdefault(gc, nic, domid, NULL); + rc = libxl__device_nic_setdefault(gc, nic, domid); if (rc) goto out; front = flexarray_make(gc, 16, 1); diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 4b02de9..24f168b 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -916,6 +916,12 @@ static void initiate_domain_create(libxl__egc *egc, goto error_out; } + /* + * Set the dm version quite early so that libxl doesn't have to pass the + * build info around just to know if the domain has a device model or not. + */ + store_libxl_entry(gc, domid, &d_config->b_info); + for (i = 0; i < d_config->num_disks; i++) { ret = libxl__device_disk_setdefault(gc, &d_config->disks[i]); if (ret) { @@ -940,8 +946,7 @@ static void initiate_domain_create(libxl__egc *egc, * called libxl_device_nic_add when domcreate_launch_dm gets called, * but qemu needs the nic information to be complete. */ - ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid, - &d_config->b_info); + ret = libxl__device_nic_setdefault(gc, &d_config->nics[i], domid); if (ret) { LOG(ERROR, "Unable to set nic defaults for nic %d", i); goto error_out; diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c index eac5501..a12c90d 100644 --- a/tools/libxl/libxl_dm.c +++ b/tools/libxl/libxl_dm.c @@ -1810,8 +1810,7 @@ static void spawn_stub_launch_dm(libxl__egc *egc, * called libxl_device_nic_add at this point, but qemu needs * the nic information to be complete. */ - ret = libxl__device_nic_setdefault(gc, &dm_config->nics[i], dm_domid, - &dm_config->b_info); + ret = libxl__device_nic_setdefault(gc, &dm_config->nics[i], dm_domid); if (ret) goto out; } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index 653c152..55896f8 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -1206,8 +1206,7 @@ _hidden int libxl__domain_build_info_setdefault(libxl__gc *gc, _hidden int libxl__device_disk_setdefault(libxl__gc *gc, libxl_device_disk *disk); _hidden int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic, - uint32_t domid, - libxl_domain_build_info *info); + uint32_t domid); _hidden int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm); _hidden int libxl__device_vfb_setdefault(libxl__gc *gc, libxl_device_vfb *vfb); _hidden int libxl__device_vkb_setdefault(libxl__gc *gc, libxl_device_vkb *vkb);
So libxl doesn't have to pass the build info around just to get the device model used by the guest. This allows to simplify libxl__device_nic_setdefault. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> --- tools/libxl/libxl.c | 24 +++++++----------------- tools/libxl/libxl_create.c | 9 +++++++-- tools/libxl/libxl_dm.c | 3 +-- tools/libxl/libxl_internal.h | 3 +-- 4 files changed, 16 insertions(+), 23 deletions(-)