1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v3] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion

Katerina Koukiou posted 1 patch 5 years, 8 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180824105440.13959-1-kkoukiou@redhat.com
Test syntax-check passed
There is a newer version of this series
src/qemu/qemu_hotplug.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
[libvirt] [PATCH v3] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion
Posted by Katerina Koukiou 5 years, 8 months ago 
This patch ensures that changes in attributes of interfaces will be emit
errors accept if they are missing from the XML.
Previously we were falsely reporting successful updates, because some
changed attributes got overwritten before the validity checks.

https://bugzilla.redhat.com/show_bug.cgi?id=1599513

Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
---
Changes from v2:
* Added check for type element in info struct.
* Moved the addr checks at start the the section with info checks.


 src/qemu/qemu_hotplug.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0b84a503bb..f9805627b7 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3598,16 +3598,22 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    /* info: if newdev->info is empty, fill it in from olddev,
-     * otherwise verify that it matches - nothing is allowed to
-     * change. (There is no helper function to do this, so
-     * individually check the few feidls of virDomainDeviceInfo that
-     * are relevant in this case).
+    /* info: Nothing is allowed to change. First fill the missing newdev->info
+     * from olddev and then check for changes.
      */
+
+    /* if addr type is missing overwrite if from olddev */
+    if (newdev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
+        newdev->info.type = olddev->info.type;
+    if (olddev->info.type != newdev->info.type) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot modify network device type"));
+    }
+
+    /* if pci addr is missing or is invalid we overwrite it from olddev */
     if (!virDomainDeviceAddressIsValid(&newdev->info,
-                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
-        virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) {
-        goto cleanup;
+                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
+        newdev->info.addr.pci = olddev->info.addr.pci;
     }
     if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci,
                                   &newdev->info.addr.pci)) {
@@ -3622,21 +3628,33 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
 
     /* device alias is checked already in virDomainDefCompatibleDevice */
 
+    if (newdev->info.rombar == VIR_TRISTATE_BOOL_ABSENT)
+        newdev->info.rombar = olddev->info.rombar;
     if (olddev->info.rombar != newdev->info.rombar) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("cannot modify network device rom bar setting"));
         goto cleanup;
     }
+
+    if (!newdev->info.romfile &&
+        VIR_STRDUP(newdev->info.romfile, olddev->info.romfile) < 0)
+        goto cleanup;
     if (STRNEQ_NULLABLE(olddev->info.romfile, newdev->info.romfile)) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("cannot modify network rom file"));
         goto cleanup;
     }
+
+    if (newdev->info.bootIndex == 0)
+        newdev->info.bootIndex = olddev->info.bootIndex;
     if (olddev->info.bootIndex != newdev->info.bootIndex) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("cannot modify network device boot index setting"));
         goto cleanup;
     }
+
+    if (newdev->info.romenabled == VIR_TRISTATE_BOOL_ABSENT)
+        newdev->info.romenabled = olddev->info.romenabled;
     if (olddev->info.romenabled != newdev->info.romenabled) {
         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                        _("cannot modify network device rom enabled setting"));
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion
Posted by Ján Tomko 5 years, 8 months ago 
On Fri, Aug 24, 2018 at 12:54:40PM +0200, Katerina Koukiou wrote:
>This patch ensures that changes in attributes of interfaces will be emit

s/will be/will/

>errors accept if they are missing from the XML.
>Previously we were falsely reporting successful updates, because some
>changed attributes got overwritten before the validity checks.
>
>https://bugzilla.redhat.com/show_bug.cgi?id=1599513
>
>Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
>---
>Changes from v2:
>* Added check for type element in info struct.
>* Moved the addr checks at start the the section with info checks.
>
>
> src/qemu/qemu_hotplug.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index 0b84a503bb..f9805627b7 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -3598,16 +3598,22 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>         goto cleanup;
>     }
>
>-    /* info: if newdev->info is empty, fill it in from olddev,
>-     * otherwise verify that it matches - nothing is allowed to
>-     * change. (There is no helper function to do this, so
>-     * individually check the few feidls of virDomainDeviceInfo that
>-     * are relevant in this case).
>+    /* info: Nothing is allowed to change. First fill the missing newdev->info
>+     * from olddev and then check for changes.
>      */
>+
>+    /* if addr type is missing overwrite if from olddev */
>+    if (newdev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE)
>+        newdev->info.type = olddev->info.type;

info.type and info.addr are tied together - if you copy the type, you
should copy the address too. (which was done by virDomainDeviceInfoCopy
in the old code). Copying the address conditionally below is asking for
trouble in case we omit some code path where we only copy the type
without the address.

>+    if (olddev->info.type != newdev->info.type) {
>+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>+                       _("cannot modify network device type"));

*network device address type

>+    }
>+

>+    /* if pci addr is missing or is invalid we overwrite it from olddev */
>     if (!virDomainDeviceAddressIsValid(&newdev->info,
>-                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
>-        virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) {
>-        goto cleanup;
>+                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
>+        newdev->info.addr.pci = olddev->info.addr.pci;

This assumes that olddev->info.type is _PCI, because virDomainDeviceAddressIsValid
returns 0 if the types do not match. Rather than adding a check if
info.type == PCI, copy info.addr, not just info.addr.pci.


Also, both checks would look better combined, to avoid the case where we
copied 'type' but not the address:

if (new->type == NONE ||
    !IsValid(new->info, new->type) {
    new->type = old->type
    new->addr = old->addr
}

Jano
>     }
>     if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci,
>                                   &newdev->info.addr.pci)) {
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.