[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 <pkrempa at 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.