On Thu, Jul 09, 2015 at 18:36:00 +0200, Martin Kletzander wrote:
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
Um, NULL? see below ...
+ * 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);
Lets take the line below as an example.
+ CHECK_EQ(error_policy, "error_policy", true);
If disk->error_policy == VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT (which equals to 0) the check will be skipped and thus the error_policy setting might be different and we would not report an error.
+ 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); +
As an example of the field below. The field is an integer ...
+ 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",
All of the usage of CHECK_EQ use the same text in the error message as is the field name. You could perhaps use the strigification macro operator.
+ 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);
Fields below are strings, but CHECK_EQ doesn't use string comparison.
+ 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)) {
The alias isn't parsed from the domain so this probably won't ever trigger.
+ 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)
Peter