8:21 a.m.
Commit v3.7.0-14-gc57f3fd2f8 prevented adding a <boot order='x'/>
element to an inactive domain with global <boot dev='...'/> element.
However, as a result of that change updating any device with boot order
would fail with 'boot order X is already used by another device', where
"another device" is in fact the device which is being updated.
To fix this we have to ignore the device which we're about to update
when checking for boot order conflicts.
https://bugzilla.redhat.com/show_bug.cgi?id=1546971
Jiri Denemark (3):
lxc: Drop useless check in live device update
Pass oldDev to virDomainDefCompatibleDevice on device update
qemu: Fix updating device with boot order
src/conf/domain_conf.c | 30 ++++++++++++++++++++++-------
src/conf/domain_conf.h | 3 ++-
src/lxc/lxc_driver.c | 18 +++++++++---------
src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++----------
4 files changed, 75 insertions(+), 27 deletions(-)
--
2.16.2
8:21 a.m.
New subject: [libvirt] [PATCH 1/3] lxc: Drop useless check in live device update
Checking the new device definition makes little sense when lxc driver
does not support live device update at all.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/lxc/lxc_driver.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 3d5b2254f2..4c6f09a435 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -4939,9 +4939,6 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
- goto endjob;
-
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("Unable to modify live devices"));
--
2.16.2
8:21 a.m.
New subject: [libvirt] [PATCH 2/3] Pass oldDev to virDomainDefCompatibleDevice on device update
When calling virDomainDefCompatibleDevice to check a new device during
device update, we need to pass the original device which is going to be
updated in addition to the new device. Otherwise, the function can
report false conflicts.
The new argument is currently ignored by virDomainDefCompatibleDevice,
but this will change in the following patch.
https://bugzilla.redhat.com/show_bug.cgi?id=1546971
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/conf/domain_conf.c | 3 ++-
src/conf/domain_conf.h | 3 ++-
src/lxc/lxc_driver.c | 15 +++++++++------
src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++----------
4 files changed, 54 insertions(+), 18 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f2ddde7a36..c71c28e8d2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27400,7 +27400,8 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def
ATTRIBUTE_UNUSED,
int
virDomainDefCompatibleDevice(virDomainDefPtr def,
- virDomainDeviceDefPtr dev)
+ virDomainDeviceDefPtr dev,
+ virDomainDeviceDefPtr oldDev ATTRIBUTE_UNUSED)
{
virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2350926e5b..368f16f3fb 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3014,7 +3014,8 @@ typedef enum {
} virDomainDeviceAction;
int virDomainDefCompatibleDevice(virDomainDefPtr def,
- virDomainDeviceDefPtr dev);
+ virDomainDeviceDefPtr dev,
+ virDomainDeviceDefPtr oldDev);
void virDomainRNGDefFree(virDomainRNGDefPtr def);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 4c6f09a435..fa6fc4643e 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3578,6 +3578,7 @@ lxcDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
{
int ret = -1;
virDomainNetDefPtr net;
+ virDomainDeviceDef oldDev = { .type = dev->type };
int idx;
switch (dev->type) {
@@ -3586,8 +3587,11 @@ lxcDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
goto cleanup;
- virDomainNetDefFree(vmdef->nets[idx]);
+ oldDev.data.net = vmdef->nets[idx];
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ return -1;
+ virDomainNetDefFree(vmdef->nets[idx]);
vmdef->nets[idx] = net;
dev->data.net = NULL;
ret = 0;
@@ -4820,7 +4824,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
if (!vmdef)
goto endjob;
- if (virDomainDefCompatibleDevice(vmdef, dev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, NULL) < 0)
goto endjob;
if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0)
@@ -4828,7 +4832,7 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
+ if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL) < 0)
goto endjob;
if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) <
0)
@@ -4931,9 +4935,8 @@ static int lxcDomainUpdateDeviceFlags(virDomainPtr dom,
if (!vmdef)
goto endjob;
- if (virDomainDefCompatibleDevice(vmdef, dev) < 0)
- goto endjob;
-
+ /* virDomainDefCompatibleDevice call is delayed until we know the
+ * device we're going to update. */
if ((ret = lxcDomainUpdateDeviceConfig(vmdef, dev)) < 0)
goto endjob;
}
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 14c39b2610..313d730c79 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7844,6 +7844,7 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
{
virDomainDiskDefPtr disk = dev->data.disk;
virDomainDiskDefPtr orig_disk = NULL;
+ virDomainDeviceDef oldDev = { .type = dev->type };
int ret = -1;
if (virDomainDiskTranslateSourcePool(disk) < 0)
@@ -7861,6 +7862,10 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
goto cleanup;
}
+ oldDev.data.disk = orig_disk;
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev) < 0)
+ goto cleanup;
+
if (!qemuDomainDiskChangeSupported(disk, orig_disk))
goto cleanup;
@@ -7903,19 +7908,36 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm,
bool force)
{
virQEMUDriverPtr driver = dom->conn->privateData;
+ virDomainDeviceDef oldDev = { .type = dev->type };
int ret = -1;
+ int idx;
switch ((virDomainDeviceType) dev->type) {
case VIR_DOMAIN_DEVICE_DISK:
qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, NULL);
ret = qemuDomainChangeDiskLive(vm, dev, driver, force);
break;
+
case VIR_DOMAIN_DEVICE_GRAPHICS:
+ if ((idx = qemuDomainFindGraphicsIndex(vm->def, dev->data.graphics) >=
0)) {
+ oldDev.data.graphics = vm->def->graphics[idx];
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev) < 0)
+ return -1;
+ }
+
ret = qemuDomainChangeGraphics(driver, vm, dev->data.graphics);
break;
+
case VIR_DOMAIN_DEVICE_NET:
+ if ((idx = virDomainNetFindIdx(vm->def, dev->data.net)) >= 0) {
+ oldDev.data.net = vm->def->nets[idx];
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev) < 0)
+ return -1;
+ }
+
ret = qemuDomainChangeNet(driver, vm, dev);
break;
+
case VIR_DOMAIN_DEVICE_FS:
case VIR_DOMAIN_DEVICE_INPUT:
case VIR_DOMAIN_DEVICE_SOUND:
@@ -8329,6 +8351,7 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
virDomainDiskDefPtr newDisk;
virDomainGraphicsDefPtr newGraphics;
virDomainNetDefPtr net;
+ virDomainDeviceDef oldDev = { .type = dev->type };
int pos;
switch ((virDomainDeviceType) dev->type) {
@@ -8340,6 +8363,10 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
return -1;
}
+ oldDev.data.disk = vmdef->disks[pos];
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ return -1;
+
virDomainDiskDefFree(vmdef->disks[pos]);
vmdef->disks[pos] = newDisk;
dev->data.disk = NULL;
@@ -8355,8 +8382,11 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
return -1;
}
- virDomainGraphicsDefFree(vmdef->graphics[pos]);
+ oldDev.data.graphics = vmdef->graphics[pos];
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ return -1;
+ virDomainGraphicsDefFree(vmdef->graphics[pos]);
vmdef->graphics[pos] = newGraphics;
dev->data.graphics = NULL;
break;
@@ -8366,8 +8396,11 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
if ((pos = virDomainNetFindIdx(vmdef, net)) < 0)
return -1;
- virDomainNetDefFree(vmdef->nets[pos]);
+ oldDev.data.net = vmdef->nets[pos];
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ return -1;
+ virDomainNetDefFree(vmdef->nets[pos]);
vmdef->nets[pos] = net;
dev->data.net = NULL;
break;
@@ -8454,7 +8487,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
if (!vmdef)
goto cleanup;
- if (virDomainDefCompatibleDevice(vmdef, dev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, NULL) < 0)
goto cleanup;
if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
parse_flags,
@@ -8463,7 +8496,7 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
+ if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL) < 0)
goto cleanup;
if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
@@ -8603,9 +8636,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
if (!vmdef)
goto endjob;
- if (virDomainDefCompatibleDevice(vmdef, dev) < 0)
- goto endjob;
-
+ /* virDomainDefCompatibleDevice call is delayed until we know the
+ * device we're going to update. */
if ((ret = qemuDomainUpdateDeviceConfig(vmdef, dev, caps,
parse_flags,
driver->xmlopt)) < 0)
@@ -8613,9 +8645,8 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy) < 0)
- goto endjob;
-
+ /* virDomainDefCompatibleDevice call is delayed until we know the
+ * device we're going to update. */
if ((ret = qemuDomainUpdateDeviceLive(vm, dev_copy, dom, force)) < 0)
goto endjob;
/*
--
2.16.2
8:21 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Fix updating device with boot order
Commit v3.7.0-14-gc57f3fd2f8 prevented adding a <boot order='x'/>
element to an inactive domain with global <boot dev='...'/> element.
However, as a result of that change updating any device with boot order
would fail with 'boot order X is already used by another device', where
"another device" is in fact the device which is being updated.
To fix this we have to ignore the device which we're about to update
when checking for boot order conflicts.
https://bugzilla.redhat.com/show_bug.cgi?id=1546971
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/conf/domain_conf.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c71c28e8d2..d96b012b98 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27381,18 +27381,30 @@ virDomainDeviceIsUSB(virDomainDeviceDefPtr dev)
return false;
}
+
+typedef struct _virDomainCompatibleDeviceData virDomainCompatibleDeviceData;
+typedef virDomainCompatibleDeviceData *virDomainCompatibleDeviceDataPtr;
+struct _virDomainCompatibleDeviceData {
+ virDomainDeviceInfoPtr newInfo;
+ virDomainDeviceInfoPtr oldInfo;
+};
+
static int
virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
virDomainDeviceInfoPtr info,
void *opaque)
{
- virDomainDeviceInfoPtr newinfo = opaque;
+ virDomainCompatibleDeviceDataPtr data = opaque;
- if (info->bootIndex == newinfo->bootIndex) {
+ /* Ignore the device we're about to update */
+ if (data->oldInfo == info)
+ return 0;
+
+ if (info->bootIndex == data->newInfo->bootIndex) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("boot order %u is already used by another device"),
- newinfo->bootIndex);
+ data->newInfo->bootIndex);
return -1;
}
return 0;
@@ -27401,9 +27413,12 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def
ATTRIBUTE_UNUSED,
int
virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
- virDomainDeviceDefPtr oldDev ATTRIBUTE_UNUSED)
+ virDomainDeviceDefPtr oldDev)
{
- virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
+ virDomainCompatibleDeviceData data = {
+ .newInfo = virDomainDeviceGetInfo(dev),
+ .oldInfo = virDomainDeviceGetInfo(oldDev),
+ };
if (!virDomainDefHasUSB(def) &&
def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
@@ -27414,7 +27429,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
return -1;
}
- if (info && info->bootIndex > 0) {
+ if (data.newInfo && data.newInfo->bootIndex > 0) {
if (def->os.nBootDevs > 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("per-device boot elements cannot be used"
@@ -27423,7 +27438,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
}
if (virDomainDeviceInfoIterate(def,
virDomainDeviceInfoCheckBootIndex,
- info) < 0)
+ &data) < 0)
return -1;
}
--
2.16.2
11:33 a.m.
New subject: [libvirt] [PATCH 3/3] qemu: Fix updating device with boot order
On 02/22/2018 09:21 AM, Jiri Denemark wrote:
Commit v3.7.0-14-gc57f3fd2f8 prevented adding a <boot
order='x'/>
element to an inactive domain with global <boot dev='...'/> element.
However, as a result of that change updating any device with boot order
would fail with 'boot order X is already used by another device', where
"another device" is in fact the device which is being updated.
To fix this we have to ignore the device which we're about to update
when checking for boot order conflicts.
https://bugzilla.redhat.com/show_bug.cgi?id=1546971
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/conf/domain_conf.c | 29 ++++++++++++++++++++++-------
1 file changed, 22 insertions(+), 7 deletions(-)
While chasing something else - I've tripped across this change...
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index c71c28e8d2..d96b012b98 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -27381,18 +27381,30 @@ virDomainDeviceIsUSB(virDomainDeviceDefPtr dev)
return false;
}
+
+typedef struct _virDomainCompatibleDeviceData virDomainCompatibleDeviceData;
+typedef virDomainCompatibleDeviceData *virDomainCompatibleDeviceDataPtr;
+struct _virDomainCompatibleDeviceData {
+ virDomainDeviceInfoPtr newInfo;
+ virDomainDeviceInfoPtr oldInfo;
+};
+
static int
virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def ATTRIBUTE_UNUSED,
virDomainDeviceDefPtr device ATTRIBUTE_UNUSED,
virDomainDeviceInfoPtr info,
void *opaque)
{
- virDomainDeviceInfoPtr newinfo = opaque;
+ virDomainCompatibleDeviceDataPtr data = opaque;
- if (info->bootIndex == newinfo->bootIndex) {
+ /* Ignore the device we're about to update */
+ if (data->oldInfo == info)
+ return 0;
+
+ if (info->bootIndex == data->newInfo->bootIndex) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("boot order %u is already used by another device"),
- newinfo->bootIndex);
+ data->newInfo->bootIndex);
return -1;
}
return 0;
@@ -27401,9 +27413,12 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def
ATTRIBUTE_UNUSED,
int
virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
- virDomainDeviceDefPtr oldDev ATTRIBUTE_UNUSED)
+ virDomainDeviceDefPtr oldDev)
{
- virDomainDeviceInfoPtr info = virDomainDeviceGetInfo(dev);
+ virDomainCompatibleDeviceData data = {
+ .newInfo = virDomainDeviceGetInfo(dev),
+ .oldInfo = virDomainDeviceGetInfo(oldDev),
+ };
I believe this has broken the ability to attach or update a CDROM for
both qemu and lxc as virDomainDefCompatibleDevice is called with a NULL
3rd parameter leading to virDomainDeviceGetInfo(NULL) being called
resulting in a fairly immediate coredump.
If I modify things to have .oldInfo = NULL, and then fill it in only if
@oldDev, that resolves the problem. Whether that's the right fix not
100% sure (I'm still trying to mentally dig out from a week away from work).
John
if (!virDomainDefHasUSB(def) &&
def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
@@ -27414,7 +27429,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
return -1;
}
- if (info && info->bootIndex > 0) {
+ if (data.newInfo && data.newInfo->bootIndex > 0) {
if (def->os.nBootDevs > 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("per-device boot elements cannot be used"
@@ -27423,7 +27438,7 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
}
if (virDomainDeviceInfoIterate(def,
virDomainDeviceInfoCheckBootIndex,
- info) < 0)
+ &data) < 0)
return -1;
}
3:16 a.m.
On 02/22/2018 03:21 PM, Jiri Denemark wrote:
Commit v3.7.0-14-gc57f3fd2f8 prevented adding a <boot
order='x'/>
element to an inactive domain with global <boot dev='...'/> element.
However, as a result of that change updating any device with boot order
would fail with 'boot order X is already used by another device', where
"another device" is in fact the device which is being updated.
To fix this we have to ignore the device which we're about to update
when checking for boot order conflicts.
https://bugzilla.redhat.com/show_bug.cgi?id=1546971
Jiri Denemark (3):
lxc: Drop useless check in live device update
Pass oldDev to virDomainDefCompatibleDevice on device update
qemu: Fix updating device with boot order
src/conf/domain_conf.c | 30 ++++++++++++++++++++++-------
src/conf/domain_conf.h | 3 ++-
src/lxc/lxc_driver.c | 18 +++++++++---------
src/qemu/qemu_driver.c | 51 ++++++++++++++++++++++++++++++++++++++++----------
4 files changed, 75 insertions(+), 27 deletions(-)
ACK
Michal
2458
days inactive
2464
days old
5 comments
3 participants
participants (3)
-
Jiri Denemark -
John Ferlan -
Michal Privoznik