7:14 p.m.
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