5:21 a.m.
v2 of:
https://www.redhat.com/archives/libvir-list/2018-June/msg00971.html
Note that 1/3 is already reviewed (not pushed though, yet).
diff to v1:
- Fixed <interface/> hotplug issue raised during review of v1,
- Removed alias change checks for disks and interfaces since there's one
common place for that now.
Michal Privoznik (3):
qemuDomainUpdateDeviceFlags: Parse device as live if needed
conf: Reintroduce action to virDomainDefCompatibleDevice
conf: Forbid device alias change on device-update
src/conf/domain_conf.c | 14 +++++++++++++-
src/conf/domain_conf.h | 4 +++-
src/lxc/lxc_driver.c | 12 +++++++++---
src/qemu/qemu_domain.c | 8 --------
src/qemu/qemu_driver.c | 44 ++++++++++++++++++++++++++++++++------------
src/qemu/qemu_hotplug.c | 5 -----
6 files changed, 57 insertions(+), 30 deletions(-)
--
2.16.4
5:21 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemuDomainUpdateDeviceFlags: Parse device as live if needed
When updating device it's worth parsing live info too as users
might want to update it as well.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_driver.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 129bacdd34..e3fb4919a5 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -8631,7 +8631,7 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
int ret = -1;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
- unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
+ unsigned int parse_flags = 0;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
VIR_DOMAIN_AFFECT_CONFIG |
@@ -8653,15 +8653,19 @@ static int qemuDomainUpdateDeviceFlags(virDomainPtr dom,
if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
+ if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
+ goto endjob;
+
+ if ((flags & VIR_DOMAIN_AFFECT_CONFIG) &&
+ !(flags & VIR_DOMAIN_AFFECT_LIVE))
+ parse_flags |= VIR_DOMAIN_DEF_PARSE_INACTIVE;
+
dev = dev_copy = virDomainDeviceDefParse(xml, vm->def,
caps, driver->xmlopt,
parse_flags);
if (dev == NULL)
goto endjob;
- if (virDomainObjUpdateModificationImpact(vm, &flags) < 0)
- goto endjob;
-
if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
flags & VIR_DOMAIN_AFFECT_LIVE) {
/* If we are affecting both CONFIG and LIVE
--
2.16.4
9:26 a.m.
New subject: [libvirt] [PATCH v2 1/3] qemuDomainUpdateDeviceFlags: Parse device as live if needed
On 06/26/2018 06:21 AM, Michal Privoznik wrote:
When updating device it's worth parsing live info too as users
might want to update it as well.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
^^ Clean this up - I assume you added the R-by and the extra which
caused your automatic one to be confused and thus add another for the
send - just check to see if it's clean in whatever is pushed...
John
---
src/qemu/qemu_driver.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
5:21 a.m.
New subject: [libvirt] [PATCH v2 2/3] conf: Reintroduce action to virDomainDefCompatibleDevice
This was lost in c57f3fd2f8999d17e01. But now we are going to
need it again.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 3 ++-
src/conf/domain_conf.h | 3 ++-
src/lxc/lxc_driver.c | 9 ++++++---
src/qemu/qemu_driver.c | 24 ++++++++++++++++--------
4 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d8cb7f37f3..93cfca351c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28205,7 +28205,8 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def
ATTRIBUTE_UNUSED,
int
virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
- virDomainDeviceDefPtr oldDev)
+ virDomainDeviceDefPtr oldDev,
+ virDomainDeviceAction action ATTRIBUTE_UNUSED)
{
virDomainCompatibleDeviceData data = {
.newInfo = virDomainDeviceGetInfo(dev),
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0924fc4f3c..f33405e097 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3106,7 +3106,8 @@ typedef enum {
int virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
- virDomainDeviceDefPtr oldDev);
+ virDomainDeviceDefPtr oldDev,
+ virDomainDeviceAction action);
void virDomainRNGDefFree(virDomainRNGDefPtr def);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index cfb431488d..850b12726b 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3549,7 +3549,8 @@ lxcDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
goto cleanup;
oldDev.data.net = vmdef->nets[idx];
- if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
virDomainNetDefFree(vmdef->nets[idx]);
@@ -4785,7 +4786,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
if (!vmdef)
goto endjob;
- if (virDomainDefCompatibleDevice(vmdef, dev, NULL) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
goto endjob;
if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0)
@@ -4793,7 +4795,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL) < 0)
+ if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
goto endjob;
if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) <
0)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e3fb4919a5..8c0c681c4d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7885,7 +7885,8 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
}
oldDev.data.disk = orig_disk;
- if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
goto cleanup;
if (!qemuDomainDiskChangeSupported(disk, orig_disk))
@@ -7943,7 +7944,8 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm,
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)
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
}
@@ -7953,7 +7955,8 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm,
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)
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
}
@@ -8406,7 +8409,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
}
oldDev.data.disk = vmdef->disks[pos];
- if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
virDomainDiskDefFree(vmdef->disks[pos]);
@@ -8425,7 +8429,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
}
oldDev.data.graphics = vmdef->graphics[pos];
- if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
virDomainGraphicsDefFree(vmdef->graphics[pos]);
@@ -8439,7 +8444,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
return -1;
oldDev.data.net = vmdef->nets[pos];
- if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
virDomainNetDefFree(vmdef->nets[pos]);
@@ -8530,7 +8536,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
if (!vmdef)
goto cleanup;
- if (virDomainDefCompatibleDevice(vmdef, dev, NULL) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
goto cleanup;
if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
parse_flags,
@@ -8539,7 +8546,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL) < 0)
+ if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
goto cleanup;
if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
--
2.16.4
9:30 a.m.
New subject: [libvirt] [PATCH v2 2/3] conf: Reintroduce action to virDomainDefCompatibleDevice
On 06/26/2018 06:21 AM, Michal Privoznik wrote:
This was lost in c57f3fd2f8999d17e01. But now we are going to
need it again.
Looks like that commit also "lost" the compatible device check on detach
too (e.g. ACTION_DETACH) without explanation. Whether at the time it
then became the last consumer of virDomainDeviceAction is another question.
So, since you're bringing this back to life - should those checks be
reinstated into the Detach code (lxc, qemu)? If not, surely
ACTION_DETACH then still isn't used.
Can I assume you tested bz1439991 with the new condition?
The restoration of what's been done thus far looks fine, just need some
more details...
John
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 3 ++-
src/conf/domain_conf.h | 3 ++-
src/lxc/lxc_driver.c | 9 ++++++---
src/qemu/qemu_driver.c | 24 ++++++++++++++++--------
4 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d8cb7f37f3..93cfca351c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28205,7 +28205,8 @@ virDomainDeviceInfoCheckBootIndex(virDomainDefPtr def
ATTRIBUTE_UNUSED,
int
virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
- virDomainDeviceDefPtr oldDev)
+ virDomainDeviceDefPtr oldDev,
+ virDomainDeviceAction action ATTRIBUTE_UNUSED)
{
virDomainCompatibleDeviceData data = {
.newInfo = virDomainDeviceGetInfo(dev),
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 0924fc4f3c..f33405e097 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3106,7 +3106,8 @@ typedef enum {
int virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
- virDomainDeviceDefPtr oldDev);
+ virDomainDeviceDefPtr oldDev,
+ virDomainDeviceAction action);
void virDomainRNGDefFree(virDomainRNGDefPtr def);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index cfb431488d..850b12726b 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3549,7 +3549,8 @@ lxcDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
goto cleanup;
oldDev.data.net = vmdef->nets[idx];
- if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
virDomainNetDefFree(vmdef->nets[idx]);
@@ -4785,7 +4786,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
if (!vmdef)
goto endjob;
- if (virDomainDefCompatibleDevice(vmdef, dev, NULL) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
goto endjob;
if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0)
@@ -4793,7 +4795,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL) < 0)
+ if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
goto endjob;
if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) <
0)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e3fb4919a5..8c0c681c4d 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7885,7 +7885,8 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
}
oldDev.data.disk = orig_disk;
- if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
goto cleanup;
if (!qemuDomainDiskChangeSupported(disk, orig_disk))
@@ -7943,7 +7944,8 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm,
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)
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
}
@@ -7953,7 +7955,8 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm,
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)
+ if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
}
@@ -8406,7 +8409,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
}
oldDev.data.disk = vmdef->disks[pos];
- if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
virDomainDiskDefFree(vmdef->disks[pos]);
@@ -8425,7 +8429,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
}
oldDev.data.graphics = vmdef->graphics[pos];
- if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
virDomainGraphicsDefFree(vmdef->graphics[pos]);
@@ -8439,7 +8444,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
return -1;
oldDev.data.net = vmdef->nets[pos];
- if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
return -1;
virDomainNetDefFree(vmdef->nets[pos]);
@@ -8530,7 +8536,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
if (!vmdef)
goto cleanup;
- if (virDomainDefCompatibleDevice(vmdef, dev, NULL) < 0)
+ if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
goto cleanup;
if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
parse_flags,
@@ -8539,7 +8546,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
}
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
- if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL) < 0)
+ if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
goto cleanup;
if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
10:35 a.m.
New subject: [libvirt] [PATCH v2 2/3] conf: Reintroduce action to virDomainDefCompatibleDevice
On 06/26/2018 04:30 PM, John Ferlan wrote:
On 06/26/2018 06:21 AM, Michal Privoznik wrote:
> This was lost in c57f3fd2f8999d17e01. But now we are going to
> need it again.
Looks like that commit also "lost" the compatible device check on detach
too (e.g. ACTION_DETACH) without explanation. Whether at the time it
then became the last consumer of virDomainDeviceAction is another question.
There's no need to check for device compatibility on device detach, is
there? The virDomainDefCompatibleDevice() merely checks whether new
device that we are trying to hot-plug or change would not break
assumptions made by other parts of domain config. These can't be broken
if we're removing a device.
So, since you're bringing this back to life - should those checks be
reinstated into the Detach code (lxc, qemu)? If not, surely
ACTION_DETACH then still isn't used.
Yes, it is unused. I can remove it, although for completion sake I
rather have it in.
Can I assume you tested bz1439991 with the new condition?
Yes you can. I've tested this.
Michal
10:54 a.m.
New subject: [libvirt] [PATCH v2 2/3] conf: Reintroduce action to virDomainDefCompatibleDevice
On 06/26/2018 11:35 AM, Michal Privoznik wrote:
On 06/26/2018 04:30 PM, John Ferlan wrote:
>
>
> On 06/26/2018 06:21 AM, Michal Privoznik wrote:
>> This was lost in c57f3fd2f8999d17e01. But now we are going to
>> need it again.
>
> Looks like that commit also "lost" the compatible device check on detach
> too (e.g. ACTION_DETACH) without explanation. Whether at the time it
> then became the last consumer of virDomainDeviceAction is another question.
There's no need to check for device compatibility on device detach, is
there? The virDomainDefCompatibleDevice() merely checks whether new
device that we are trying to hot-plug or change would not break
assumptions made by other parts of domain config. These can't be broken
if we're removing a device.
Well it was there at some point in time and it's removal (or reason
therein) wasn't provided in either the bz or the commit message from the
referenced patch. So that led me down the path of curiosity.
Maybe use this opportunity to add to the commit message that restoring
the DETACH checks isn't necessary for the reasons you've outlined
although it's not that important.
>
> So, since you're bringing this back to life - should those checks be
> reinstated into the Detach code (lxc, qemu)? If not, surely
> ACTION_DETACH then still isn't used.
Yes, it is unused. I can remove it, although for completion sake I
rather have it in.
No need to remove it now, but since it's not used, perhaps eventually
some trivial patch will wind it's way through the system. When that
patch makes it's way through maybe it can have the explanation for why
DETACH checking wasn't necessary.
>
> Can I assume you tested bz1439991 with the new condition?
>
Yes you can. I've tested this.
Michal
OK, so considered this patch,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
9:45 a.m.
New subject: [libvirt] [PATCH v2 2/3] conf: Reintroduce action to virDomainDefCompatibleDevice
On 06/26/2018 05:54 PM, John Ferlan wrote:
<snip/>
OK, so considered this patch,
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
Thanks, I've pushed these since they are a bug fix.
Michal
5:21 a.m.
New subject: [libvirt] [PATCH v2 3/3] conf: Forbid device alias change on device-update
https://bugzilla.redhat.com/show_bug.cgi?id=1585108
When updating a live device users might pass different alias than
the one the device has. Currently, this is silently ignored which
goes against our behaviour for other parts of the device where we
explicitly allow only certain changes and error out loudly on
anything else.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 13 ++++++++++++-
src/conf/domain_conf.h | 3 ++-
src/lxc/lxc_driver.c | 9 ++++++---
src/qemu/qemu_domain.c | 8 --------
src/qemu/qemu_driver.c | 24 ++++++++++++++++--------
src/qemu/qemu_hotplug.c | 5 -----
6 files changed, 36 insertions(+), 26 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 93cfca351c..b8b53450fa 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28206,7 +28206,8 @@ int
virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
virDomainDeviceDefPtr oldDev,
- virDomainDeviceAction action ATTRIBUTE_UNUSED)
+ virDomainDeviceAction action,
+ bool live)
{
virDomainCompatibleDeviceData data = {
.newInfo = virDomainDeviceGetInfo(dev),
@@ -28216,6 +28217,16 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
if (oldDev)
data.oldInfo = virDomainDeviceGetInfo(oldDev);
+ if (action == VIR_DOMAIN_DEVICE_ACTION_UPDATE &&
+ live &&
+ ((!!data.newInfo != !!data.oldInfo) ||
+ (data.newInfo && data.oldInfo &&
+ STRNEQ_NULLABLE(data.newInfo->alias, data.oldInfo->alias)))) {
+ virReportError(VIR_ERR_OPERATION_DENIED, "%s",
+ _("changing device alias is not allowed"));
+ return -1;
+ }
+
if (!virDomainDefHasUSB(def) &&
def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
virDomainDeviceIsUSB(dev)) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index f33405e097..71437dc485 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3107,7 +3107,8 @@ typedef enum {
int virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
virDomainDeviceDefPtr oldDev,
- virDomainDeviceAction action);
+ virDomainDeviceAction action,
+ bool live);
void virDomainRNGDefFree(virDomainRNGDefPtr def);
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 850b12726b..8c02f888f4 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -3550,7 +3550,8 @@ lxcDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
oldDev.data.net = vmdef->nets[idx];
if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
- VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ false) < 0)
return -1;
virDomainNetDefFree(vmdef->nets[idx]);
@@ -4787,7 +4788,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
goto endjob;
if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
- VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH,
+ false) < 0)
goto endjob;
if ((ret = lxcDomainAttachDeviceConfig(vmdef, dev)) < 0)
@@ -4796,7 +4798,8 @@ static int lxcDomainAttachDeviceFlags(virDomainPtr dom,
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
- VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH,
+ true) < 0)
goto endjob;
if ((ret = lxcDomainAttachDeviceLive(dom->conn, driver, vm, dev_copy)) <
0)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6d203e1f2e..d750f3382a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8613,14 +8613,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
return false;
}
- if (disk->info.alias &&
- STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
- _("cannot modify field '%s' of the disk"),
- "alias");
- return false;
- }
-
CHECK_EQ(info.bootIndex, "boot order", true);
CHECK_EQ(rawio, "rawio", true);
CHECK_EQ(sgio, "sgio", true);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8c0c681c4d..21c97feed8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7886,7 +7886,8 @@ qemuDomainChangeDiskLive(virDomainObjPtr vm,
oldDev.data.disk = orig_disk;
if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
- VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ true) < 0)
goto cleanup;
if (!qemuDomainDiskChangeSupported(disk, orig_disk))
@@ -7945,7 +7946,8 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm,
if ((idx = qemuDomainFindGraphicsIndex(vm->def, dev->data.graphics)) >=
0) {
oldDev.data.graphics = vm->def->graphics[idx];
if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
- VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ true) < 0)
return -1;
}
@@ -7956,7 +7958,8 @@ qemuDomainUpdateDeviceLive(virDomainObjPtr vm,
if ((idx = virDomainNetFindIdx(vm->def, dev->data.net)) >= 0) {
oldDev.data.net = vm->def->nets[idx];
if (virDomainDefCompatibleDevice(vm->def, dev, &oldDev,
- VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ true) < 0)
return -1;
}
@@ -8410,7 +8413,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
oldDev.data.disk = vmdef->disks[pos];
if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
- VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ false) < 0)
return -1;
virDomainDiskDefFree(vmdef->disks[pos]);
@@ -8430,7 +8434,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
oldDev.data.graphics = vmdef->graphics[pos];
if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
- VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ false) < 0)
return -1;
virDomainGraphicsDefFree(vmdef->graphics[pos]);
@@ -8445,7 +8450,8 @@ qemuDomainUpdateDeviceConfig(virDomainDefPtr vmdef,
oldDev.data.net = vmdef->nets[pos];
if (virDomainDefCompatibleDevice(vmdef, dev, &oldDev,
- VIR_DOMAIN_DEVICE_ACTION_UPDATE) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_UPDATE,
+ false) < 0)
return -1;
virDomainNetDefFree(vmdef->nets[pos]);
@@ -8537,7 +8543,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
goto cleanup;
if (virDomainDefCompatibleDevice(vmdef, dev, NULL,
- VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH,
+ false) < 0)
goto cleanup;
if ((ret = qemuDomainAttachDeviceConfig(vmdef, dev, caps,
parse_flags,
@@ -8547,7 +8554,8 @@ qemuDomainAttachDeviceLiveAndConfig(virDomainObjPtr vm,
if (flags & VIR_DOMAIN_AFFECT_LIVE) {
if (virDomainDefCompatibleDevice(vm->def, dev_copy, NULL,
- VIR_DOMAIN_DEVICE_ACTION_ATTACH) < 0)
+ VIR_DOMAIN_DEVICE_ACTION_ATTACH,
+ true) < 0)
goto cleanup;
if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7a1bbc7c8c..a8991800b3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3193,11 +3193,6 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
if (!newdev->info.alias &&
VIR_STRDUP(newdev->info.alias, olddev->info.alias) < 0)
goto cleanup;
- if (STRNEQ_NULLABLE(olddev->info.alias, newdev->info.alias)) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("cannot modify network device alias"));
- goto cleanup;
- }
if (olddev->info.rombar != newdev->info.rombar) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("cannot modify network device rom bar setting"));
--
2.16.4
10:04 a.m.
New subject: [libvirt] [PATCH v2 3/3] conf: Forbid device alias change on device-update
On 06/26/2018 06:21 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1585108
When updating a live device users might pass different alias than
the one the device has. Currently, this is silently ignored which
goes against our behaviour for other parts of the device where we
explicitly allow only certain changes and error out loudly on
anything else.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/conf/domain_conf.c | 13 ++++++++++++-
src/conf/domain_conf.h | 3 ++-
src/lxc/lxc_driver.c | 9 ++++++---
src/qemu/qemu_domain.c | 8 --------
src/qemu/qemu_driver.c | 24 ++++++++++++++++--------
src/qemu/qemu_hotplug.c | 5 -----
6 files changed, 36 insertions(+), 26 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
NB: I've left a couple of "my review notes" below for your consideration
or thoughts... They are somewhat tangents to the changes, but may give
you reason to add a comment or two somewhere if you feel compelled to do so.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 93cfca351c..b8b53450fa 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -28206,7 +28206,8 @@ int
virDomainDefCompatibleDevice(virDomainDefPtr def,
virDomainDeviceDefPtr dev,
virDomainDeviceDefPtr oldDev,
- virDomainDeviceAction action ATTRIBUTE_UNUSED)
+ virDomainDeviceAction action,
+ bool live)
{
virDomainCompatibleDeviceData data = {
.newInfo = virDomainDeviceGetInfo(dev),
@@ -28216,6 +28217,16 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
if (oldDev)
data.oldInfo = virDomainDeviceGetInfo(oldDev);
+ if (action == VIR_DOMAIN_DEVICE_ACTION_UPDATE &&
+ live &&
+ ((!!data.newInfo != !!data.oldInfo) ||
+ (data.newInfo && data.oldInfo &&
+ STRNEQ_NULLABLE(data.newInfo->alias, data.oldInfo->alias)))) {
+ virReportError(VIR_ERR_OPERATION_DENIED, "%s",
+ _("changing device alias is not allowed"));
+ return -1;
+ }
+
I'd assume that this isn't affected by whether or not REMOVE was
supplied since we seem to allow a lot less to match for REMOVE and I
wouldn't think alias really matters at that point.
I assume what happens is people use the same XML for attach as they use
for remove and thus don't provide alias for either. Update - yeah,
that's a different scenario.
if (!virDomainDefHasUSB(def) &&
def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
virDomainDeviceIsUSB(dev)) {
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 6d203e1f2e..d750f3382a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8613,14 +8613,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
return false;
}
Perhaps leaving the breadcrumb (e.g. comment) here or at the top of the
function indicating alias is checked in virDomainDefCompatibleDevice.
That then at least "matches" what the function description indicates and
the reason why alias isn't specifically checked.
- if (disk->info.alias &&
- STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
- _("cannot modify field '%s' of the disk"),
- "alias");
- return false;
- }
-
CHECK_EQ(info.bootIndex, "boot order", true);
CHECK_EQ(rawio, "rawio", true);
CHECK_EQ(sgio, "sgio", true);
[...]
if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7a1bbc7c8c..a8991800b3 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3193,11 +3193,6 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
if (!newdev->info.alias &&
VIR_STRDUP(newdev->info.alias, olddev->info.alias) < 0)
goto cleanup;
I'll casually note that based on what I showed from the last review the
the comment at the top of virDomainDeviceInfoCopy doesn't seem to ring
true and thus if virDomainDeviceAddressIsValid does return 0 (for
multiple reasons) it would cause virDomainDeviceInfoCopy to overwrite
alias, romfile, and loadparm.
But that's unrelated to your code.
This at least takes care of the empty alias case, which is expected.
Whether you decide to note that virDomainDefCompatibleDevice ensures the
provided alias matches - is your call. Since there is only one caller
to this function, removing this check is fine because of that call
(hence the hedge whether to note it or not so that some future change
realizes that alias is checked elsewhere).
John
- if (STRNEQ_NULLABLE(olddev->info.alias,
newdev->info.alias)) {
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("cannot modify network device alias"));
- goto cleanup;
- }
if (olddev->info.rombar != newdev->info.rombar) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("cannot modify network device rom bar setting"));
10:47 a.m.
New subject: [libvirt] [PATCH v2 3/3] conf: Forbid device alias change on device-update
On 06/26/2018 05:04 PM, John Ferlan wrote:
On 06/26/2018 06:21 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1585108
>
> When updating a live device users might pass different alias than
> the one the device has. Currently, this is silently ignored which
> goes against our behaviour for other parts of the device where we
> explicitly allow only certain changes and error out loudly on
> anything else.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/conf/domain_conf.c | 13 ++++++++++++-
> src/conf/domain_conf.h | 3 ++-
> src/lxc/lxc_driver.c | 9 ++++++---
> src/qemu/qemu_domain.c | 8 --------
> src/qemu/qemu_driver.c | 24 ++++++++++++++++--------
> src/qemu/qemu_hotplug.c | 5 -----
> 6 files changed, 36 insertions(+), 26 deletions(-)
>
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
NB: I've left a couple of "my review notes" below for your consideration
or thoughts... They are somewhat tangents to the changes, but may give
you reason to add a comment or two somewhere if you feel compelled to do so.
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 93cfca351c..b8b53450fa 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -28206,7 +28206,8 @@ int
> virDomainDefCompatibleDevice(virDomainDefPtr def,
> virDomainDeviceDefPtr dev,
> virDomainDeviceDefPtr oldDev,
> - virDomainDeviceAction action ATTRIBUTE_UNUSED)
> + virDomainDeviceAction action,
> + bool live)
> {
> virDomainCompatibleDeviceData data = {
> .newInfo = virDomainDeviceGetInfo(dev),
> @@ -28216,6 +28217,16 @@ virDomainDefCompatibleDevice(virDomainDefPtr def,
> if (oldDev)
> data.oldInfo = virDomainDeviceGetInfo(oldDev);
>
> + if (action == VIR_DOMAIN_DEVICE_ACTION_UPDATE &&
> + live &&
> + ((!!data.newInfo != !!data.oldInfo) ||
> + (data.newInfo && data.oldInfo &&
> + STRNEQ_NULLABLE(data.newInfo->alias, data.oldInfo->alias)))) {
> + virReportError(VIR_ERR_OPERATION_DENIED, "%s",
> + _("changing device alias is not allowed"));
> + return -1;
> + }
> +
I'd assume that this isn't affected by whether or not REMOVE was
supplied since we seem to allow a lot less to match for REMOVE and I
wouldn't think alias really matters at that point.
Yes. Users are required to pass full device XML for detach. To cite from
virDomainDetachDeviceFlags() documentation:
The supplied XML description of the device should be as specific
as its definition in the domain XML. The set of attributes used
to match the device are internal to the drivers. Using a partial definition,
or attempting to detach a device that is not present in the domain XML,
but shares some specific attributes with one that is present,
may lead to unexpected results.
So at REMOVE it doesn't really matter if alias matches or not. We can
change the code to require matching alias, but I think that is another
issue orthogonal to this one ;-)
I assume what happens is people use the same XML for attach as they use
for remove and thus don't provide alias for either. Update - yeah,
that's a different scenario.
Yep, because of the reasoning I provided in one of the replies to
previous version. Users provide us with new device XML and any
difference to old XML must be treated as request for change. But we
can't change some things for live XML (e.g. alias), therefore we have to
check for them explicitly and error out.
> if (!virDomainDefHasUSB(def) &&
> def->os.type != VIR_DOMAIN_OSTYPE_EXE &&
> virDomainDeviceIsUSB(dev)) {
[...]
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6d203e1f2e..d750f3382a 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8613,14 +8613,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
> return false;
> }
>
Perhaps leaving the breadcrumb (e.g. comment) here or at the top of the
function indicating alias is checked in virDomainDefCompatibleDevice.
That then at least "matches" what the function description indicates and
the reason why alias isn't specifically checked.
Okay, done.
> - if (disk->info.alias &&
> - STRNEQ_NULLABLE(disk->info.alias, orig_disk->info.alias)) {
> - virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> - _("cannot modify field '%s' of the disk"),
> - "alias");
> - return false;
> - }
> -
> CHECK_EQ(info.bootIndex, "boot order", true);
> CHECK_EQ(rawio, "rawio", true);
> CHECK_EQ(sgio, "sgio", true);
[...]
>
> if ((ret = qemuDomainAttachDeviceLive(vm, dev_copy, driver)) < 0)
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7a1bbc7c8c..a8991800b3 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3193,11 +3193,6 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
> if (!newdev->info.alias &&
> VIR_STRDUP(newdev->info.alias, olddev->info.alias) < 0)
> goto cleanup;
I'll casually note that based on what I showed from the last review the
the comment at the top of virDomainDeviceInfoCopy doesn't seem to ring
true and thus if virDomainDeviceAddressIsValid does return 0 (for
multiple reasons) it would cause virDomainDeviceInfoCopy to overwrite
alias, romfile, and loadparm.
But that's unrelated to your code.
This at least takes care of the empty alias case, which is expected.
Whether you decide to note that virDomainDefCompatibleDevice ensures the
provided alias matches - is your call. Since there is only one caller
to this function, removing this check is fine because of that call
(hence the hedge whether to note it or not so that some future change
realizes that alias is checked elsewhere).
Yup, I'll put the comment here too.
Thanks,
Michal
2340
days inactive
2341
days old
10 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik