[PATCH] qemu: forbid updating any attributes of an interface <backend> with update-device

[Martin Kletzander]

On Tue, Feb 21, 2023 at 09:11:11AM +0100, Martin Kletzander wrote:
>On Wed, Feb 15, 2023 at 04:06:41PM -0500, Laine Stump wrote:
>>Changing any of the attributes of an <interface>'s <backend> would
>>require removing and re-adding the interface for the new setting to
>>take effect, so fail any update-device that changes anything in
>><backend>
>>
>>Resolves: https://bugzilla.redhat.com/2169245
>>Signed-off-by: Laine Stump <laine at redhat.com>
>>---
>> src/conf/domain_conf.c   | 14 ++++++++++++++
>> src/conf/domain_conf.h   |  2 ++
>> src/libvirt_private.syms |  1 +
>> src/qemu/qemu_hotplug.c  |  9 +++++++++
>> 4 files changed, 26 insertions(+)
>>
>>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>index 3dfc5c87af..aa7bed7dc3 100644
>>--- a/src/conf/domain_conf.c
>>+++ b/src/conf/domain_conf.c
>>@@ -19853,6 +19853,20 @@ virDomainFsDefCheckABIStability(virDomainFSDef *src,
>> }
>>
>>
>>+bool
>>+virDomainNetBackendIsEqual(virDomainNetBackend *src,
>>+                           virDomainNetBackend *dst)
>>+{
>>+    if (src->type != dst->type ||
>>+        STRNEQ_NULLABLE(src->tap, dst->tap) ||
>>+        STRNEQ_NULLABLE(src->vhost, dst->vhost) ||
>>+        STRNEQ_NULLABLE(src->logFile, dst->logFile)) {
>
>I thought you are missing the @upstream member comparison here, but it
>looks like it is not used anywhere in the code.  So I guess
>
>Reviewed-by: Martin Kletzander <mkletzan at redhat.com>
>
>and I'll do some grave-digging about the long lost member.
>

And so I did.  And I'm a bit baffled by the whole function now.

1) There is no check that the sourceDev does not change. OK, I can add
    that.

2) The other interface type using sourceDev (direct) is also not
    checking the change.  OK, maybe we support that?

3) There is no code that changes that device, or copies
    newdev->sourceDev from olddev->sourceDev (like with ->ifname, for
    example)

4) needReconnect is set in couple of places, together with the comment
    "we'll need a full reconnect", giving the impression that the
    function will handle that.  But the value is read in only one place,
    giving an error.

Honestly, I feel uncertain about sending a patch to "fix" this because
I could miss something and I don't feel like digging into this just for
the sake of one member _maybe_ not being accounted for.

>[...]
>
>>diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>index c490e2b97a..b4cddef9f5 100644
>>--- a/src/qemu/qemu_hotplug.c
>>+++ b/src/qemu/qemu_hotplug.c
>>@@ -3675,6 +3675,15 @@ qemuDomainChangeNet(virQEMUDriver *driver,
>>         goto cleanup;
>>     }
>>
>>+    /* nothing in <backend> can be modified in an existing interface -
>>+     * the entire device will need to be removed and re-added.
>
>If I were a nit picker I could tell you to start the sentence with an
>uppercase letter or remove the full stop.  O:-)
>
>>+     */
>>+    if (!virDomainNetBackendIsEqual(&olddev->backend, &newdev->backend)) {
>>+        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>>+                       _("cannot modify network device backend settings"));
>>+        goto cleanup;
>>+    }
>>+
>>     /* allocate new actual device to compare to old - we will need to
>>      * free it if we fail for any reason
>>      */
>>--
>>2.39.1
>>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20230221/ddffff65/attachment.sig>