[libvirt] [PATCH 0/2] Fix crash from live attach of device (for 4.1.0)
Fix is in patch 1/2 - if virDomainDefCompatibleDevice is called with a NULL @oldDev, then call to virDomainDeviceGetInfo would fail rather spectacularly. Patch 2 extends the similar fix/adjustment from the update path to the add path for the cdrom/floppy since the add path since it's possible to use attach device as well as update device. John Ferlan (2): conf: Fix crash in virDomainDefCompatibleDevice qemu: Call virDomainDefCompatibleDevice when live add cdrom/floppy src/conf/domain_conf.c | 5 ++++- src/qemu/qemu_hotplug.c | 8 ++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) -- 2.13.6
Commit id 'edae027c' blindly assumed that the passed @oldDev parameter would not be NULL when calling virDomainDeviceGetInfo; however, commit id 'b6a264e8' passed NULL for AttachDevice callers under the premise that there wouldn't be a device to check/update against. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d96b012b9..fcafc8b2f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27417,9 +27417,12 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, { virDomainCompatibleDeviceData data = { .newInfo = virDomainDeviceGetInfo(dev), - .oldInfo = virDomainDeviceGetInfo(oldDev), + .oldInfo = NULL, }; + if (oldDev) + data.oldInfo = virDomainDeviceGetInfo(oldDev); + if (!virDomainDefHasUSB(def) && def->os.type != VIR_DOMAIN_OSTYPE_EXE && virDomainDeviceIsUSB(dev)) { -- 2.13.6
On Thu, Mar 01, 2018 at 08:03:38 -0500, John Ferlan wrote:
Commit id 'edae027c' blindly assumed that the passed @oldDev parameter would not be NULL when calling virDomainDeviceGetInfo; however, commit id 'b6a264e8' passed NULL for AttachDevice callers under the premise that there wouldn't be a device to check/update against.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d96b012b9..fcafc8b2f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -27417,9 +27417,12 @@ virDomainDefCompatibleDevice(virDomainDefPtr def, { virDomainCompatibleDeviceData data = { .newInfo = virDomainDeviceGetInfo(dev), - .oldInfo = virDomainDeviceGetInfo(oldDev), + .oldInfo = NULL, };
+ if (oldDev) + data.oldInfo = virDomainDeviceGetInfo(oldDev); + if (!virDomainDefHasUSB(def) && def->os.type != VIR_DOMAIN_OSTYPE_EXE && virDomainDeviceIsUSB(dev)) {
Oops, it was supposed to be like this as oldDev is explicitly optional. Not sure where I lost this... ACK, thanks. Jirka
Prior to calling qemuDomainChangeEjectableMedia when adding the cdrom/floppy to the guest, make a second pass at the virDomainDefCompatibleDevice with the old_disk that's being updated by the call. This is similar to the qemuDomainChangeDiskLive path from qemuDomainUpdateDeviceLive prior to making the same call to qemuDomainChangeEjectableMedia when the cdrom/floppy is updated rather than added. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e0a5300f0..4fbc0f48c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -697,6 +697,7 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, size_t i; virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; + virDomainDeviceDef orig_dev = { .type = dev->type }; int ret = -1; if (STRNEQ_NULLABLE(virDomainDiskGetDriver(disk), "qemu")) { @@ -732,6 +733,13 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, goto cleanup; } + /* Although called in qemuDomainAttachDeviceLiveAndConfig, now we + * know the orig_disk/dev so let's make the additional check for + * boot order checking */ + orig_dev.data.disk = orig_disk; + if (virDomainDefCompatibleDevice(vm->def, dev, &orig_dev) < 0) + goto cleanup; + if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, disk->src, false) < 0) goto cleanup; -- 2.13.6
On Thu, Mar 01, 2018 at 08:03:39 -0500, John Ferlan wrote:
Prior to calling qemuDomainChangeEjectableMedia when adding the cdrom/floppy to the guest, make a second pass at the virDomainDefCompatibleDevice with the old_disk that's being updated by the call.
This is similar to the qemuDomainChangeDiskLive path from qemuDomainUpdateDeviceLive prior to making the same call to qemuDomainChangeEjectableMedia when the cdrom/floppy is updated rather than added.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_hotplug.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e0a5300f0..4fbc0f48c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -697,6 +697,7 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, size_t i; virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; + virDomainDeviceDef orig_dev = { .type = dev->type }; int ret = -1;
if (STRNEQ_NULLABLE(virDomainDiskGetDriver(disk), "qemu")) { @@ -732,6 +733,13 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriverPtr driver, goto cleanup; }
+ /* Although called in qemuDomainAttachDeviceLiveAndConfig, now we + * know the orig_disk/dev so let's make the additional check for + * boot order checking */ + orig_dev.data.disk = orig_disk; + if (virDomainDefCompatibleDevice(vm->def, dev, &orig_dev) < 0) + goto cleanup; + if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk, disk->src, false) < 0) goto cleanup;
This change will have no effect. Calling virDomainDefCompatibleDevice with oldDev != NULL makes the check less strict (the oldDev will be ignored when checking for conflicts) so if virDomainDefCompatibleDevice with NULL oldDev did not complain about any conflict, this second call will not complain either. Moreover, if the new device configuration changed more than source (e.g., it changed boot order too), the first virDomainDefCompatibleDevice call will report conflict with the original version of the disk even though the second one which passes non-NULL oldDev would have ignored this conflict. Fixing this would require us to skip the first call to virDomainDefCompatibleDevice for cdrom and floppy devices, but I think it's actually fine if AttachDevice refuses to update anything but the source (since the code really cares only about the source). That said, I don't think there's anything we need to fix here. Jirka
participants (2)
-
Jiri Denemark -
John Ferlan