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

[Ján Tomko]

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 at 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)) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180809/9220731e/attachment-0001.sig>