9:34 a.m.
*** 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
9:34 a.m.
New subject: [libvirt] [PATCH 1/4] Revert "qemu: emit error when trying to update blkiotune group_name in qemuDomainChangeDiskLive"
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(a)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
12:47 p.m.
New subject: [libvirt] [PATCH 1/4] Revert "qemu: emit error when trying to update blkiotune group_name in qemuDomainChangeDiskLive"
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(a)redhat.com>
Reviewed-by: Laine Stump <laine(a)laine.org>
9:34 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: introduce CHECK_STREQ_NULLABLE in qemuDomainDiskChangeSupported
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(a)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
12:54 p.m.
New subject: [libvirt] [PATCH 2/4] qemu: introduce CHECK_STREQ_NULLABLE in qemuDomainDiskChangeSupported
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(a)laine.org>
(seriously, though - s/marco/macro/ :-)
https://bugzilla.redhat.com/show_bug.cgi?id=1601677
Signed-off-by: Ján Tomko <jtomko(a)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(a)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;
}
6:45 a.m.
New subject: [libvirt] [PATCH 2/4] qemu: introduce CHECK_STREQ_NULLABLE in qemuDomainDiskChangeSupported
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(a)laine.org>
(seriously, though - s/marco/macro/ :-)
>
>https://bugzilla.redhat.com/show_bug.cgi?id=1601677
>
>Signed-off-by: Ján Tomko <jtomko(a)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(a)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;
> }
9:34 a.m.
New subject: [libvirt] [PATCH 3/4] qemu: error out on attempt to change blkiotune group name
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(a)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
2:52 p.m.
New subject: [libvirt] [PATCH 3/4] qemu: error out on attempt to change blkiotune group name
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(a)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(a)laine.org>
9:34 a.m.
New subject: [libvirt] [PATCH 4/4] qemuDomainDiskChangeSupported: use CHECK_STREQ_NULLABLE more
Convert the other string comparisons to use the macro.
Signed-off-by: Ján Tomko <jtomko(a)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
12:57 p.m.
New subject: [libvirt] [PATCH 4/4] qemuDomainDiskChangeSupported: use CHECK_STREQ_NULLABLE more
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(a)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(a)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
12:58 p.m.
On 3/28/19 10:34 AM, Ján Tomko wrote:
*** BLURB HERE ***
*** BLURB THERE ***
*** BLURB EVERYWHERE ***
Reviewed-here-by: Laine Stump <laine(a)laine.org>
Reviewed-there-by: Laine Stump <laine(a)laine.org>
Reviewed-everywhere-by: Laine Stump <laine(a)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(-)
2065
days inactive
2066
days old
10 comments
2 participants
participants (2)
-
Ján Tomko -
Laine Stump