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

[Jonathon Jongsma]

On 5/13/22 6:45 AM, Peter Krempa wrote:
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 <pkre...@redhat.com>
---
   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.


OK, I guess I missed that.

Also, the name of the 'standalone' argument never really gave me a very 
good sense of what it was supposed to mean, so I'm in favor of removing 
it if possible.