This was lost in c57f3fd2f8999d17e01. But now we are going to
need it again.
Signed-off-by: Michal Privoznik <mprivozn@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com> John -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 06/26/2018 05:54 PM, John Ferlan wrote: > <snip/> > OK, so considered this patch, > > Reviewed-by: John Ferlan <jferlan@redhat.com> Thanks, I've pushed these since they are a bug fix. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.