[libvirt] [PATCH v2] qemu: Reject updating unsupported disk information

Michal Privoznik mprivozn at redhat.com
Fri Jul 17 14:21:29 UTC 2015


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 at redhat.com>
> ---
> 
> 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




More information about the libvir-list mailing list