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@xxxxxxxxx> (seriously, though - s/marco/macro/ :-)https://bugzilla.redhat.com/show_bug.cgi?id=1601677 Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx> --- 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@xxxxxxxxx>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; }
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
