[PATCH] qemu: forbid updating any attributes of an interface <backend> with update-device
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@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)) { + return false; + } + return true; +} + + static bool virDomainNetDefCheckABIStability(virDomainNetDef *src, virDomainNetDef *dst) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7d58efb011..bd8ce562d9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3862,6 +3862,8 @@ virDomainNetDef *virDomainNetFindByName(virDomainDef *def, const char *ifname); bool virDomainHasNet(virDomainDef *def, virDomainNetDef *net); int virDomainNetInsert(virDomainDef *def, virDomainNetDef *net); int virDomainNetUpdate(virDomainDef *def, size_t netidx, virDomainNetDef *newnet); +bool virDomainNetBackendIsEqual(virDomainNetBackend *src, + virDomainNetBackend *dst); int virDomainNetDHCPInterfaces(virDomainDef *def, virDomainInterfacePtr **ifaces); int virDomainNetARPInterfaces(virDomainDef *def, virDomainInterfacePtr **ifaces); virDomainNetDef *virDomainNetRemove(virDomainDef *def, size_t i); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 97c3d86217..c6c47dbfac 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -528,6 +528,7 @@ virDomainMouseModeTypeToString; virDomainNetAllocateActualDevice; virDomainNetAppendIPAddress; virDomainNetARPInterfaces; +virDomainNetBackendIsEqual; virDomainNetBandwidthUpdate; virDomainNetDefActualFromNetworkPort; virDomainNetDefActualToNetworkPort; 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 (!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
Ping On 2/15/23 4:06 PM, 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@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)) { + return false; + } + return true; +} + + static bool virDomainNetDefCheckABIStability(virDomainNetDef *src, virDomainNetDef *dst) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7d58efb011..bd8ce562d9 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3862,6 +3862,8 @@ virDomainNetDef *virDomainNetFindByName(virDomainDef *def, const char *ifname); bool virDomainHasNet(virDomainDef *def, virDomainNetDef *net); int virDomainNetInsert(virDomainDef *def, virDomainNetDef *net); int virDomainNetUpdate(virDomainDef *def, size_t netidx, virDomainNetDef *newnet); +bool virDomainNetBackendIsEqual(virDomainNetBackend *src, + virDomainNetBackend *dst); int virDomainNetDHCPInterfaces(virDomainDef *def, virDomainInterfacePtr **ifaces); int virDomainNetARPInterfaces(virDomainDef *def, virDomainInterfacePtr **ifaces); virDomainNetDef *virDomainNetRemove(virDomainDef *def, size_t i); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 97c3d86217..c6c47dbfac 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -528,6 +528,7 @@ virDomainMouseModeTypeToString; virDomainNetAllocateActualDevice; virDomainNetAppendIPAddress; virDomainNetARPInterfaces; +virDomainNetBackendIsEqual; virDomainNetBandwidthUpdate; virDomainNetDefActualFromNetworkPort; virDomainNetDefActualToNetworkPort; 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 (!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 */
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@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@redhat.com> and I'll do some grave-digging about the long lost member. [...]
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
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@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@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
On 2/21/23 4:20 AM, Martin Kletzander wrote:
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@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@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.
Well, yeah, I did miss that, but on the other hand, that isn't in <backend>, which is what the BZ was filed about, and the subject of the patch (and more specifically of this function). I know that's nitpicking, but that explains why the check isn't here (I mean that, along with the fact that I forgot :-))
2) The other interface type using sourceDev (direct) is also not checking the change. OK, maybe we support that?
Believe me when I say there is *a lot* of stuff not checked in that function; at one time it may have been closer to complete, but things get added and people forget to add them to that function. I definitely should have included a check for ->sourceDev (and all the stuff in ->backend / <backend>) in the original patches.
3) There is no code that changes that device, or copies newdev->sourceDev from olddev->sourceDev (like with ->ifname, for example)
It *shouldn't* be copied from olddev to newdev because it is never autofilled during parse or guest startup / device hotplug - the parts of the object that are copied from old to new (if new->blah is empty) are only those things that get filled in automatically with "something" if the attribute isn't specified in the original XML. If you don't specify <source dev='blah'> then sourceDev remains empty. (And in the case of the <source dev='blah'> for direct (macvtap) interfaces, it is a required field, so it can never be empty, and therefore will never be auto-filled with some value if it's missing) olddev->ifname, on the other hand, almost always is auto-filled by libvirt as the guest is starting (a tap/macvtap device is created using a generated name, and that generated name is stored in ->ifname, which ends up being displayed in <target dev='blah'> when the netdef is formatted to XML). If the user has specified a <target dev> in the update XML, and that dev is different from what's in use, we do need to flag that as an error; but if they don't specify anything for target dev, or if what they specified happens to match what it is already set to, then that isn't an error; the cleanest way to handle those cases properly is to copy the attribute from olddev to newdev iff the attribute is empty in newdev.
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.
needReconnect was a very nice idea, based on the (false) promise that it was going to be possible to restart the netdev (host) side of an emulated network device without completely unplugging it on the guest side. At one point back in 2011 or 2012 or something, *someone* on the QEMU team made a patch that madde this "kind of" work, and so I modified the update function to classify some previously disallowed changes as needing a full "reconnect" (basically remove the current -netdev and replace it with a new one, without the need of completely unplugging the device on the guest side). Until QEMU had that working, a single check for needReconnect at the end was left generating an error. Unfortunately the QEMU side of that never happened (I have a vague memory that the person who at first suggested they could do it, later told me it simply wasn't possible, for [reasons that sounded plausible], so the bit that logs an error when needReconnect == true was never replaced with code to actually implement the reconnect.
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.
Well, you're correct that <source dev='blah'> should be checked for being changed, when the netdev type is 'user' (so it's in ->sourceDev), 'direct' (so it's in ->data.direct.linkdev), and vdpa (so it's in ->data.vdpa.devicepath). But again that's not in <backend>, and there are *so many* other things not being checked for, and it could be a real rabbit hole :-) P.S. I forgot to add your Reviewed-by: before pushing (and also forgot to capitalize the first word of the comment). Sorry. That's what happens when I don't get enough sleep and then wake up early and push something before my brain is fully functioning :-/
[...]
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
On 2/21/23 3:11 AM, 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@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.
Oh right! I noticed the same thing when I noticed this, and had a 1st patch that removed the upstream member, along with pointing out how it got there and why it is no longer needed, but I had forgotten about it by the time I sent the other patch, and continued to forget about it when I sent the ping :-/ But I see you already figured that out, and sent a patch to remove it, while I was sleeping. Thanks! So I guess
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
and thanks!
and I'll do some grave-digging about the long lost member.
And sorry for making you spend the time to go digging.
[...]
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
participants (2)
-
Laine Stump -
Martin Kletzander