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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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;
>   }


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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;
>>  }
>
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list