1. Patchew
  2. Libvirt
  3. View series

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

Katerina Koukiou posted 1 patch 7 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180801103713.11698-1-kkoukiou@redhat.com
Test syntax-check passed
There is a newer version of this series
src/qemu/qemu_hotplug.c | 43 ++++++++++++++++++++++++++---------------
1 file changed, 27 insertions(+), 16 deletions(-)
[libvirt] [PATCH] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion
Posted by Katerina Koukiou 7 years, 6 months ago 
We need to first perform the checks for changed/missing elements
and then we can overwrite the missing ones. Otherwise we might
falsely report successfull update, because some elements got
overwritten before the validity checks.

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

Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
---
 src/qemu/qemu_hotplug.c | 43 ++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1488f0a7c2..8d98c149e2 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3445,23 +3445,9 @@ 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 check for changes and
+     * then fill the missing newdev->info from olddev.
      */
-    if (!virDomainDeviceAddressIsValid(&newdev->info,
-                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
-        virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) {
-        goto cleanup;
-    }
-    if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci,
-                                  &newdev->info.addr.pci)) {
-        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-                       _("cannot modify network device guest PCI address"));
-        goto cleanup;
-    }
     /* grab alias from olddev if not set in newdev */
     if (!newdev->info.alias &&
         VIR_STRDUP(newdev->info.alias, olddev->info.alias) < 0)
@@ -3469,26 +3455,51 @@ 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"));
         goto cleanup;
     }
+
+    /* if pci addr is missing or is invalid we overwrite all info struct */
+    if (!virDomainDeviceAddressIsValid(&newdev->info,
+                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
+        virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) {
+        goto cleanup;
+    }
+    if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci,
+                                  &newdev->info.addr.pci)) {
+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+                       _("cannot modify network device guest PCI address"));
+        goto cleanup;
+    }
     /* (end of device info checks) */
 
     if (STRNEQ_NULLABLE(olddev->filter, newdev->filter) ||
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion
Posted by Ján Tomko 7 years, 6 months ago 
On Wed, Aug 01, 2018 at 12:37:13PM +0200, Katerina Koukiou wrote:
>We need to first perform the checks for changed/missing elements

If we copy all the missing elements,

>and then we can overwrite the missing ones. Otherwise we might

there's no need to overwrite them again.

>falsely report successfull update, because some elements got
>overwritten before the validity checks.
>
>https://bugzilla.redhat.com/show_bug.cgi?id=1599513
>
>Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
>---
> src/qemu/qemu_hotplug.c | 43 ++++++++++++++++++++++++++---------------
> 1 file changed, 27 insertions(+), 16 deletions(-)
>
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index 1488f0a7c2..8d98c149e2 100644
>--- a/src/qemu/qemu_hotplug.c
>+++ b/src/qemu/qemu_hotplug.c
>@@ -3445,23 +3445,9 @@ 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 check for changes and
>+     * then fill the missing newdev->info from olddev.
>      */
>-    if (!virDomainDeviceAddressIsValid(&newdev->info,
>-                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) &&
>-        virDomainDeviceInfoCopy(&newdev->info, &olddev->info) < 0) {

IOW we can get rid of 'virDomainDeviceInfoCopy' completely.
Here we only need to check and copy the PCI address,

>-        goto cleanup;
>-    }
>-    if (!virPCIDeviceAddressEqual(&olddev->info.addr.pci,
>-                                  &newdev->info.addr.pci)) {
>-        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>-                       _("cannot modify network device guest PCI address"));
>-        goto cleanup;
>-    }
>     /* grab alias from olddev if not set in newdev */
>     if (!newdev->info.alias &&
>         VIR_STRDUP(newdev->info.alias, olddev->info.alias) < 0)
>@@ -3469,26 +3455,51 @@ 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;

Because all the other attributes are filled out here.

Jano

>     if (olddev->info.romenabled != newdev->info.romenabled) {
>         virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>                        _("cannot modify network device rom enabled setting"));
>         goto cleanup;
>     }
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.