1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v2] 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/20180809092152.10128-1-kkoukiou@redhat.com
Test syntax-check passed
There is a newer version of this series
src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 16 deletions(-)
[libvirt] [PATCH v2] 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 successfull 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>
---
 src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 1488f0a7c2..76ab56a479 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 fill the missing newdev->info
+     * from olddev and then check for changes.
      */
-    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,50 @@ 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 it from olddev */
+    if (!virDomainDeviceAddressIsValid(&newdev->info,
+                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
+        newdev->info.addr.pci = olddev->info.addr.pci;
+    }
+    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 v2] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion
Posted by Ján Tomko 5 years, 8 months ago 
On Thu, Aug 09, 2018 at 11:21:52AM +0200, Katerina Koukiou wrote:
>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 successfull updates, because some

*successful

>changed attributes got overwritten before the validity checks.

The more I think about this 'feature' that allows you to omit parts of
the changed XML, the weirder it gets.

>
>https://bugzilla.redhat.com/show_bug.cgi?id=1599513
>
>Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
>---
> src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++++++----------------
> 1 file changed, 26 insertions(+), 16 deletions(-)
>
>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>index 1488f0a7c2..76ab56a479 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 fill the missing newdev->info
>+     * from olddev and then check for changes.

Maybe the other way around (checking first - with respect to what was
specified in the XML, then overwriting) might be cleaner, see below.

>      */
>-    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,50 @@ 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 it from olddev */
>+    if (!virDomainDeviceAddressIsValid(&newdev->info,
>+                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
>+        newdev->info.addr.pci = olddev->info.addr.pci;

This is an insufficient way to copy the address. The 'addr' union might
be containing an address of a different type. And the 'info.type' attribute also
needs to be copied, either 1) manually or via a 2) DeviceInfoCopy call.

Also, the checks can be moved to a separate fuction first (qemuDomainChangeNet
is over 400 lines long now), e.g. qemuDomainChangeNetAllowed.

Then either:
1) change the unconditional copy of the attributes not present in newdev
   to call another helper (DeviceInfoCopyIfNeeded? MaybeCopy?) that only fills
   the missing parts; or
2) Make qemuDomainChangeNetAllowed only look at specified attributes and
   move the DeviceInfoCopy call after it. (Also, to prevent memory
   leaks, the original info should be cleared before calling copy)

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
Re: [libvirt] [PATCH v2] qemu: qemuDomainChangeNet: validity checks should be done before XML autocompletion
Posted by Katerina Koukiou 5 years, 8 months ago 
On Thu, Aug 09, 2018 at 05:50:11PM +0200, Ján Tomko wrote:
> On Thu, Aug 09, 2018 at 11:21:52AM +0200, Katerina Koukiou wrote:
> > 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 successfull updates, because some
> 
> *successful
> 
> > changed attributes got overwritten before the validity checks.
> 
> The more I think about this 'feature' that allows you to omit parts of
> the changed XML, the weirder it gets.
> 
> > 
> > https://bugzilla.redhat.com/show_bug.cgi?id=1599513
> > 
> > Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
> > ---
> > src/qemu/qemu_hotplug.c | 42 +++++++++++++++++++++++++----------------
> > 1 file changed, 26 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> > index 1488f0a7c2..76ab56a479 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 fill the missing newdev->info
> > +     * from olddev and then check for changes.
> 
> Maybe the other way around (checking first - with respect to what was
> specified in the XML, then overwriting) might be cleaner, see below.
> 
> >      */
> > -    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,50 @@ 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 it from olddev */
> > +    if (!virDomainDeviceAddressIsValid(&newdev->info,
> > +                                       VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI)) {
> > +        newdev->info.addr.pci = olddev->info.addr.pci;
> 
> This is an insufficient way to copy the address. The 'addr' union might
> be containing an address of a different type. And the 'info.type' attribute also
> needs to be copied, either 1) manually or via a 2) DeviceInfoCopy call.

I am going to add the type check before the addr check. Good point.

> 
> Also, the checks can be moved to a separate fuction first (qemuDomainChangeNet
> is over 400 lines long now), e.g. qemuDomainChangeNetAllowed.

I am against splitting the checks and autofilling in different
functions, since it would be harder to spot if someone who is doing some
change changed both parts.

Katerina

> 
> Then either:
> 1) change the unconditional copy of the attributes not present in newdev
>   to call another helper (DeviceInfoCopyIfNeeded? MaybeCopy?) that only fills
>   the missing parts; or
> 2) Make qemuDomainChangeNetAllowed only look at specified attributes and
>   move the DeviceInfoCopy call after it. (Also, to prevent memory
>   leaks, the original info should be cleared before calling copy)
> 
> 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.