[PATCH] qemu: fix multiple missing setup/teardown of passt process for interface type='vhostuser'
passt networking support was originally added only for <interface type='user'>, and all of the codepaths leading to qemuPasst*() functions were protected with if (net->type == VIR_DOMAIN_NET_TYPE_USER && net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) When support was later added to use a vhost-user socket to connect between the passt process and qemu process, *some* of the conditionals similar to the above were changed to be if ((net->type == VIR_DOMAIN_NET_TYPE_USER || net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) && net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) As a matter of fact, enough of these places were changed to make passt+vhostuser work. However I missed a few places that resulted in the passt process not being properly shutdown/cleaned up when the interface type was vhostuser, and also as far as I can see from examining the code, the passt process wasn't being added to the cgroup for the domain. We could fix these problems by adding the extra condition to all the missing places (checking for either 'user' or 'vhostuser' as well as for backend type of 'passt'), but since validation already guarantees that if backend type='passt' then the interface type MUST be either 'user' or 'vhostuser', it's really just adding extra code for no good purpose (and would leave open the possibility of the same problem recurring in the future if a different interface type begins using passt as well). So the better solution is to not bother checking net->type at all in those locations - if backend type is 'passt' then we call the passt-related code. Resolves: https://issues.redhat.com/browse/RHEL-80285 Resolves: https://issues.redhat.com/browse/RHEL-92842 Fixes: 1e9054b9c79d721a55f413c2983c5370044f8f60 Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_extdevice.c | 15 +++------------ src/qemu/qemu_hotplug.c | 23 ++++++++--------------- 2 files changed, 11 insertions(+), 27 deletions(-) diff --git a/src/qemu/qemu_extdevice.c b/src/qemu/qemu_extdevice.c index 8df93a77ce..28cea52980 100644 --- a/src/qemu/qemu_extdevice.c +++ b/src/qemu/qemu_extdevice.c @@ -213,11 +213,6 @@ qemuExtDevicesStart(virQEMUDriver *driver, for (i = 0; i < def->nnets; i++) { virDomainNetDef *net = def->nets[i]; - if (net->type != VIR_DOMAIN_NET_TYPE_USER && - net->type != VIR_DOMAIN_NET_TYPE_VHOSTUSER) { - continue; - } - if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { if (qemuPasstStart(vm, net) < 0) return -1; @@ -310,10 +305,8 @@ qemuExtDevicesStop(virQEMUDriver *driver, if (slirp) qemuSlirpStop(slirp, vm, driver, net); - if (net->type == VIR_DOMAIN_NET_TYPE_USER && - net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { + if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) qemuPasstStop(vm, net); - } if (actualType == VIR_DOMAIN_NET_TYPE_ETHERNET && net->downscript) virNetDevRunEthernetScript(net->ifname, net->downscript); @@ -373,8 +366,7 @@ qemuExtDevicesHasDevice(virDomainDef *def) if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp) return true; - if (net->type == VIR_DOMAIN_NET_TYPE_USER && - net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) + if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) return true; } @@ -455,8 +447,7 @@ qemuExtDevicesSetupCgroup(virQEMUDriver *driver, if (slirp && qemuSlirpSetupCgroup(slirp, cgroup) < 0) return -1; - if (net->type == VIR_DOMAIN_NET_TYPE_USER && - net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST && + if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST && qemuPasstSetupCgroup(vm, net, cgroup) < 0) { return -1; } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e9568af125..866dbace90 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1298,8 +1298,6 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, qemuPasstPrepareVhostUser(vm, net); - if (qemuPasstStart(vm, net) < 0) - goto cleanup; } else { if (virNetDevOpenvswitchGetVhostuserIfname(net->data.vhostuser->data.nix.path, net->data.vhostuser->data.nix.listen, @@ -1313,13 +1311,8 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, break; case VIR_DOMAIN_NET_TYPE_USER: - if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { - - if (qemuPasstStart(vm, net) < 0) - goto cleanup; - - } else if (!priv->disableSlirp && - virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { + if (!priv->disableSlirp && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) { if (qemuInterfacePrepareSlirp(driver, net) < 0) goto cleanup; @@ -1356,6 +1349,10 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, goto cleanup; } + /* both NET_TYPE_USER and NET_TYPE_VHOSTUSER might use passt */ + if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST && qemuPasstStart(vm, net) < 0) + goto cleanup; + /* Set device online immediately */ if (virDomainInterfaceStartDevice(net) < 0) goto cleanup; @@ -1539,10 +1536,8 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp) qemuSlirpStop(QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp, vm, driver, net); - if (net->type == VIR_DOMAIN_NET_TYPE_USER && - net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { + if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) qemuPasstStop(vm, net); - } qemuDomainObjEnterMonitor(vm); if (charDevPlugged && @@ -5112,10 +5107,8 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver, if (QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp) qemuSlirpStop(QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp, vm, driver, net); - if (net->type == VIR_DOMAIN_NET_TYPE_USER && - net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) { + if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST) qemuPasstStop(vm, net); - } virDomainAuditNet(vm, net, NULL, "detach", true); -- 2.51.0
On Sun, Aug 24, 2025 at 11:49:10PM -0400, Laine Stump via Devel wrote:
passt networking support was originally added only for <interface type='user'>, and all of the codepaths leading to qemuPasst*() functions were protected with
if (net->type == VIR_DOMAIN_NET_TYPE_USER && net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST)
When support was later added to use a vhost-user socket to connect between the passt process and qemu process, *some* of the conditionals similar to the above were changed to be
if ((net->type == VIR_DOMAIN_NET_TYPE_USER || net->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER) && net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST)
As a matter of fact, enough of these places were changed to make passt+vhostuser work. However I missed a few places that resulted in the passt process not being properly shutdown/cleaned up when the interface type was vhostuser, and also as far as I can see from examining the code, the passt process wasn't being added to the cgroup for the domain.
We could fix these problems by adding the extra condition to all the missing places (checking for either 'user' or 'vhostuser' as well as for backend type of 'passt'), but since validation already guarantees that if backend type='passt' then the interface type MUST be either 'user' or 'vhostuser', it's really just adding extra code for no good purpose (and would leave open the possibility of the same problem recurring in the future if a different interface type begins using passt as well). So the better solution is to not bother checking net->type at all in those locations - if backend type is 'passt' then we call the passt-related code.
Resolves: https://issues.redhat.com/browse/RHEL-80285 Resolves: https://issues.redhat.com/browse/RHEL-92842 Fixes: 1e9054b9c79d721a55f413c2983c5370044f8f60 Signed-off-by: Laine Stump <laine@redhat.com>
Great, so our new list works ;) With one nitpick in the code below, Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
--- src/qemu/qemu_extdevice.c | 15 +++------------ src/qemu/qemu_hotplug.c | 23 ++++++++--------------- 2 files changed, 11 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e9568af125..866dbace90 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1356,6 +1349,10 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, goto cleanup; }
+ /* both NET_TYPE_USER and NET_TYPE_VHOSTUSER might use passt */ + if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST && qemuPasstStart(vm, net) < 0)
If it's not that much of a bother, I'd read it way faster if the condition was split into two lines like this: if (net->backend.type == VIR_DOMAIN_NET_BACKEND_PASST && qemuPasstStart(vm, net) < 0) Thanks
+ goto cleanup; + /* Set device online immediately */ if (virDomainInterfaceStartDevice(net) < 0) goto cleanup;
participants (2)
-
Laine Stump -
Martin Kletzander