If one calls update-device with information that is not updatable, libvirt reports success even though no data were updated. The example used in the bug linked below uses updating device with <boot order='2'/> which, in my opinion, is a valid thing to request from user's perspective. Mainly since we properly error out if user wants to update such data on a network device for example. And since there are many things that might happen (update-device on disk basically knows just how to change removable media), check for what's changing and moreover, since the function might be usable in other dirvers (updating only disk path is a valid possibility) let's abstract it for any two disks. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1007228 --- src/conf/domain_conf.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 3 ++ 4 files changed, 117 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0219c3c4814d..a6950087d987 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5687,6 +5687,117 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def, return NULL; } + +/* + * 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 (can be NULL) 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); + CHECK_EQ(serial, "serial", true); + CHECK_EQ(wwn, "wwn", true); + CHECK_EQ(vendor, "vendor", true); + CHECK_EQ(product, "product", true); + + CHECK_EQ(info.bootIndex, "boot order", true); + + if (disk->info.alias && STRNEQ(disk->info.alias, orig_disk->info.alias)) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("cannot modify field '%s' of the disk"), + "alias name"); + return false; + } + +#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 12d945ea7f24..b84640c2de56 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2475,6 +2475,8 @@ 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 1566d11e4156..5534dc7ed704 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -249,6 +249,7 @@ virDomainDiskDefFree; virDomainDiskDefNew; virDomainDiskDefSourceParse; virDomainDiskDeviceTypeToString; +virDomainDiskDiffersSourceOnly; virDomainDiskDiscardTypeToString; virDomainDiskErrorPolicyTypeFromString; virDomainDiskErrorPolicyTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ba804429a28a..4f1724e92e4a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7916,6 +7916,9 @@ qemuDomainChangeDiskMediaLive(virConnectPtr conn, goto end; } + if (!virDomainDiskDiffersSourceOnly(disk, orig_disk)) + goto end; + /* Add the new disk src into shared disk hash table */ if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0) goto end; -- 2.4.5