[libvirt] [PATCH v2 0/7] Allow startupPolicy change on update-device
So, as Peter pointed out, we may want to updated startupPolicy for live domains too. That's what patches 2/7-7/7 do. Michal Privoznik (7): qemuDomainUpdateDeviceConfig: Allow startupPolicy update, yet again qemu: s/qemuDomainChangeDiskMediaLive/qemuDomainChangeDiskLive/ qemu_domain: Introduce qemuDomainDiskSourceDiffers qemuDomainChangeDiskLive: rework slightly qemu: s/virDomainDiskDiffersSourceOnly/qemuDomainDiskChangeSupported/ qemuDomainDiskChangeSupported: Fill in missing checks qemuDomainChangeDiskLive: Allow startupPolicy change src/conf/domain_conf.c | 127 ---------------------------------- src/conf/domain_conf.h | 2 - src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 7 ++ src/qemu/qemu_driver.c | 80 ++++++++++++--------- 6 files changed, 232 insertions(+), 161 deletions(-) -- 2.4.6
https://bugzilla.redhat.com/show_bug.cgi?id=1159219 So, in 11e058ca589808bd I've tried to make UpdateDevice update startupPolicy too. And it worked well until somebody came around and pushed d0dc6c036914da which accidentally removed my contribution. Redo my commit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fcf86b6..fadead5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8356,6 +8356,7 @@ qemuDomainUpdateDeviceConfig(virQEMUCapsPtr qemuCaps, * We allow updating src/type//driverType/cachemode/ */ orig->cachemode = disk->cachemode; + orig->startupPolicy = disk->startupPolicy; virStorageSourceFree(orig->src); orig->src = disk->src; -- 2.4.6
While we currently only allow changing a media in a disk, this is going to change in a while, so the function name would be invalid. Moreover, the old name does not match the pattern laid out by other update functions. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fadead5..8946b20 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7914,11 +7914,11 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm, } static int -qemuDomainChangeDiskMediaLive(virConnectPtr conn, - virDomainObjPtr vm, - virDomainDeviceDefPtr dev, - virQEMUDriverPtr driver, - bool force) +qemuDomainChangeDiskLive(virConnectPtr conn, + virDomainObjPtr vm, + virDomainDeviceDefPtr dev, + virQEMUDriverPtr driver, + bool force) { virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; @@ -7983,7 +7983,7 @@ qemuDomainUpdateDeviceLive(virConnectPtr conn, switch ((virDomainDeviceType) dev->type) { case VIR_DOMAIN_DEVICE_DISK: qemuDomainObjCheckDiskTaint(driver, vm, dev->data.disk, -1); - ret = qemuDomainChangeDiskMediaLive(conn, vm, dev, driver, force); + ret = qemuDomainChangeDiskLive(conn, vm, dev, driver, force); break; case VIR_DOMAIN_DEVICE_GRAPHICS: ret = qemuDomainChangeGraphics(driver, vm, dev->data.graphics); -- 2.4.6
This new private API should return true iff sources of two disks differs in sense that qemu should be instructed to change the disk backend. For instance, ejecting a CDROM is such case, or pointing disk into a different ISO location, and so on. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 33 +++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 37 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c8b0ccd..60bd560 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3002,6 +3002,39 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, bool +qemuDomainDiskSourceDiffers(virConnectPtr conn, + virDomainDiskDefPtr disk, + virDomainDiskDefPtr origDisk) +{ + char *diskSrc = NULL, *origDiskSrc = NULL; + bool diskEmpty, origDiskEmpty; + bool ret = true; + + diskEmpty = virStorageSourceIsEmpty(disk->src); + origDiskEmpty = virStorageSourceIsEmpty(origDisk->src); + + if (diskEmpty && origDiskEmpty) + return false; + + if (diskEmpty ^ origDiskEmpty) + return true; + + if (qemuGetDriveSourceString(disk->src, conn, &diskSrc) < 0 || + qemuGetDriveSourceString(origDisk->src, conn, &origDiskSrc) < 0) + goto cleanup; + + /* So far in qemu disk sources are considered different + * if either path to disk or its format changes. */ + ret = virDomainDiskGetFormat(disk) != virDomainDiskGetFormat(origDisk) || + STRNEQ_NULLABLE(diskSrc, origDiskSrc); + cleanup: + VIR_FREE(diskSrc); + VIR_FREE(origDiskSrc); + return ret; +} + + +bool qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk) { qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 91eaea1..0b9fd1c 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -411,6 +411,10 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, bool force_probe, bool report_broken); +bool qemuDomainDiskSourceDiffers(virConnectPtr conn, + virDomainDiskDefPtr disk, + virDomainDiskDefPtr origDisk); + int qemuDomainStorageFileInit(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src); -- 2.4.6
Firstly, our coding guidelines suggest using 'cleanup' label instead of 'end'. Then, @ret should be set to value representing success as the last statement before the 'cleanup' label. And while I am at this function, lets enumerate all the possible enum items (virDomainDiskDevice) and avoid using 'default' in switch(). Pooh. Also, nothing bad happens if we look up the disk to change in the domain upfront. In fact, it's going to be helpful later when we want to keep some old values for performing a rollback. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 48 +++++++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8946b20..6a8e863 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7925,48 +7925,54 @@ qemuDomainChangeDiskLive(virConnectPtr conn, int ret = -1; if (virStorageTranslateDiskSourcePool(conn, disk) < 0) - goto end; + goto cleanup; if (qemuDomainDetermineDiskChain(driver, vm, disk, false, true) < 0) - goto end; + goto cleanup; - switch (disk->device) { + if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, + disk->bus, disk->dst))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("No device with bus '%s' and target '%s'"), + virDomainDiskBusTypeToString(disk->bus), + disk->dst); + goto cleanup; + } + + switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - if (!(orig_disk = virDomainDiskFindByBusAndDst(vm->def, - disk->bus, disk->dst))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("No device with bus '%s' and target '%s'"), - virDomainDiskBusTypeToString(disk->bus), - disk->dst); - goto end; - } - if (!virDomainDiskDiffersSourceOnly(disk, orig_disk)) - goto end; + goto cleanup; /* Add the new disk src into shared disk hash table */ if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) - goto end; + goto cleanup; if (qemuDomainChangeEjectableMedia(driver, conn, vm, - orig_disk, dev->data.disk->src, force) < 0) { - ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, vm->def->name)); - goto end; + orig_disk, disk->src, force) < 0) { + ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); + goto cleanup; } - dev->data.disk->src = NULL; - ret = 0; + disk->src = NULL; break; - default: + case VIR_DOMAIN_DISK_DEVICE_DISK: + case VIR_DOMAIN_DISK_DEVICE_LUN: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("disk bus '%s' cannot be updated."), virDomainDiskBusTypeToString(disk->bus)); + goto cleanup; + break; + + case VIR_DOMAIN_DISK_DEVICE_LAST: + /* nada */ break; } - end: + ret = 0; + cleanup: return ret; } -- 2.4.6
I always felt like this function is qemu specific rather than libvirt-wide. Other drivers may act differently on virDomainDef change and in fact may require talking to underlying hypervisor even if something else's than disk->src has changed. I know that the function is still incomplete, but lets break that into two commits that are easier to review. This one is pure code movement. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 127 ----------------------------------------------- src/conf/domain_conf.h | 2 - src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 3 ++ src/qemu/qemu_driver.c | 2 +- 6 files changed, 131 insertions(+), 131 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6df1618..a3b3ccb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5838,133 +5838,6 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def, } -/* - * Makes sure the @disk differs from @orig_disk only by the source - * path and nothing else. Fields that are being checked and the - * information whether they are nullable (may not be specified) or is - * taken from the virDomainDiskDefFormat() code. - */ -bool -virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, - virDomainDiskDefPtr orig_disk) -{ -#define CHECK_EQ(field, field_name, nullable) \ - do { \ - if (nullable && !disk->field) \ - break; \ - if (disk->field != orig_disk->field) { \ - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, \ - _("cannot modify field '%s' of the disk"), \ - field_name); \ - return false; \ - } \ - } while (0) - - CHECK_EQ(device, "device", false); - CHECK_EQ(cachemode, "cache", true); - CHECK_EQ(error_policy, "error_policy", true); - CHECK_EQ(rerror_policy, "rerror_policy", true); - CHECK_EQ(iomode, "io", true); - CHECK_EQ(ioeventfd, "ioeventfd", true); - CHECK_EQ(event_idx, "event_idx", true); - CHECK_EQ(copy_on_read, "copy_on_read", true); - CHECK_EQ(discard, "discard", true); - CHECK_EQ(iothread, "iothread", true); - - if (disk->geometry.cylinders && - disk->geometry.heads && - disk->geometry.sectors) { - CHECK_EQ(geometry.cylinders, "geometry cylinders", false); - CHECK_EQ(geometry.heads, "geometry heads", false); - CHECK_EQ(geometry.sectors, "geometry sectors", false); - CHECK_EQ(geometry.trans, "BIOS-translation-modus", true); - } - - CHECK_EQ(blockio.logical_block_size, - "blockio logical_block_size", false); - CHECK_EQ(blockio.physical_block_size, - "blockio physical_block_size", false); - - if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) - CHECK_EQ(removable, "removable", true); - - CHECK_EQ(blkdeviotune.total_bytes_sec, - "blkdeviotune total_bytes_sec", - true); - CHECK_EQ(blkdeviotune.read_bytes_sec, - "blkdeviotune read_bytes_sec", - true); - CHECK_EQ(blkdeviotune.write_bytes_sec, - "blkdeviotune write_bytes_sec", - true); - CHECK_EQ(blkdeviotune.total_iops_sec, - "blkdeviotune total_iops_sec", - true); - CHECK_EQ(blkdeviotune.read_iops_sec, - "blkdeviotune read_iops_sec", - true); - CHECK_EQ(blkdeviotune.write_iops_sec, - "blkdeviotune write_iops_sec", - true); - CHECK_EQ(blkdeviotune.total_bytes_sec_max, - "blkdeviotune total_bytes_sec_max", - true); - CHECK_EQ(blkdeviotune.read_bytes_sec_max, - "blkdeviotune read_bytes_sec_max", - true); - CHECK_EQ(blkdeviotune.write_bytes_sec_max, - "blkdeviotune write_bytes_sec_max", - true); - CHECK_EQ(blkdeviotune.total_iops_sec_max, - "blkdeviotune total_iops_sec_max", - true); - CHECK_EQ(blkdeviotune.read_iops_sec_max, - "blkdeviotune read_iops_sec_max", - true); - CHECK_EQ(blkdeviotune.write_iops_sec_max, - "blkdeviotune write_iops_sec_max", - true); - CHECK_EQ(blkdeviotune.size_iops_sec, - "blkdeviotune size_iops_sec", - true); - - CHECK_EQ(transient, "transient", true); - - if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "serial"); - return false; - } - - if (disk->wwn && STRNEQ_NULLABLE(disk->wwn, orig_disk->wwn)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "wwn"); - return false; - } - - if (disk->vendor && STRNEQ_NULLABLE(disk->vendor, orig_disk->vendor)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "vendor"); - return false; - } - - if (disk->product && STRNEQ_NULLABLE(disk->product, orig_disk->product)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "product"); - return false; - } - - CHECK_EQ(info.bootIndex, "boot order", true); - -#undef CHECK_EQ - - return true; -} - int virDomainDiskDefAssignAddress(virDomainXMLOptionPtr xmlopt, virDomainDiskDefPtr def, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f043554..8be390b 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2512,8 +2512,6 @@ int virDomainDeviceFindControllerModel(virDomainDefPtr def, virDomainDiskDefPtr virDomainDiskFindByBusAndDst(virDomainDefPtr def, int bus, char *dst); -bool virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, - virDomainDiskDefPtr orig_disk); void virDomainControllerDefFree(virDomainControllerDefPtr def); void virDomainFSDefFree(virDomainFSDefPtr def); void virDomainActualNetDefFree(virDomainActualNetDefPtr def); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5b3da83..1bb41f7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -251,7 +251,6 @@ virDomainDiskDefFree; virDomainDiskDefNew; virDomainDiskDefSourceParse; virDomainDiskDeviceTypeToString; -virDomainDiskDiffersSourceOnly; virDomainDiskDiscardTypeToString; virDomainDiskErrorPolicyTypeFromString; virDomainDiskErrorPolicyTypeToString; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 60bd560..21f5002 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3034,6 +3034,133 @@ qemuDomainDiskSourceDiffers(virConnectPtr conn, } +/* + * Makes sure the @disk differs from @orig_disk only by the source + * path and nothing else. Fields that are being checked and the + * information whether they are nullable (may not be specified) or is + * taken from the virDomainDiskDefFormat() code. + */ +bool +qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, + virDomainDiskDefPtr orig_disk) +{ +#define CHECK_EQ(field, field_name, nullable) \ + do { \ + if (nullable && !disk->field) \ + break; \ + if (disk->field != orig_disk->field) { \ + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, \ + _("cannot modify field '%s' of the disk"), \ + field_name); \ + return false; \ + } \ + } while (0) + + CHECK_EQ(device, "device", false); + CHECK_EQ(cachemode, "cache", true); + CHECK_EQ(error_policy, "error_policy", true); + CHECK_EQ(rerror_policy, "rerror_policy", true); + CHECK_EQ(iomode, "io", true); + CHECK_EQ(ioeventfd, "ioeventfd", true); + CHECK_EQ(event_idx, "event_idx", true); + CHECK_EQ(copy_on_read, "copy_on_read", true); + CHECK_EQ(discard, "discard", true); + CHECK_EQ(iothread, "iothread", true); + + if (disk->geometry.cylinders && + disk->geometry.heads && + disk->geometry.sectors) { + CHECK_EQ(geometry.cylinders, "geometry cylinders", false); + CHECK_EQ(geometry.heads, "geometry heads", false); + CHECK_EQ(geometry.sectors, "geometry sectors", false); + CHECK_EQ(geometry.trans, "BIOS-translation-modus", true); + } + + CHECK_EQ(blockio.logical_block_size, + "blockio logical_block_size", false); + CHECK_EQ(blockio.physical_block_size, + "blockio physical_block_size", false); + + if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) + CHECK_EQ(removable, "removable", true); + + CHECK_EQ(blkdeviotune.total_bytes_sec, + "blkdeviotune total_bytes_sec", + true); + CHECK_EQ(blkdeviotune.read_bytes_sec, + "blkdeviotune read_bytes_sec", + true); + CHECK_EQ(blkdeviotune.write_bytes_sec, + "blkdeviotune write_bytes_sec", + true); + CHECK_EQ(blkdeviotune.total_iops_sec, + "blkdeviotune total_iops_sec", + true); + CHECK_EQ(blkdeviotune.read_iops_sec, + "blkdeviotune read_iops_sec", + true); + CHECK_EQ(blkdeviotune.write_iops_sec, + "blkdeviotune write_iops_sec", + true); + CHECK_EQ(blkdeviotune.total_bytes_sec_max, + "blkdeviotune total_bytes_sec_max", + true); + CHECK_EQ(blkdeviotune.read_bytes_sec_max, + "blkdeviotune read_bytes_sec_max", + true); + CHECK_EQ(blkdeviotune.write_bytes_sec_max, + "blkdeviotune write_bytes_sec_max", + true); + CHECK_EQ(blkdeviotune.total_iops_sec_max, + "blkdeviotune total_iops_sec_max", + true); + CHECK_EQ(blkdeviotune.read_iops_sec_max, + "blkdeviotune read_iops_sec_max", + true); + CHECK_EQ(blkdeviotune.write_iops_sec_max, + "blkdeviotune write_iops_sec_max", + true); + CHECK_EQ(blkdeviotune.size_iops_sec, + "blkdeviotune size_iops_sec", + true); + + CHECK_EQ(transient, "transient", true); + + if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "serial"); + return false; + } + + if (disk->wwn && STRNEQ_NULLABLE(disk->wwn, orig_disk->wwn)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "wwn"); + return false; + } + + if (disk->vendor && STRNEQ_NULLABLE(disk->vendor, orig_disk->vendor)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "vendor"); + return false; + } + + if (disk->product && STRNEQ_NULLABLE(disk->product, orig_disk->product)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "product"); + return false; + } + + CHECK_EQ(info.bootIndex, "boot order", true); + +#undef CHECK_EQ + + return true; +} + bool qemuDomainDiskBlockJobIsActive(virDomainDiskDefPtr disk) { diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 0b9fd1c..8cf535f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -415,6 +415,9 @@ bool qemuDomainDiskSourceDiffers(virConnectPtr conn, virDomainDiskDefPtr disk, virDomainDiskDefPtr origDisk); +bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, + virDomainDiskDefPtr orig_disk); + int qemuDomainStorageFileInit(virQEMUDriverPtr driver, virDomainObjPtr vm, virStorageSourcePtr src); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6a8e863..1a189cc 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7942,7 +7942,7 @@ qemuDomainChangeDiskLive(virConnectPtr conn, switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: - if (!virDomainDiskDiffersSourceOnly(disk, orig_disk)) + if (!qemuDomainDiskChangeSupported(disk, orig_disk)) goto cleanup; /* Add the new disk src into shared disk hash table */ -- 2.4.6
So far this function was not kept in sync with changing virDomainDiskDef. Fill in all the missing checks and reorganize their order so it's easier to track which items are not being checked for. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 45 +++++++++++++++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 21f5002..ed92d8a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3057,15 +3057,15 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, } while (0) CHECK_EQ(device, "device", false); - CHECK_EQ(cachemode, "cache", true); - CHECK_EQ(error_policy, "error_policy", true); - CHECK_EQ(rerror_policy, "rerror_policy", true); - CHECK_EQ(iomode, "io", true); - CHECK_EQ(ioeventfd, "ioeventfd", true); - CHECK_EQ(event_idx, "event_idx", true); - CHECK_EQ(copy_on_read, "copy_on_read", true); - CHECK_EQ(discard, "discard", true); - CHECK_EQ(iothread, "iothread", true); + CHECK_EQ(bus, "bus", false); + if (STRNEQ(disk->dst, orig_disk->dst)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "bus"); + return false; + } + CHECK_EQ(tray_status, "tray", true); + CHECK_EQ(removable, "removable", true); if (disk->geometry.cylinders && disk->geometry.heads && @@ -3081,9 +3081,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(blockio.physical_block_size, "blockio physical_block_size", false); - if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) - CHECK_EQ(removable, "removable", true); - CHECK_EQ(blkdeviotune.total_bytes_sec, "blkdeviotune total_bytes_sec", true); @@ -3124,8 +3121,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, "blkdeviotune size_iops_sec", true); - CHECK_EQ(transient, "transient", true); - if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("cannot modify field '%s' of the disk"), @@ -3154,7 +3149,29 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, return false; } + CHECK_EQ(cachemode, "cache", true); + CHECK_EQ(error_policy, "error_policy", true); + CHECK_EQ(rerror_policy, "rerror_policy", true); + CHECK_EQ(iomode, "io", true); + CHECK_EQ(ioeventfd, "ioeventfd", true); + CHECK_EQ(event_idx, "event_idx", true); + CHECK_EQ(copy_on_read, "copy_on_read", true); + CHECK_EQ(snapshot, "snapshot", true); + CHECK_EQ(startupPolicy, "startupPolicy", true); + CHECK_EQ(transient, "transient", true); CHECK_EQ(info.bootIndex, "boot order", true); + CHECK_EQ(rawio, "rawio", true); + CHECK_EQ(sgio, "sgio", true); + CHECK_EQ(discard, "discard", true); + CHECK_EQ(iothread, "iothread", true); + + if (disk->domain_name && + STRNEQ_NULLABLE(disk->domain_name, orig_disk->domain_name)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "backenddomain"); + return false; + } #undef CHECK_EQ -- 2.4.6
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_driver.c | 29 ++++++++++++++++++++--------- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed92d8a..fb8ab30 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3157,7 +3157,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(event_idx, "event_idx", true); CHECK_EQ(copy_on_read, "copy_on_read", true); CHECK_EQ(snapshot, "snapshot", true); - CHECK_EQ(startupPolicy, "startupPolicy", true); CHECK_EQ(transient, "transient", true); CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1a189cc..d74255b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7922,6 +7922,7 @@ qemuDomainChangeDiskLive(virConnectPtr conn, { virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; + int startupPolicy; int ret = -1; if (virStorageTranslateDiskSourcePool(conn, disk) < 0) @@ -7939,23 +7940,29 @@ qemuDomainChangeDiskLive(virConnectPtr conn, goto cleanup; } + startupPolicy = orig_disk->startupPolicy; + switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: if (!qemuDomainDiskChangeSupported(disk, orig_disk)) goto cleanup; - /* Add the new disk src into shared disk hash table */ - if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) - goto cleanup; + orig_disk->startupPolicy = dev->data.disk->startupPolicy; - if (qemuDomainChangeEjectableMedia(driver, conn, vm, - orig_disk, disk->src, force) < 0) { - ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - goto cleanup; + if (qemuDomainDiskSourceDiffers(conn, disk, orig_disk)) { + /* Add the new disk src into shared disk hash table */ + if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) + goto cleanup; + + if (qemuDomainChangeEjectableMedia(driver, conn, vm, + orig_disk, dev->data.disk->src, force) < 0) { + ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, vm->def->name)); + goto rollback; + } + + dev->data.disk->src = NULL; } - - disk->src = NULL; break; case VIR_DOMAIN_DISK_DEVICE_DISK: @@ -7974,6 +7981,10 @@ qemuDomainChangeDiskLive(virConnectPtr conn, ret = 0; cleanup: return ret; + + rollback: + orig_disk->startupPolicy = startupPolicy; + goto cleanup; } static int -- 2.4.6
On 09/16/2015 05:15 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_driver.c | 29 ++++++++++++++++++++--------- 2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed92d8a..fb8ab30 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3157,7 +3157,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(event_idx, "event_idx", true); CHECK_EQ(copy_on_read, "copy_on_read", true); CHECK_EQ(snapshot, "snapshot", true); - CHECK_EQ(startupPolicy, "startupPolicy", true);
Perhaps leave a comment indicating that startupPolicy is special so that someone doesn't come along one day and say - oh look startupPolicy is missing, I'm going to add it here... Assuming one reads comments ;-) It's really too bad there isn't some "simple mechanism" to ensure new fields are listed here too.
CHECK_EQ(transient, "transient", true); CHECK_EQ(info.bootIndex, "boot order", true); CHECK_EQ(rawio, "rawio", true); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1a189cc..d74255b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7922,6 +7922,7 @@ qemuDomainChangeDiskLive(virConnectPtr conn, { virDomainDiskDefPtr disk = dev->data.disk; virDomainDiskDefPtr orig_disk = NULL; + int startupPolicy; int ret = -1;
if (virStorageTranslateDiskSourcePool(conn, disk) < 0) @@ -7939,23 +7940,29 @@ qemuDomainChangeDiskLive(virConnectPtr conn, goto cleanup; }
+ startupPolicy = orig_disk->startupPolicy; + switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_CDROM: case VIR_DOMAIN_DISK_DEVICE_FLOPPY: if (!qemuDomainDiskChangeSupported(disk, orig_disk)) goto cleanup;
- /* Add the new disk src into shared disk hash table */ - if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) - goto cleanup; + orig_disk->startupPolicy = dev->data.disk->startupPolicy;
- if (qemuDomainChangeEjectableMedia(driver, conn, vm, - orig_disk, disk->src, force) < 0) { - ignore_value(qemuRemoveSharedDisk(driver, disk, vm->def->name)); - goto cleanup; + if (qemuDomainDiskSourceDiffers(conn, disk, orig_disk)) { + /* Add the new disk src into shared disk hash table */ + if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) + goto cleanup; + + if (qemuDomainChangeEjectableMedia(driver, conn, vm, + orig_disk, dev->data.disk->src, force) < 0) { + ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, vm->def->name)); + goto rollback; + } + + dev->data.disk->src = NULL; } - - disk->src = NULL; break;
case VIR_DOMAIN_DISK_DEVICE_DISK: @@ -7974,6 +7981,10 @@ qemuDomainChangeDiskLive(virConnectPtr conn, ret = 0; cleanup: return ret; + + rollback: + orig_disk->startupPolicy = startupPolicy; + goto cleanup; }
static int
On 18.09.2015 22:47, John Ferlan wrote:
On 09/16/2015 05:15 AM, Michal Privoznik wrote:
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 1 - src/qemu/qemu_driver.c | 29 ++++++++++++++++++++--------- 2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ed92d8a..fb8ab30 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3157,7 +3157,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(event_idx, "event_idx", true); CHECK_EQ(copy_on_read, "copy_on_read", true); CHECK_EQ(snapshot, "snapshot", true); - CHECK_EQ(startupPolicy, "startupPolicy", true);
Perhaps leave a comment indicating that startupPolicy is special so that someone doesn't come along one day and say - oh look startupPolicy is missing, I'm going to add it here... Assuming one reads comments ;-)
Good point. Added.
It's really too bad there isn't some "simple mechanism" to ensure new fields are listed here too.
Yeah. Perhaps some autogenerated function. But that would mean parsing C code, which alone explodes straight away. Michal
On 09/16/2015 05:14 AM, Michal Privoznik wrote:
So, as Peter pointed out, we may want to updated startupPolicy for live domains too. That's what patches 2/7-7/7 do.
Michal Privoznik (7): qemuDomainUpdateDeviceConfig: Allow startupPolicy update, yet again qemu: s/qemuDomainChangeDiskMediaLive/qemuDomainChangeDiskLive/ qemu_domain: Introduce qemuDomainDiskSourceDiffers qemuDomainChangeDiskLive: rework slightly qemu: s/virDomainDiskDiffersSourceOnly/qemuDomainDiskChangeSupported/ qemuDomainDiskChangeSupported: Fill in missing checks qemuDomainChangeDiskLive: Allow startupPolicy change
src/conf/domain_conf.c | 127 ---------------------------------- src/conf/domain_conf.h | 2 - src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 7 ++ src/qemu/qemu_driver.c | 80 ++++++++++++--------- 6 files changed, 232 insertions(+), 161 deletions(-)
ACK series (although there is a minor comment in 7/7) John
participants (2)
-
John Ferlan -
Michal Privoznik