Re: [libvirt PATCH] qemu: honour fatal errors dealing with qemu slirp helper

[Daniel Henrique Barboza <danielhb413@xxxxxxxxx>]

On 10/20/20 12:07 PM, Daniel P. Berrangé wrote:
> Currently all errors from qemuInterfacePrepareSlirp() are completely
> ignored by the callers. The intention is that missing qemu-slirp binary
> should cause the caller to fallback to the built-in slirp impl.
>
> Many of the possible errors though should indeed be considered fatal.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---

Just a FYI: there is a trivial conflict in qemu_interface.h when applying
it to current master.


Reviewed-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx>

>   src/qemu/qemu_hotplug.c   |  7 +++++--
>   src/qemu/qemu_interface.c | 21 +++++++++++++++------
>   src/qemu/qemu_interface.h |  5 +++--
>   src/qemu/qemu_process.c   |  8 ++++++--
>   src/qemu/qemu_slirp.c     |  3 ---
>   5 files changed, 29 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 79fc8baa5c..dc998236de 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1311,9 +1311,12 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>       case VIR_DOMAIN_NET_TYPE_USER:
>           if (!priv->disableSlirp &&
>               virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
> -            qemuSlirpPtr slirp = qemuInterfacePrepareSlirp(driver, net);
> +            qemuSlirpPtr slirp = NULL;
> +            int rv = qemuInterfacePrepareSlirp(driver, net, &slirp);
>   
> -            if (!slirp)
> +            if (rv == -1)
> +                return -1;
> +            if (rv == 0)
>                   break;
>   
>               QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp;
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index cbf3d99981..b4ab809970 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -636,30 +636,39 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
>   }
>   
>   
> -qemuSlirpPtr
> +/*
> + * Returns: -1 on error, 0 if slirp isn't available, 1 on succcess
> + */
> +int
>   qemuInterfacePrepareSlirp(virQEMUDriverPtr driver,
> -                          virDomainNetDefPtr net)
> +                          virDomainNetDefPtr net,
> +                          qemuSlirpPtr *slirpret)
>   {
>       g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>       g_autoptr(qemuSlirp) slirp = NULL;
>       size_t i;
>   
> +    if (!cfg->slirpHelperName ||
> +        !virFileExists(cfg->slirpHelperName))
> +        return 0; /* fallback to builtin slirp impl */
> +
>       if (!(slirp = qemuSlirpNewForHelper(cfg->slirpHelperName)))
> -        return NULL;
> +        return -1;
>   
>       for (i = 0; i < net->guestIP.nips; i++) {
>           const virNetDevIPAddr *ip = net->guestIP.ips[i];
>   
>           if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET) &&
>               !qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_IPV4))
> -            return NULL;
> +            return 0;
>   
>           if (VIR_SOCKET_ADDR_IS_FAMILY(&ip->address, AF_INET6) &&
>               !qemuSlirpHasFeature(slirp, QEMU_SLIRP_FEATURE_IPV6))
> -            return NULL;
> +            return 0;
>       }
>   
> -    return g_steal_pointer(&slirp);
> +    *slirpret = g_steal_pointer(&slirp);
> +    return 1;
>   }
>   
>   
> diff --git a/src/qemu/qemu_interface.h b/src/qemu/qemu_interface.h
> index 3dcefc6a12..b5e91e3ab2 100644
> --- a/src/qemu/qemu_interface.h
> +++ b/src/qemu/qemu_interface.h
> @@ -56,5 +56,6 @@ int qemuInterfaceOpenVhostNet(virDomainDefPtr def,
>                                 int *vhostfd,
>                                 size_t *vhostfdSize) G_GNUC_NO_INLINE;
>   
> -qemuSlirpPtr qemuInterfacePrepareSlirp(virQEMUDriverPtr driver,
> -                                       virDomainNetDefPtr net);
> +int qemuInterfacePrepareSlirp(virQEMUDriverPtr driver,
> +                              virDomainNetDefPtr net,
> +                              qemuSlirpPtr *slirp);
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 5bc76a75e3..59206a17fb 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -5697,9 +5697,13 @@ qemuProcessNetworkPrepareDevices(virQEMUDriverPtr driver,
>           } else if (actualType == VIR_DOMAIN_NET_TYPE_USER &&
>                      !priv->disableSlirp &&
>                      virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DBUS_VMSTATE)) {
> -            qemuSlirpPtr slirp = qemuInterfacePrepareSlirp(driver, net);
> +            qemuSlirpPtr slirp = NULL;
> +            int rv = qemuInterfacePrepareSlirp(driver, net, &slirp);
>   
> -            QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp;
> +            if (rv == -1)
> +                return -1;
> +            if (rv == 1)
> +                QEMU_DOMAIN_NETWORK_PRIVATE(net)->slirp = slirp;
>            }
>   
>       }
> diff --git a/src/qemu/qemu_slirp.c b/src/qemu/qemu_slirp.c
> index d2e4ed79be..dfb36125f0 100644
> --- a/src/qemu/qemu_slirp.c
> +++ b/src/qemu/qemu_slirp.c
> @@ -101,9 +101,6 @@ qemuSlirpNewForHelper(const char *helper)
>       virJSONValuePtr featuresJSON;
>       size_t i, nfeatures;
>   
> -    if (!helper)
> -        return NULL;
> -
>       slirp = qemuSlirpNew();
>       if (!slirp) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,