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

[Ján Tomko]

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 at 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)) {
-------------- 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/20180824/25b98a0c/attachment-0001.sig>