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

[Jonathon Jongsma <jjongsma@xxxxxxxxxx>]

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 <pkrempa@xxxxxxxxxx>
> > > ---
> > >    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.