On 10.07.2015 08:20, 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 > drivers (updating only disk path is a valid possibility) let's abstract > it for any two disks. > > We can't possibly check for everything since for many fields our code > does not properly differentiate between default and unspecified values. > Even though this could be changed, I don't feel like it's worth the > complexity so it's not the aim of this patch. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1007228 > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> > --- > > Notes: > v2: > - Don't say 'NULL' when it should be 'unspecified' > - Don't blindly copy field name into the error message, but use space > instead of dot. That way it looks the same as in the XML provided. > - Check strings properly instead of addresses > > src/conf/domain_conf.c | 133 +++++++++++++++++++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 2 + > src/libvirt_private.syms | 1 + > src/qemu/qemu_driver.c | 3 ++ > 4 files changed, 139 insertions(+) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 1f7862b00463..69e5df27c270 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -5690,6 +5690,139 @@ 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 (may not be specified) or is > + * taken from the virDomainDiskDefFormat() code. > + */ > +bool > +virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk, > + virDomainDiskDefPtr orig_disk) > +{ Nice work, but I think this should go somewhere into src/qemu/. I mean, some o these fields are not changeable as a domain is running. But some might be depending on the underlying hypervisor. For instance, xen might be able to change serial number of a HDD. On the other hand, that might be just a few corner cases I'm talking about, so it's up to you what you prefer more. > +#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); > + > + if (disk->transient && STRNEQ(disk->transient, orig_disk->transient)) { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, > + _("cannot modify field '%s' of the disk"), > + "transient"); > + return false; > + } > + > + if (disk->serial && STRNEQ(disk->serial, orig_disk->serial)) { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, > + _("cannot modify field '%s' of the disk"), > + "serial"); > + return false; > + } > + > + if (disk->wwn && STRNEQ(disk->wwn, orig_disk->wwn)) { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, > + _("cannot modify field '%s' of the disk"), > + "wwn"); > + return false; > + } > + > + if (disk->vendor && STRNEQ(disk->vendor, orig_disk->vendor)) { > + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, > + _("cannot modify field '%s' of the disk"), > + "vendor"); > + return false; > + } > + > + if (disk->product && STRNEQ(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 50750c1dfa14..0fe6b1a47c8f 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -2474,6 +2474,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 720afdf4dd11..1c7c492951ba 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 c4b3979f89ef..62bba2ea4054 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; > ACK Michal -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list