[libvirt] [PATCH 2/4] qemu: introduce CHECK_STREQ_NULLABLE in qemuDomainDiskChangeSupported

[Ján Tomko]

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 at laine.org>
>
>
>(seriously, though - s/marco/macro/ :-)
>
>
>>
>>https://bugzilla.redhat.com/show_bug.cgi?id=1601677
>>
>>Signed-off-by: Ján Tomko <jtomko at 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 at 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;
>>  }
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190329/186d5123/attachment-0001.sig>