4:14 p.m.
v2: https://www.redhat.com/archives/libvir-list/2018-July/msg00361.html
Differences to v2:
Patch1: NEW - As a result of code review the suggestion was to utilize
the virDomainDefCompatibleDevice in order to make the check more
generic to include <disk>'s which were also afflicted with the
same problem that I was trying to solve with the former patch1
just for <hostdev>'s. However, this led me down into the abyss of
more changes since <disk>'s have multiple <address
type='drive'...>
target bus types (IDE and SCSI). That means we need to have a
mechanism to pass the target bus along so that a SCSI drive
address doesn't inadvertently match an IDE drive address. All
that is complicated by the way virDomainDefHasDeviceAddress
iterates through all the device lists. Whether there are more
similar devices I'm assuming will fall out of code review.
Patch2: This moves the virDomainDefHasDeviceAddress into the more
common config checking virDomainDefCompatibleDevice method,
but now needs to also account for the disk bus issue.
This theoretically could be combined with Patch1, but keeping
them separate I would hope makes for simpler code review. I
could also move code out of virDomainDefHasDeviceAddressIterator
into patch2, but if just felt better in patch1.
Patch3: No changes were made (amazingly so).
In the end quite a bit more complicated
John Ferlan (3):
conf: Add @target_bus to virDomainDefHasDeviceAddress
qemu: Check for existing address when cold attach device
qemu: Use the correct vm def on cold attach
src/conf/domain_conf.c | 54 +++++++++++++++++++++++++++++++++++---
src/conf/domain_conf.h | 3 ++-
src/qemu/qemu_driver.c | 59 ++++++++++++++++--------------------------
3 files changed, 75 insertions(+), 41 deletions(-)
--
2.17.1
4:14 p.m.
New subject: [libvirt] [PATCH v3 1/3] conf: Add @target_bus to virDomainDefHasDeviceAddress
Add the @target_bus argument which will allow a caller to
pass the virDomainDiskBus onto which the @info (<address>)
would be placed. This will allow logic to provide the bus for
cold plugged devices to determine whether the about to
be added device <address> is already present on the @bus.
Just passing the @info isn't sufficient since, for example,
ADDRESS_TYPE_DRIVE is used for both SCSI and IDE <disk>'s
as well as 'scsi' and 'scsi_host' <hostdev>'s.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 32 ++++++++++++++++++++++++++++----
src/conf/domain_conf.h | 3 ++-
src/qemu/qemu_driver.c | 2 +-
3 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7396616eda..82df8012af 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3688,13 +3688,31 @@ virDomainDeviceGetInfo(virDomainDeviceDefPtr device)
}
+struct virDomainDefHasDeviceAddressIteratorData {
+ int target_bus; /* virDomainDiskBus or -1 */
+ virDomainDeviceInfoPtr info;
+};
+
static int
virDomainDefHasDeviceAddressIterator(virDomainDefPtr def ATTRIBUTE_UNUSED,
- virDomainDeviceDefPtr dev ATTRIBUTE_UNUSED,
+ virDomainDeviceDefPtr dev,
virDomainDeviceInfoPtr info,
void *opaque)
{
- virDomainDeviceInfoPtr needle = opaque;
+ struct virDomainDefHasDeviceAddressIteratorData *data = opaque;
+ int target_bus = data->target_bus;
+ virDomainDeviceInfoPtr needle = data->info;
+
+ /* If the target_bus of the about to be cold plugged device needs
+ * to be checked and the currently to be matched device is a disk,
+ * then compare it's target bus against the new device. If they don't
+ * match, then no need to compare. For disks this ensures addresses
+ * using drive won't erroneously match if one is IDE and another is SCSI.
+ * Likewise, for SCSI hostdev's this ensures the new hostdev doesn't
+ * erroneously match an IDE for the address comparison. */
+ if (target_bus != -1 && dev->type == VIR_DOMAIN_DEVICE_DISK &&
+ dev->data.disk->bus != target_bus)
+ return 0;
/* break iteration if the info was found */
if (virDomainDeviceInfoAddressIsEqual(info, needle))
@@ -3933,12 +3951,18 @@ virDomainDeviceInfoIterate(virDomainDefPtr def,
bool
virDomainDefHasDeviceAddress(virDomainDefPtr def,
+ int target_bus,
virDomainDeviceInfoPtr info)
{
+ struct virDomainDefHasDeviceAddressIteratorData data = {
+ .target_bus = target_bus,
+ .info = info,
+ };
+
if (virDomainDeviceInfoIterateInternal(def,
virDomainDefHasDeviceAddressIterator,
true,
- info) < 0)
+ &data) < 0)
return true;
return false;
@@ -17508,7 +17532,7 @@ virDomainMemoryInsert(virDomainDefPtr def,
int id = def->nmems;
if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
- virDomainDefHasDeviceAddress(def, &mem->info)) {
+ virDomainDefHasDeviceAddress(def, -1, &mem->info)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("Domain already contains a device with the same "
"address"));
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0f10e242fd..82231161c6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2912,8 +2912,9 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def,
void *opaque);
bool virDomainDefHasDeviceAddress(virDomainDefPtr def,
+ int target_bus,
virDomainDeviceInfoPtr info)
- ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_RETURN_CHECK;
void virDomainDefFree(virDomainDefPtr vm);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8fae46370e..5f91d463ae 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8082,7 +8082,7 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
case VIR_DOMAIN_DEVICE_RNG:
if (dev->data.rng->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
&&
- virDomainDefHasDeviceAddress(vmdef, &dev->data.rng->info)) {
+ virDomainDefHasDeviceAddress(vmdef, -1, &dev->data.rng->info)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("a device with the same address already exists
"));
return -1;
--
2.17.1
4:14 p.m.
New subject: [libvirt] [PATCH v3 2/3] qemu: Check for existing address when cold attach device
Now that virDomainDefCompatibleDevice accepts an action,
let's use that to generically determine whether a to be
cold plugged device with a defined <address> would conflict
with any existing address for the domain config. This
replaces the RNG check in qemuDomainAttachDeviceConfig.
This type of check would need to be made before a device
is inserted into its domain device list (disk, rng, hostdev,
etc.). This type of check cannot be done during the post
parse processing because the check would conflict with
itself as the device would be placed onto the its device
list prior to post parse.
By comparison this is similar to the validation phase
checks in virDomainDefCheckDuplicateDriveAddresses that
occur during define/startup processing, but are not run
during config attach of a live guest.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
src/qemu/qemu_driver.c | 7 -------
2 files changed, 22 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 82df8012af..fd5bc10c82 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28270,11 +28270,25 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceAction action,
bool live)
{
+ int target_bus = -1;
virDomainCompatibleDeviceData data = {
.newInfo = virDomainDeviceGetInfo(dev),
.oldInfo = NULL,
};
+ if (dev->type == VIR_DOMAIN_DEVICE_DISK) {
+ target_bus = dev->data.disk->bus;
+ } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV &&
+ dev->data.hostdev->mode == VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
+ virDomainHostdevSubsysPtr subsys = &dev->data.hostdev->source.subsys;
+
+ /* If using 'scsi' or 'scsi_host', then the device can be placed
+ * on the same bus as a <disk>, so account for that */
+ if (subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI ||
+ subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST)
+ target_bus = VIR_DOMAIN_DISK_BUS_SCSI;
+ }
+
if (oldDev)
data.oldInfo = virDomainDeviceGetInfo(oldDev);
@@ -28288,6 +28302,14 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
return -1;
}
+ if (action == VIR_DOMAIN_DEVICE_ACTION_ATTACH && !live &&
data.newInfo &&
+ data.newInfo->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE &&
+ virDomainDefHasDeviceAddress(def, target_bus, data.newInfo)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("a device with the same address already exists"));
+ return -1;
+ }
+
if (!virDomainDefHasUSB(def) &&
def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
virDomainDeviceIsUSB(dev)) {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 5f91d463ae..7d519c0714 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8081,13 +8081,6 @@ qemuDomainAttachDeviceConfig(virDomainDefPtr vmdef,
break;
case VIR_DOMAIN_DEVICE_RNG:
- if (dev->data.rng->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE
&&
- virDomainDefHasDeviceAddress(vmdef, -1, &dev->data.rng->info)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("a device with the same address already exists
"));
- return -1;
- }
-
if (VIR_APPEND_ELEMENT(vmdef->rngs, vmdef->nrngs, dev->data.rng) <
0)
return -1;
dev->data.rng = NULL;
--
2.17.1
4:14 p.m.
New subject: [libvirt] [PATCH v3 3/3] qemu: Use the correct vm def on cold attach
https://bugzilla.redhat.com/show_bug.cgi?id=1559867
When attaching a device to the domain we need to be sure
to use the correct domain definition (vm->def or vm->newDef)
when calling virDomainDeviceDefParse because the post parse
processing algorithms that may assign an address for the
device will use whatever domain definition was passed in.
Additionally, some devices (SCSI hostdev and SCSI disk) use
algorithms that rely on knowing what already exists of the
other type when generating the new device's address. Using
the wrong VM definition could result in duplicated addresses.
In the case of the bz, two hostdev's with no domain address
provided were added to the running domain's config only.
However, the parsing algorithm used the live domain in
order to figure out the host device address resulting in
the same address being used and a subsequent start failing
due to duplicate address.
Fix this by separating the checks/code into CONFIG and LIVE
processing using the correct definition for each block and
performing cleanup for both options as necessary.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 52 +++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7d519c0714..3dbd5c62d2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8473,7 +8473,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
{
virDomainDefPtr vmdef = NULL;
virQEMUDriverConfigPtr cfg = NULL;
- virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
+ virDomainDeviceDefPtr devConf = NULL;
+ virDomainDeviceDefPtr devLive = NULL;
int ret = -1;
virCapsPtr caps = NULL;
unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
@@ -8487,49 +8488,43 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
- dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
- caps, driver->xmlopt,
- parse_flags);
- if (dev == NULL)
- goto cleanup;
-
- if (virDomainDeviceValidateAliasForHotplug(vm, dev, flags) < 0)
- goto cleanup;
-
- if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
- flags & VIR_DOMAIN_AFFECT_LIVE) {
- /* If we are affecting both CONFIG and LIVE
- * create a deep copy of device as adding
- * to CONFIG takes one instance.
- */
- dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt);
- if (!dev_copy)
- goto cleanup;
- }
-
+ /* The config and live post processing address auto-generation algorithms
+ * rely on the correct vm->def or vm->newDef being passed, so call the
+ * device parse based on which definition is in use */
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
- /* Make a copy for updated domain. */
vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
if (!vmdef)
goto cleanup;
- if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
+ if (!(devConf = virDomainDeviceDefParse(xml, vmdef, caps,
+ driver->xmlopt, parse_flags)))
+ goto cleanup;
+
+ if (virDomainDefCompatibleDevice(vmdef, devConf, NULL,
VIR_DOMAIN_DEVICE_ACTION_ATTACH,
false) < 0)
goto cleanup;
- if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
+
+ if ((ret = qemuDomainAttachDeviceConfig(vmdef, devConf, caps,
parse_flags,
driver->xmlopt)) < 0)
goto cleanup;
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
+ if (!(devLive = virDomainDeviceDefParse(xml, vm->def, caps,
+ driver->xmlopt, parse_flags)))
+ goto cleanup;
+
+ if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0)
+ goto cleanup;
+
+ if (virDomainDefCompatibleDevice(vm->def, devLive, NULL,
VIR_DOMAIN_DEVICE_ACTION_ATTACH,
true) < 0)
goto cleanup;
- if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
+ if ((ret = qemuDomainAttachDeviceLive(vm, devLive, driver)) < 0)
goto cleanup;
/*
* update domain status forcibly because the domain status may be
@@ -8553,9 +8548,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
cleanup:
virDomainDefFree(vmdef);
- if (dev != dev_copy)
- virDomainDeviceDefFree(dev_copy);
- virDomainDeviceDefFree(dev);
+ virDomainDeviceDefFree(devConf);
+ virDomainDeviceDefFree(devLive);
virObjectUnref(cfg);
virObjectUnref(caps);
--
2.17.1
5:40 a.m.
New subject: [libvirt] [PATCH v3 3/3] qemu: Use the correct vm def on cold attach
On 07/16/2018 11:14 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1559867
When attaching a device to the domain we need to be sure
to use the correct domain definition (vm->def or vm->newDef)
when calling virDomainDeviceDefParse because the post parse
processing algorithms that may assign an address for the
device will use whatever domain definition was passed in.
Additionally, some devices (SCSI hostdev and SCSI disk) use
algorithms that rely on knowing what already exists of the
other type when generating the new device's address. Using
the wrong VM definition could result in duplicated addresses.
In the case of the bz, two hostdev's with no domain address
provided were added to the running domain's config only.
However, the parsing algorithm used the live domain in
order to figure out the host device address resulting in
the same address being used and a subsequent start failing
due to duplicate address.
Fix this by separating the checks/code into CONFIG and LIVE
processing using the correct definition for each block and
performing cleanup for both options as necessary.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 52 +++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 29 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7d519c0714..3dbd5c62d2 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8473,7 +8473,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
{
virDomainDefPtr vmdef = NULL;
virQEMUDriverConfigPtr cfg = NULL;
- virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
+ virDomainDeviceDefPtr devConf = NULL;
+ virDomainDeviceDefPtr devLive = NULL;
int ret = -1;
virCapsPtr caps = NULL;
unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
@@ -8487,49 +8488,43 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
goto cleanup;
- dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
- caps, driver->xmlopt,
- parse_flags);
- if (dev == NULL)
- goto cleanup;
-
- if (virDomainDeviceValidateAliasForHotplug(vm, dev, flags) < 0)
- goto cleanup;
-
- if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
- flags & VIR_DOMAIN_AFFECT_LIVE) {
- /* If we are affecting both CONFIG and LIVE
- * create a deep copy of device as adding
- * to CONFIG takes one instance.
- */
- dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps, driver->xmlopt);
- if (!dev_copy)
- goto cleanup;
- }
-
+ /* The config and live post processing address auto-generation algorithms
+ * rely on the correct vm->def or vm->newDef being passed, so call the
+ * device parse based on which definition is in use */
if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
- /* Make a copy for updated domain. */
vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
if (!vmdef)
goto cleanup;
- if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
+ if (!(devConf = virDomainDeviceDefParse(xml, vmdef, caps,
+ driver->xmlopt, parse_flags)))
+ goto cleanup;
+
+ if (virDomainDefCompatibleDevice(vmdef, devConf, NULL,
VIR_DOMAIN_DEVICE_ACTION_ATTACH,
false) < 0)
goto cleanup;
- if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
+
+ if ((ret = qemuDomainAttachDeviceConfig(vmdef, devConf, caps,
parse_flags,
driver->xmlopt)) < 0)
goto cleanup;
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
+ if (!(devLive = virDomainDeviceDefParse(xml, vm->def, caps,
+ driver->xmlopt, parse_flags)))
+ goto cleanup;
+
+ if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0)
+ goto cleanup;
+
+ if (virDomainDefCompatibleDevice(vm->def, devLive, NULL,
VIR_DOMAIN_DEVICE_ACTION_ATTACH,
true) < 0)
goto cleanup;
- if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
+ if ((ret = qemuDomainAttachDeviceLive(vm, devLive, driver)) < 0)
goto cleanup;
/*
* update domain status forcibly because the domain status may be
@@ -8553,9 +8548,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
cleanup:
virDomainDefFree(vmdef);
- if (dev != dev_copy)
- virDomainDeviceDefFree(dev_copy);
- virDomainDeviceDefFree(dev);
+ virDomainDeviceDefFree(devConf);
+ virDomainDeviceDefFree(devLive);
virObjectUnref(cfg);
virObjectUnref(caps);
I'm failing to see why the other patches are necessary. I'm unable to
reproduce the bug after I applied only this single patch.
It also makes sense, because the source of error is that when parsing
device XML wrong vmdef was passed. Therefore, when postParse callback
tried to fill device address it was looking at wrong data (thinking
there's no such drive address) and returned the next free address which
was the same all the time.
ACK to this patch.
Michal
7:39 a.m.
New subject: [libvirt] [PATCH v3 3/3] qemu: Use the correct vm def on cold attach
On 07/25/2018 06:40 AM, Michal Privoznik wrote:
On 07/16/2018 11:14 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1559867
>
> When attaching a device to the domain we need to be sure
> to use the correct domain definition (vm->def or vm->newDef)
> when calling virDomainDeviceDefParse because the post parse
> processing algorithms that may assign an address for the
> device will use whatever domain definition was passed in.
>
> Additionally, some devices (SCSI hostdev and SCSI disk) use
> algorithms that rely on knowing what already exists of the
> other type when generating the new device's address. Using
> the wrong VM definition could result in duplicated addresses.
>
> In the case of the bz, two hostdev's with no domain address
> provided were added to the running domain's config only.
> However, the parsing algorithm used the live domain in
> order to figure out the host device address resulting in
> the same address being used and a subsequent start failing
> due to duplicate address.
>
> Fix this by separating the checks/code into CONFIG and LIVE
> processing using the correct definition for each block and
> performing cleanup for both options as necessary.
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/qemu/qemu_driver.c | 52 +++++++++++++++++++-----------------------
> 1 file changed, 23 insertions(+), 29 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7d519c0714..3dbd5c62d2 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -8473,7 +8473,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
> {
> virDomainDefPtr vmdef = NULL;
> virQEMUDriverConfigPtr cfg = NULL;
> - virDomainDeviceDefPtr dev = NULL, dev_copy = NULL;
> + virDomainDeviceDefPtr devConf = NULL;
> + virDomainDeviceDefPtr devLive = NULL;
> int ret = -1;
> virCapsPtr caps = NULL;
> unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> @@ -8487,49 +8488,43 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
> if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> goto cleanup;
>
> - dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
> - caps, driver->xmlopt,
> - parse_flags);
> - if (dev == NULL)
> - goto cleanup;
> -
> - if (virDomainDeviceValidateAliasForHotplug(vm, dev, flags) < 0)
> - goto cleanup;
> -
> - if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
> - flags & VIR_DOMAIN_AFFECT_LIVE) {
> - /* If we are affecting both CONFIG and LIVE
> - * create a deep copy of device as adding
> - * to CONFIG takes one instance.
> - */
> - dev_copy = virDomainDeviceDefCopy(dev, vm->def, caps,
driver->xmlopt);
> - if (!dev_copy)
> - goto cleanup;
> - }
> -
> + /* The config and live post processing address auto-generation algorithms
> + * rely on the correct vm->def or vm->newDef being passed, so call the
> + * device parse based on which definition is in use */
> if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> - /* Make a copy for updated domain. */
> vmdef = virDomainObjCopyPersistentDef(vm, caps, driver->xmlopt);
> if (!vmdef)
> goto cleanup;
>
> - if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
> + if (!(devConf = virDomainDeviceDefParse(xml, vmdef, caps,
> + driver->xmlopt, parse_flags)))
> + goto cleanup;
> +
> + if (virDomainDefCompatibleDevice(vmdef, devConf, NULL,
> VIR_DOMAIN_DEVICE_ACTION_ATTACH,
> false) < 0)
> goto cleanup;
> - if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
> +
> + if ((ret = qemuDomainAttachDeviceConfig(vmdef, devConf, caps,
> parse_flags,
> driver->xmlopt)) < 0)
> goto cleanup;
> }
>
> if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> - if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
> + if (!(devLive = virDomainDeviceDefParse(xml, vm->def, caps,
> + driver->xmlopt, parse_flags)))
> + goto cleanup;
> +
> + if (virDomainDeviceValidateAliasForHotplug(vm, devLive, flags) < 0)
> + goto cleanup;
> +
> + if (virDomainDefCompatibleDevice(vm->def, devLive, NULL,
> VIR_DOMAIN_DEVICE_ACTION_ATTACH,
> true) < 0)
> goto cleanup;
>
> - if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
> + if ((ret = qemuDomainAttachDeviceLive(vm, devLive, driver)) < 0)
> goto cleanup;
> /*
> * update domain status forcibly because the domain status may be
> @@ -8553,9 +8548,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
>
> cleanup:
> virDomainDefFree(vmdef);
> - if (dev != dev_copy)
> - virDomainDeviceDefFree(dev_copy);
> - virDomainDeviceDefFree(dev);
> + virDomainDeviceDefFree(devConf);
> + virDomainDeviceDefFree(devLive);
> virObjectUnref(cfg);
> virObjectUnref(caps);
>
>
I'm failing to see why the other patches are necessary. I'm unable to
reproduce the bug after I applied only this single patch.
It also makes sense, because the source of error is that when parsing
device XML wrong vmdef was passed. Therefore, when postParse callback
tried to fill device address it was looking at wrong data (thinking
there's no such drive address) and returned the next free address which
was the same all the time.
ACK to this patch.
Michal
OK - fair enough.
It's been a few weeks, but I thought I had gone through some testing
with only this patch applied and attempting the attach of a <hostdev>
without an <address> defined and was getting the same unit#, but now of
course with just this applied I cannot. So much for short term memory.
Thanks for the review - I'll drop 1 and 2 and just push 3 -
John
8:51 a.m.
New subject: [libvirt] [PATCH v3 3/3] qemu: Use the correct vm def on cold attach
On Wed, Jul 25, 2018 at 08:39:26AM -0400, John Ferlan wrote:
On 07/25/2018 06:40 AM, Michal Privoznik wrote:
> On 07/16/2018 11:14 PM, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1559867
>>
>> When attaching a device to the domain we need to be sure
>> to use the correct domain definition (vm->def or vm->newDef)
>> when calling virDomainDeviceDefParse because the post parse
>> processing algorithms that may assign an address for the
>> device will use whatever domain definition was passed in.
>>
This effectively reduces AttachDevice(AFFECT_LIVE | AFFECT_CONFIG) to
two subsequent AttachDevice calls, thus possibly attaching two
different devices, making the AFFECT_CONFIG option pointless.
E.g. when hotplugging a network interface, it might end up both on a
different PCI slot and with a different MAC address, which I'm afraid
might break some use cases.
>> Additionally, some devices (SCSI hostdev and SCSI disk) use
>> algorithms that rely on knowing what already exists of the
>> other type when generating the new device's address. Using
>> the wrong VM definition could result in duplicated addresses.
>>
If suitablity for both live and persistent definition is a problem,
the address allocation code should take both domain definitions into
account and generate a single address for both device copies.
>> In the case of the bz, two hostdev's with no domain
address
>> provided were added to the running domain's config only.
>> However, the parsing algorithm used the live domain in
>> order to figure out the host device address resulting in
>> the same address being used and a subsequent start failing
>> due to duplicate address.
>>
>> Fix this by separating the checks/code into CONFIG and LIVE
>> processing using the correct definition for each block and
>> performing cleanup for both options as necessary.
>>
>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>> ---
>> src/qemu/qemu_driver.c | 52 +++++++++++++++++++-----------------------
>> 1 file changed, 23 insertions(+), 29 deletions(-)
As I said in my review of v2 I still consider this a waste of energy and
we might possibly end up having to revert this change because it breaks
someone's use case.
Jano
1:39 p.m.
New subject: [libvirt] [PATCH v3 3/3] qemu: Use the correct vm def on cold attach
On 07/25/2018 09:51 AM, Ján Tomko wrote:
On Wed, Jul 25, 2018 at 08:39:26AM -0400, John Ferlan wrote:
>
>
> On 07/25/2018 06:40 AM, Michal Privoznik wrote:
>> On 07/16/2018 11:14 PM, John Ferlan wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1559867
>>>
>>> When attaching a device to the domain we need to be sure
>>> to use the correct domain definition (vm->def or vm->newDef)
>>> when calling virDomainDeviceDefParse because the post parse
>>> processing algorithms that may assign an address for the
>>> device will use whatever domain definition was passed in.
>>>
This effectively reduces AttachDevice(AFFECT_LIVE | AFFECT_CONFIG) to
two subsequent AttachDevice calls, thus possibly attaching two
different devices, making the AFFECT_CONFIG option pointless.
Ironically, the v2 response you gave was:
"
IMO this is a step into the right direction - rather than tuning up the
device to be compatible with the live definition and hoping it will work
in the persistent definition is just naive.
"
Still what I did doesn't change the AttachDevice calls. Prior to my
changes and after my changes there is a qemuDomainAttachDeviceConfig for
CONFIG and a qemuDomainAttachDeviceLive for LIVE.
What the change does is separate the virDomainDeviceDefParse calls such
that the "config" one is made with the persistent def rather than the
live def for both.
Thus if some previous change updates persistent, then the next change to
just config doesn't miss that and perhaps duplicate something not
provided - in this case the <address... unit=0> was duplicated which
results in subsequent start failure.
So looking at the old and new code side by side - how exactly is new
code pointless? What subtle point am I missing?
E.g. when hotplugging a network interface, it might end up both on a
different PCI slot and with a different MAC address, which I'm afraid
might break some use cases.
An example would have greatly helped...
So, I assume you mean this:
# cat n1.xml
<interface type='network'>
<source network='default'/>
<model type='virtio'/>
</interface>
# virsh attach-device $dom n1.xml --live --config
Device attached successfully
# virsh dumpxml $dom | grep interface -A 10
...
<interface type='network'>
<mac address='52:54:00:8a:bb:ea'/>
<source network='default' bridge='virbr0'/>
<target dev='vnet1'/>
<model type='virtio'/>
<alias name='net1'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x0d'
function='0x0'/>
</interface>
...
# virsh dumpxml $dom --inactive | grep interface -A 10
...
<interface type='network'>
<mac address='52:54:00:be:29:3c'/>
<source network='default'/>
<model type='virtio'/>
<address type='pci' domain='0x0000' bus='0x00'
slot='0x0d'
function='0x0'/>
</interface>
...
whereas, previously the same MAC address would have been generated for
both live and config based on the current live def.
Is that it?
One other consideration. Prior to my changes. Assume the same n1.xml,
then attach-device --config only, check out the resulting address using
slot=0xd. Now use the --config --live - for the config guest you'll see
that some MAC gets added at slot=0x0e while the same MAC gets added to
slot=0x0d for the live guest. So I would contend this is no different
than my changes other than with the changes the next guest wouldn't have
the duplicated MAC from the live guest so the MAC being at a different
address shouldn't matter.
So the downside to libvirt generating a different MAC for live and
config ends up being is what? The guest will boot the next time, but
with a different MAC for the network device. If though a consumer
doesn't provide one, how much of a problem is that compared to not being
able to boot the guest because the <address ... unit=#> conflict for disks.
This is certainly one of those "competing interests" type problems for
generated data. I really don't know the "perfect answer" other than
telling customers to provide the specific things they want to avoid
these conflicts.
One could argue that if one wants a predictable MAC, then they should
provide it. Similar argument for the <address>. The one difference
between the two for me being it's not possible to boot the machine with
address conflicts; whereas, a change in an unprovided MAC is kind of a
WYSIWYG type problem.
>>> Additionally, some devices (SCSI hostdev and SCSI disk)
use
>>> algorithms that rely on knowing what already exists of the
>>> other type when generating the new device's address. Using
>>> the wrong VM definition could result in duplicated addresses.
>>>
If suitablity for both live and persistent definition is a problem,
the address allocation code should take both domain definitions into
account and generate a single address for both device copies.
Could be a real challenge to do both - the algorithms aren't exactly
"simple" now, especially when one has to take into account SCSI disks
and SCSI hostdevs "share" the same bus.
Not sure it'll work, but perhaps a different option - don't assign an
address to a device that's being cold plugged while the domain is
active. Although there's enough different types of addresses and devices
that I'm not sure that's feasible for each type.
>>> In the case of the bz, two hostdev's with no domain
address
>>> provided were added to the running domain's config only.
>>> However, the parsing algorithm used the live domain in
>>> order to figure out the host device address resulting in
>>> the same address being used and a subsequent start failing
>>> due to duplicate address.
>>>
>>> Fix this by separating the checks/code into CONFIG and LIVE
>>> processing using the correct definition for each block and
>>> performing cleanup for both options as necessary.
>>>
>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
>>> ---
>>> src/qemu/qemu_driver.c | 52 +++++++++++++++++++-----------------------
>>> 1 file changed, 23 insertions(+), 29 deletions(-)
As I said in my review of v2 I still consider this a waste of energy and
we might possibly end up having to revert this change because it breaks
someone's use case.
Jano
For reference, the v2 review:
https://www.redhat.com/archives/libvir-list/2018-June/msg01012.html
And there were differing opinions related to whether it was a waste of
energy. In the long run, IDC if this patch is reverted and the bz is
closed as impossible or won't fix or whatever magic wording needs to be
used if that's what you prefer/desire.
John
2:55 p.m.
ping.
Tks,
John
On 07/16/2018 05:14 PM, John Ferlan wrote:
v2:
https://www.redhat.com/archives/libvir-list/2018-July/msg00361.html
Differences to v2:
Patch1: NEW - As a result of code review the suggestion was to utilize
the virDomainDefCompatibleDevice in order to make the check more
generic to include <disk>'s which were also afflicted with the
same problem that I was trying to solve with the former patch1
just for <hostdev>'s. However, this led me down into the abyss of
more changes since <disk>'s have multiple <address
type='drive'...>
target bus types (IDE and SCSI). That means we need to have a
mechanism to pass the target bus along so that a SCSI drive
address doesn't inadvertently match an IDE drive address. All
that is complicated by the way virDomainDefHasDeviceAddress
iterates through all the device lists. Whether there are more
similar devices I'm assuming will fall out of code review.
Patch2: This moves the virDomainDefHasDeviceAddress into the more
common config checking virDomainDefCompatibleDevice method,
but now needs to also account for the disk bus issue.
This theoretically could be combined with Patch1, but keeping
them separate I would hope makes for simpler code review. I
could also move code out of virDomainDefHasDeviceAddressIterator
into patch2, but if just felt better in patch1.
Patch3: No changes were made (amazingly so).
In the end quite a bit more complicated
John Ferlan (3):
conf: Add @target_bus to virDomainDefHasDeviceAddress
qemu: Check for existing address when cold attach device
qemu: Use the correct vm def on cold attach
src/conf/domain_conf.c | 54 +++++++++++++++++++++++++++++++++++---
src/conf/domain_conf.h | 3 ++-
src/qemu/qemu_driver.c | 59 ++++++++++++++++--------------------------
3 files changed, 75 insertions(+), 41 deletions(-)
2312
days inactive
2321
days old
8 comments
3 participants
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik