[libvirt] [PATCH 0/4] Couple of almost trivial disk cold-/hot-plug fixes
*** BLURB HERE *** Michal Prívozník (4): src: Check for virDomainDiskInsert() retval properly qemuDomainAttachDeviceLiveAndConfig: Don't overwrite @ret qemu: On attach to live XML check for user alias collision only live XML qemu: Check for user alias collisions in coldplug src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 32 ++++++++++++++++++-------------- 3 files changed, 20 insertions(+), 16 deletions(-) -- 2.21.0
Our coding style specifies that only negative values are considered as error. Check for return value of virDomainDiskInsert() properly, following the style. Not that the function can now return anything other than 0 or -1, but it just triggers my OCD. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 42221cb925..e7234c1479 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -3517,7 +3517,7 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev) _("target %s already exists."), disk->dst); return -1; } - if (virDomainDiskInsert(vmdef, disk)) + if (virDomainDiskInsert(vmdef, disk) < 0) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ dev->data.disk = NULL; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 37002dbf23..fbdd33156a 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3417,7 +3417,7 @@ lxcDomainAttachDeviceConfig(virDomainDefPtr vmdef, _("target %s already exists."), disk->dst); return -1; } - if (virDomainDiskInsert(vmdef, disk)) + if (virDomainDiskInsert(vmdef, disk) < 0) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ dev->data.disk = NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f48d9256e4..30fc335883 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8144,7 +8144,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef, return -1; if (qemuCheckDiskConfig(disk, NULL) < 0) return -1; - if (virDomainDiskInsert(vmdef, disk)) + if (virDomainDiskInsert(vmdef, disk) < 0) return -1; /* vmdef has the pointer. Generic codes for vmdef will do all jobs */ dev->data.disk = NULL; -- 2.21.0
On Fri, Apr 26, 2019 at 15:52:20 +0200, Michal Privoznik wrote:
Our coding style specifies that only negative values are considered as error. Check for return value of virDomainDiskInsert() properly, following the style. Not that the function can now return anything other than 0 or -1, but it just triggers my OCD.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/libxl_driver.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_driver.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
ACK
If we're attaching a device to both inactive and live XML then @ret is overwritten which may result in incorrect return value. For instance, if attaching to inactive XML succeeds, @ret is assigned value of zero and control proceeds to attaching the device to live XML. Here, if say virDomainDeviceValidateAliasForHotplug() fails the control jumps over to 'cleanup' label and zero is returned indicating success. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 30fc335883..2b2d531441 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8652,9 +8652,9 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, false) < 0) goto cleanup; - if ((ret = qemuDomainAttachDeviceConfig(vmdef, devConf, caps, - parse_flags, - driver->xmlopt)) < 0) + if (qemuDomainAttachDeviceConfig(vmdef, devConf, caps, + parse_flags, + driver->xmlopt) < 0) goto cleanup; } @@ -8671,28 +8671,27 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, true) < 0) goto cleanup; - if ((ret = qemuDomainAttachDeviceLive(vm, devLive, driver)) < 0) + if (qemuDomainAttachDeviceLive(vm, devLive, driver) < 0) goto cleanup; /* * update domain status forcibly because the domain status may be * changed even if we failed to attach the device. For example, * a new controller may be created. */ - if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) { - ret = -1; + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0) goto cleanup; - } } /* Finally, if no error until here, we can save config. */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - ret = virDomainSaveConfig(cfg->configDir, driver->caps, vmdef); - if (!ret) { - virDomainObjAssignDef(vm, vmdef, false, NULL); - vmdef = NULL; - } + if (virDomainSaveConfig(cfg->configDir, driver->caps, vmdef) < 0) + goto cleanup; + + virDomainObjAssignDef(vm, vmdef, false, NULL); + vmdef = NULL; } + ret = 0; cleanup: virDomainDefFree(vmdef); virDomainDeviceDefFree(devConf); -- 2.21.0
On Fri, Apr 26, 2019 at 15:52:21 +0200, Michal Privoznik wrote:
If we're attaching a device to both inactive and live XML then @ret is overwritten which may result in incorrect return value. For instance, if attaching to inactive XML succeeds, @ret is assigned value of zero and control proceeds to attaching the device to live XML. Here, if say virDomainDeviceValidateAliasForHotplug() fails the control jumps over to 'cleanup' label and zero is returned indicating success.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-)
ACK
When attaching a device to live XML we don't care (well, shouldn't care) that there's already a device in inactive XML that has the same user alias. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b2d531441..40fd8b9d2d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8663,7 +8663,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, driver->xmlopt, parse_flags))) goto cleanup; - if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0) + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, + VIR_DOMAIN_AFFECT_LIVE) < 0) goto cleanup; if (virDomainDefCompatibleDevice(vm->def, devLive, NULL, -- 2.21.0
On Fri, Apr 26, 2019 at 15:52:22 +0200, Michal Privoznik wrote:
When attaching a device to live XML we don't care (well, shouldn't care) that there's already a device in inactive XML that has the same user alias.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b2d531441..40fd8b9d2d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8663,7 +8663,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, driver->xmlopt, parse_flags))) goto cleanup;
- if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0) + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, + VIR_DOMAIN_AFFECT_LIVE) < 0)
Since this function already interprets the VIR_DOMAIN_AFFECT_* flags, wouldn't it make more sense to move this out before the LIVE/CONFIG attach is actually attempted? That would also replace the second copy of the function which is added by the next patch.
On 4/26/19 4:23 PM, Peter Krempa wrote:
On Fri, Apr 26, 2019 at 15:52:22 +0200, Michal Privoznik wrote:
When attaching a device to live XML we don't care (well, shouldn't care) that there's already a device in inactive XML that has the same user alias.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b2d531441..40fd8b9d2d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8663,7 +8663,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, driver->xmlopt, parse_flags))) goto cleanup;
- if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0) + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, + VIR_DOMAIN_AFFECT_LIVE) < 0)
Since this function already interprets the VIR_DOMAIN_AFFECT_* flags, wouldn't it make more sense to move this out before the LIVE/CONFIG attach is actually attempted?
That would also replace the second copy of the function which is added by the next patch.
That's how it used to be e2797e3256c but then we had to change it in 55ce6564634. Long story short, at the beginning of this function we haven't parsed device XML yet, so we don't know the device alias. Michal
On Fri, Apr 26, 2019 at 15:52:22 +0200, Michal Privoznik wrote:
When attaching a device to live XML we don't care (well, shouldn't care) that there's already a device in inactive XML that has the same user alias.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2b2d531441..40fd8b9d2d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8663,7 +8663,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, driver->xmlopt, parse_flags))) goto cleanup;
- if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0) + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, + VIR_DOMAIN_AFFECT_LIVE) < 0) goto cleanup;
ACK
https://bugzilla.redhat.com/show_bug.cgi?id=1697676 If an user tries to attach a device with colliding user alias then we attach it happily and thus leave domain unable to start. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 40fd8b9d2d..c072bed1ce 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8647,6 +8647,10 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, driver->xmlopt, parse_flags))) goto cleanup; + if (virDomainDeviceValidateAliasForHotplug(vm, devConf, + VIR_DOMAIN_AFFECT_CONFIG) < 0) + goto cleanup; + if (virDomainDefCompatibleDevice(vmdef, devConf, NULL, VIR_DOMAIN_DEVICE_ACTION_ATTACH, false) < 0) -- 2.21.0
On Fri, Apr 26, 2019 at 15:52:23 +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1697676
If an user tries to attach a device with colliding user alias then we attach it happily and thus leave domain unable to start.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 40fd8b9d2d..c072bed1ce 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8647,6 +8647,10 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm, driver->xmlopt, parse_flags))) goto cleanup;
+ if (virDomainDeviceValidateAliasForHotplug(vm, devConf, + VIR_DOMAIN_AFFECT_CONFIG) < 0) + goto cleanup; + if (virDomainDefCompatibleDevice(vmdef, devConf, NULL, VIR_DOMAIN_DEVICE_ACTION_ATTACH,
ACK
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa