Re: [PATCH 34/37] qemu: slirp: Pass FDs to qemu via qemuFDPass in the network private data

On Thu, May 12, 2022 at 14:07:00 -0500, Jonathon Jongsma wrote: > On 5/10/22 10:20 AM, Peter Krempa wrote: >> Populate the 'slirpfd' qemuFDPass structure inside the private data for >> passing the fd to qemu rather than using out-of-band variables. >> >> Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; >> --- >> src/qemu/qemu_command.c | 27 +++++++++------------------ >> src/qemu/qemu_command.h | 3 +-- >> src/qemu/qemu_hotplug.c | 9 +++------ >> src/qemu/qemu_slirp.c | 8 +++++++- >> 4 files changed, 20 insertions(+), 27 deletions(-) >> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index 92b91b0a52..e48b59abbb 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -4186,8 +4186,7 @@ qemuBuildNicDevProps(virDomainDef *def, >> >> >> virJSONValue * >> -qemuBuildHostNetProps(virDomainNetDef *net, >> - const char *slirpfd) >> +qemuBuildHostNetProps(virDomainNetDef *net) >> { >> virDomainNetType netType = virDomainNetGetActualType(net); >> size_t i; >> @@ -4302,9 +4301,11 @@ qemuBuildHostNetProps(virDomainNetDef *net, >> break; >> >> case VIR_DOMAIN_NET_TYPE_USER: >> - if (slirpfd) { >> - if (virJSONValueObjectAdd(&netprops, "s:type", "socket", NULL) < 0 || >> - virJSONValueObjectAppendString(netprops, "fd", slirpfd) < 0) >> + if (netpriv->slirpfd) { >> + if (virJSONValueObjectAdd(&netprops, >> + "s:type", "socket", >> + "s:fd", qemuFDPassGetPath(netpriv->slirpfd), >> + NULL) < 0) >> return NULL; >> } else { >> if (virJSONValueObjectAdd(&netprops, "s:type", "user", NULL) < 0) >> @@ -8760,11 +8761,9 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, >> int ret = -1; >> g_autoptr(virJSONValue) nicprops = NULL; >> g_autofree char *nic = NULL; >> - g_autofree char *slirpfdName = NULL; >> virDomainNetType actualType = virDomainNetGetActualType(net); >> const virNetDevBandwidth *actualBandwidth; >> bool requireNicdev = false; >> - qemuSlirp *slirp; >> g_autoptr(virJSONValue) hostnetprops = NULL; >> qemuDomainNetworkPrivate *netpriv = QEMU_DOMAIN_NETWORK_PRIVATE(net); >> GSList *n; >> @@ -8890,14 +8889,6 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, >> virNetDevSetMTU(net->ifname, net->mtu) < 0) >> goto cleanup; >> >> - slirp = QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp; >> - if (slirp && !standalone) { >> - int slirpfd = qemuSlirpGetFD(slirp); >> - virCommandPassFD(cmd, slirpfd, >> - VIR_COMMAND_PASS_FD_CLOSE_PARENT); >> - slirpfdName = g_strdup_printf("%d", slirpfd); >> - } >> - >> for (n = netpriv->tapfds; n; n = n->next) { >> if (qemuFDPassTransferCommand(n->data, cmd) < 0) >> return -1; >> @@ -8908,11 +8899,11 @@ qemuBuildInterfaceCommandLine(virQEMUDriver *driver, >> return -1; >> } >> >> - if (qemuFDPassTransferCommand(netpriv->vdpafd, cmd) < 0) >> + if (qemuFDPassTransferCommand(netpriv->slirpfd, cmd) < 0 || >> + qemuFDPassTransferCommand(netpriv->vdpafd, cmd) < 0) >> return -1; > > > If I'm reading this correctly, there's a small behavior change here when > 'standalone' is true. Previously we were not passing the slirpfd in that > case, but now it looks like we will to pass it regardless of the value of > 'standalone'. This should not be the case: netpriv->slirpfd is initialized in qemuSlirpStart, which is called from qemuExtDevicesStart which in turn is called from qemuProcessLaunch. Now qemuProcessLaunch calls the command line formatting machinery with standalone=false. Now the other code path which allows control of the 'standalone' parameter is via qemuProcessCreatePretendCmdBuild, which actually skips the call to qemuExtDevicesStart, thus the qemuFDPass object will not be filled. Now I think I'll actually remove the 'standalone' flag altogether as the only thing that it does is that the 'vhost' fds are not formatted, but in any case where we format those we already do use FD passing for the TAP device file descriptors so it won't make the commandline magically usable without libvirt anyways.