[libvirt] [PATCH 0/4] Fix checking of blkiotune group name on update
*** BLURB HERE *** *** BLURB THERE *** *** BLURB EVERYWHERE *** Ján Tomko (4): Revert "qemu: emit error when trying to update blkiotune group_name in qemuDomainChangeDiskLive" qemu: introduce CHECK_STREQ_NULLABLE in qemuDomainDiskChangeSupported qemu: error out on attempt to change blkiotune group name qemuDomainDiskChangeSupported: use CHECK_STREQ_NULLABLE more src/qemu/qemu_domain.c | 64 +++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 38 deletions(-) -- 2.20.1
https://bugzilla.redhat.com/show_bug.cgi?id=1601677 This reverts commit 047cfb05ee949325e77726531fd381820be8dc62 Using numeric comparison on strings means we reject every update that does include the group name, even if it's unchanged. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9ca4ca33e5..bb3a672d47 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9386,9 +9386,6 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(blkdeviotune.size_iops_sec, "blkdeviotune size_iops_sec", true); - CHECK_EQ(blkdeviotune.group_name, - "blkdeviotune group_name", - true); if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, -- 2.20.1
On 3/28/19 10:34 AM, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1601677
This reverts commit 047cfb05ee949325e77726531fd381820be8dc62 Using numeric comparison on strings means we reject every update that does include the group name, even if it's unchanged.
Oopsy!
Signed-off-by: Ján Tomko <jtomko@redhat.com>
Reviewed-by: Laine Stump <laine@laine.org>
A marco for comparing string fields of the disk. https://bugzilla.redhat.com/show_bug.cgi?id=1601677 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bb3a672d47..72e322d6a7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9322,6 +9322,18 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, } \ } while (0) +#define CHECK_STREQ_NULLABLE(field, field_name) \ + do { \ + if (!disk->field) \ + break; \ + if (STRNEQ_NULLABLE(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(bus, "bus", false); if (STRNEQ(disk->dst, orig_disk->dst)) { @@ -9469,6 +9481,7 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, } #undef CHECK_EQ +#undef CHECK_STREQ_NULLABLE return true; } -- 2.20.1
On 3/28/19 10:34 AM, Ján Tomko wrote:
A marco for comparing string fields of the disk.
Polo'ed-by: Laine Stump <laine@laine.org> (seriously, though - s/marco/macro/ :-)
https://bugzilla.redhat.com/show_bug.cgi?id=1601677
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bb3a672d47..72e322d6a7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9322,6 +9322,18 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, } \ } while (0)
+#define CHECK_STREQ_NULLABLE(field, field_name) \ + do { \ + if (!disk->field) \ + break; \
So is a missing value in the updated XML equal to "no change"? Or Does a missing value actually mean "this should be un-set if it has been set to something"? (I'm asking this because in the case of MTU for <interface>, if the existing interface has an mtu set (even to 1500), and the updated XML has no MTU, we consider that a change (and don't allow it). Reviewed-by: Laine Stump <laine@laine.org> once the commit message typo is fixed, and if the meaning of "not specified" for a field in the update truly is meant to be "don't change" rather than "remove any previous setting of this field and return it to default".
+ if (STRNEQ_NULLABLE(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(bus, "bus", false); if (STRNEQ(disk->dst, orig_disk->dst)) { @@ -9469,6 +9481,7 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, }
#undef CHECK_EQ +#undef CHECK_STREQ_NULLABLE
return true; }
On Thu, Mar 28, 2019 at 01:54:50PM -0400, Laine Stump wrote:
On 3/28/19 10:34 AM, Ján Tomko wrote:
A marco for comparing string fields of the disk.
Polo'ed-by: Laine Stump <laine@laine.org>
(seriously, though - s/marco/macro/ :-)
https://bugzilla.redhat.com/show_bug.cgi?id=1601677
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bb3a672d47..72e322d6a7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9322,6 +9322,18 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, } \ } while (0) +#define CHECK_STREQ_NULLABLE(field, field_name) \ + do { \ + if (!disk->field) \ + break; \
So is a missing value in the updated XML equal to "no change"? Or Does a missing value actually mean "this should be un-set if it has been set to something"?
It is interpreted as "no change" here for all the numeric attributes using CHECK_EQ with the last parameter set to true and all the string attributes. For interfaces, most of the attributes are considered as "no change" when not present - most notably the PCI address, omitting it would mean we use the DeviceInfo structure from the existing device until Katerina fixed this recently: https://bugzilla.redhat.com/show_bug.cgi?id=1599513 Thanks to this bug requesting us not to require the alias to be present: https://bugzilla.redhat.com/show_bug.cgi?id=1621910 Michal formally documented our requirements for the virDomainUpdateDeviceFlags API: The supplied XML description of the device should contain all the information that is found in the corresponding domain XML. Leaving out any piece of information may be treated as a request for its removal, which may be denied. So we're consistently inconsistent here and I plan to flip a coin to figure out whether a lack of boot order means "no change" or a "request for removal": https://bugzilla.redhat.com/show_bug.cgi?id=1661367 Jano
(I'm asking this because in the case of MTU for <interface>, if the existing interface has an mtu set (even to 1500), and the updated XML has no MTU, we consider that a change (and don't allow it).
Reviewed-by: Laine Stump <laine@laine.org>
once the commit message typo is fixed, and if the meaning of "not specified" for a field in the update truly is meant to be "don't change" rather than "remove any previous setting of this field and return it to default".
+ if (STRNEQ_NULLABLE(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(bus, "bus", false); if (STRNEQ(disk->dst, orig_disk->dst)) { @@ -9469,6 +9481,7 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, } #undef CHECK_EQ +#undef CHECK_STREQ_NULLABLE return true; }
Check that the attribute is the same in qemuDomainDiskChangeSupported in case somebody tries to change it using the UpdateDevice API. https://bugzilla.redhat.com/show_bug.cgi?id=1601677 Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72e322d6a7..e75ea15c91 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9398,6 +9398,8 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(blkdeviotune.size_iops_sec, "blkdeviotune size_iops_sec", true); + CHECK_STREQ_NULLABLE(blkdeviotune.group_name, + "blkdeviotune group name"); if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, -- 2.20.1
On 3/28/19 10:34 AM, Ján Tomko wrote:
Check that the attribute is the same in qemuDomainDiskChangeSupported in case somebody tries to change it using the UpdateDevice API.
https://bugzilla.redhat.com/show_bug.cgi?id=1601677
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 72e322d6a7..e75ea15c91 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9398,6 +9398,8 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(blkdeviotune.size_iops_sec, "blkdeviotune size_iops_sec", true); + CHECK_STREQ_NULLABLE(blkdeviotune.group_name, + "blkdeviotune group name");
if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
Again, as long as "no serial specified" is supposed to mean "serial remains unchanged" rather than "there should be no serial", then Reviewed-by: Laine Stump <laine@laine.org>
Convert the other string comparisons to use the macro. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 44 ++++++++++-------------------------------- 1 file changed, 10 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e75ea15c91..836e30decf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9401,33 +9401,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_STREQ_NULLABLE(blkdeviotune.group_name, "blkdeviotune group name"); - if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "serial"); - return false; - } - - if (disk->wwn && STRNEQ_NULLABLE(disk->wwn, orig_disk->wwn)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "wwn"); - return false; - } - - if (disk->vendor && STRNEQ_NULLABLE(disk->vendor, orig_disk->vendor)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "vendor"); - return false; - } - - if (disk->product && STRNEQ_NULLABLE(disk->product, orig_disk->product)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "product"); - return false; - } + CHECK_STREQ_NULLABLE(serial, + "serial"); + CHECK_STREQ_NULLABLE(wwn, + "wwn"); + CHECK_STREQ_NULLABLE(vendor, + "vendor"); + CHECK_STREQ_NULLABLE(product, + "product"); CHECK_EQ(cachemode, "cache", true); CHECK_EQ(error_policy, "error_policy", true); @@ -9460,13 +9441,8 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(discard, "discard", true); CHECK_EQ(iothread, "iothread", true); - if (disk->domain_name && - STRNEQ_NULLABLE(disk->domain_name, orig_disk->domain_name)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "backenddomain"); - return false; - } + CHECK_STREQ_NULLABLE(domain_name, + "backenddomain"); /* checks for fields stored in disk->src */ /* unfortunately 'readonly' and 'shared' can't be converted to tristate -- 2.20.1
On 3/28/19 10:34 AM, Ján Tomko wrote:
Convert the other string comparisons to use the macro.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 44 ++++++++++-------------------------------- 1 file changed, 10 insertions(+), 34 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e75ea15c91..836e30decf 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -9401,33 +9401,14 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_STREQ_NULLABLE(blkdeviotune.group_name, "blkdeviotune group name");
- if (disk->serial && STRNEQ_NULLABLE(disk->serial, orig_disk->serial)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "serial"); - return false; - }
Hmmm. Well (to answer my own question from the previous patch) certainly in the current implementation of all these the meaining of "not specified" is "do not change", so I guess at the very least you're preserving current behavior. Reviewed-by: Laine Stump <laine@laine.org>
- - if (disk->wwn && STRNEQ_NULLABLE(disk->wwn, orig_disk->wwn)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "wwn"); - return false; - } - - if (disk->vendor && STRNEQ_NULLABLE(disk->vendor, orig_disk->vendor)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "vendor"); - return false; - } - - if (disk->product && STRNEQ_NULLABLE(disk->product, orig_disk->product)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "product"); - return false; - } + CHECK_STREQ_NULLABLE(serial, + "serial"); + CHECK_STREQ_NULLABLE(wwn, + "wwn"); + CHECK_STREQ_NULLABLE(vendor, + "vendor"); + CHECK_STREQ_NULLABLE(product, + "product");
CHECK_EQ(cachemode, "cache", true); CHECK_EQ(error_policy, "error_policy", true); @@ -9460,13 +9441,8 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk, CHECK_EQ(discard, "discard", true); CHECK_EQ(iothread, "iothread", true);
- if (disk->domain_name && - STRNEQ_NULLABLE(disk->domain_name, orig_disk->domain_name)) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("cannot modify field '%s' of the disk"), - "backenddomain"); - return false; - } + CHECK_STREQ_NULLABLE(domain_name, + "backenddomain");
/* checks for fields stored in disk->src */ /* unfortunately 'readonly' and 'shared' can't be converted to tristate
On 3/28/19 10:34 AM, Ján Tomko wrote:
*** BLURB HERE *** *** BLURB THERE *** *** BLURB EVERYWHERE ***
Reviewed-here-by: Laine Stump <laine@laine.org> Reviewed-there-by: Laine Stump <laine@laine.org> Reviewed-everywhere-by: Laine Stump <laine@laine.org>
Ján Tomko (4): Revert "qemu: emit error when trying to update blkiotune group_name in qemuDomainChangeDiskLive" qemu: introduce CHECK_STREQ_NULLABLE in qemuDomainDiskChangeSupported qemu: error out on attempt to change blkiotune group name qemuDomainDiskChangeSupported: use CHECK_STREQ_NULLABLE more
src/qemu/qemu_domain.c | 64 +++++++++++++++++------------------------- 1 file changed, 26 insertions(+), 38 deletions(-)
participants (2)
-
Ján Tomko -
Laine Stump