[libvirt] [PATCH] qemu: Reject updating unsupported disk information
Peter Krempa
pkrempa at redhat.com
Thu Jul 9 17:14:32 UTC 2015
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150709/e961f814/attachment-0001.sig>
More information about the libvir-list
mailing list