[PATCH] net: print a more actionable error when slirp is not found

[PATCH] net: print a more actionable error when slirp is not found

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

If slirp is not found during compile-time, and not manually disabled,
print a friendly error message, as suggested in the "If your networking
is failing after updating to the latest git version of QEMU..." thread
by various people.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 meson.build |  4 ++++
 net/net.c   | 19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/meson.build b/meson.build
index 8dc661363f..4f69d7d0b4 100644
--- a/meson.build
+++ b/meson.build
@@ -657,6 +657,10 @@ if not get_option('slirp').auto() or have_system
   endif
 endif
 
+if get_option('slirp').disabled()
+  config_host_data.set('CONFIG_SLIRP_DISABLED', true)
+endif
+
 vde = not_found
 if not get_option('vde').auto() or have_system or have_tools
   vde = cc.find_library('vdeplug', has_headers: ['libvdeplug.h'],
diff --git a/net/net.c b/net/net.c
index 2db160e063..e6072a5ddd 100644
--- a/net/net.c
+++ b/net/net.c
@@ -990,14 +990,29 @@ static int net_init_nic(const Netdev *netdev, const char *name,
     return idx;
 }
 
+#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
+static int net_init_user(const Netdev *netdev, const char *name,
+                         NetClientState *peer, Error **errp)
+{
+#ifdef CONFIG_SLIRP
+    return net_init_slirp(netdev, name, peer, errp);
+#else
+    error_setg(errp,
+               "Type 'user' is not a supported netdev backend by this QEMU build "
+               "because the libslirp development files were not found during build "
+               "of QEMU.");
+#endif
+    return -1;
+}
+#endif
 
 static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
     const Netdev *netdev,
     const char *name,
     NetClientState *peer, Error **errp) = {
         [NET_CLIENT_DRIVER_NIC]       = net_init_nic,
-#ifdef CONFIG_SLIRP
-        [NET_CLIENT_DRIVER_USER]      = net_init_slirp,
+#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
+        [NET_CLIENT_DRIVER_USER]      = net_init_user,
 #endif
         [NET_CLIENT_DRIVER_TAP]       = net_init_tap,
         [NET_CLIENT_DRIVER_SOCKET]    = net_init_socket,
-- 
2.37.3

Re: [PATCH] net: print a more actionable error when slirp is not found

[Christian Schoenebeck]

On Donnerstag, 29. September 2022 18:32:37 CEST Marc-André Lureau wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> If slirp is not found during compile-time, and not manually disabled,
> print a friendly error message, as suggested in the "If your networking
> is failing after updating to the latest git version of QEMU..." thread
> by various people.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  meson.build |  4 ++++
>  net/net.c   | 19 +++++++++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index 8dc661363f..4f69d7d0b4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -657,6 +657,10 @@ if not get_option('slirp').auto() or have_system
>    endif
>  endif
> 
> +if get_option('slirp').disabled()
> +  config_host_data.set('CONFIG_SLIRP_DISABLED', true)
> +endif
> +
>  vde = not_found
>  if not get_option('vde').auto() or have_system or have_tools
>    vde = cc.find_library('vdeplug', has_headers: ['libvdeplug.h'],
> diff --git a/net/net.c b/net/net.c
> index 2db160e063..e6072a5ddd 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -990,14 +990,29 @@ static int net_init_nic(const Netdev *netdev, const
> char *name, return idx;
>  }
> 
> +#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
> +static int net_init_user(const Netdev *netdev, const char *name,
> +                         NetClientState *peer, Error **errp)
> +{
> +#ifdef CONFIG_SLIRP
> +    return net_init_slirp(netdev, name, peer, errp);
> +#else
> +    error_setg(errp,
> +               "Type 'user' is not a supported netdev backend by this QEMU
> build " +               "because the libslirp development files were not
> found during build " +               "of QEMU.");
> +#endif
> +    return -1;
> +}
> +#endif

I just tried this, but somehow it is not working for me. net_init_user() is 
never called and therefore I don't get the error message. That should be 
working if the user launched QEMU without any networking arg, right?

And still, I would find it better if there was also a clear build-time error 
if there was no libslirp and slirp feature was not explicitly disabled.

> 
>  static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>      const Netdev *netdev,
>      const char *name,
>      NetClientState *peer, Error **errp) = {
>          [NET_CLIENT_DRIVER_NIC]       = net_init_nic,
> -#ifdef CONFIG_SLIRP
> -        [NET_CLIENT_DRIVER_USER]      = net_init_slirp,
> +#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
> +        [NET_CLIENT_DRIVER_USER]      = net_init_user,
>  #endif
>          [NET_CLIENT_DRIVER_TAP]       = net_init_tap,
>          [NET_CLIENT_DRIVER_SOCKET]    = net_init_socket,

Re: [PATCH] net: print a more actionable error when slirp is not found

[Marc-André Lureau]

Hi

On Fri, Sep 30, 2022 at 11:49 PM Christian Schoenebeck
<qemu_oss@crudebyte.com> wrote:
>
> On Donnerstag, 29. September 2022 18:32:37 CEST Marc-André Lureau wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > If slirp is not found during compile-time, and not manually disabled,
> > print a friendly error message, as suggested in the "If your networking
> > is failing after updating to the latest git version of QEMU..." thread
> > by various people.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  meson.build |  4 ++++
> >  net/net.c   | 19 +++++++++++++++++--
> >  2 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 8dc661363f..4f69d7d0b4 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -657,6 +657,10 @@ if not get_option('slirp').auto() or have_system
> >    endif
> >  endif
> >
> > +if get_option('slirp').disabled()
> > +  config_host_data.set('CONFIG_SLIRP_DISABLED', true)
> > +endif
> > +
> >  vde = not_found
> >  if not get_option('vde').auto() or have_system or have_tools
> >    vde = cc.find_library('vdeplug', has_headers: ['libvdeplug.h'],
> > diff --git a/net/net.c b/net/net.c
> > index 2db160e063..e6072a5ddd 100644
> > --- a/net/net.c
> > +++ b/net/net.c
> > @@ -990,14 +990,29 @@ static int net_init_nic(const Netdev *netdev, const
> > char *name, return idx;
> >  }
> >
> > +#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
> > +static int net_init_user(const Netdev *netdev, const char *name,
> > +                         NetClientState *peer, Error **errp)
> > +{
> > +#ifdef CONFIG_SLIRP
> > +    return net_init_slirp(netdev, name, peer, errp);
> > +#else
> > +    error_setg(errp,
> > +               "Type 'user' is not a supported netdev backend by this QEMU
> > build " +               "because the libslirp development files were not
> > found during build " +               "of QEMU.");
> > +#endif
> > +    return -1;
> > +}
> > +#endif
>
> I just tried this, but somehow it is not working for me. net_init_user() is
> never called and therefore I don't get the error message. That should be
> working if the user launched QEMU without any networking arg, right?
>

That's because vl.c has:
if (default_net) {
...
#ifdef CONFIG_SLIRP
        qemu_opts_parse(net, "user", true, &error_abort);
#endif

Iow, it doesn't try to use slirp by default if it's not found at
compile time. We can eventually change that, but that might break
existing users who don't build with slirp.

Alternatively, it could error out only if slirp was not explicitly
disabled at configure time.

> And still, I would find it better if there was also a clear build-time error
> if there was no libslirp and slirp feature was not explicitly disabled.

That's not the typical way we deal with dependencies, but I can try to
do that as well.

>
> >
> >  static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
> >      const Netdev *netdev,
> >      const char *name,
> >      NetClientState *peer, Error **errp) = {
> >          [NET_CLIENT_DRIVER_NIC]       = net_init_nic,
> > -#ifdef CONFIG_SLIRP
> > -        [NET_CLIENT_DRIVER_USER]      = net_init_slirp,
> > +#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
> > +        [NET_CLIENT_DRIVER_USER]      = net_init_user,
> >  #endif
> >          [NET_CLIENT_DRIVER_TAP]       = net_init_tap,
> >          [NET_CLIENT_DRIVER_SOCKET]    = net_init_socket,
>
>
>
>

Re: [PATCH] net: print a more actionable error when slirp is not found

[Jakob Bohm]

On 02/10/2022 15:49, Marc-André Lureau wrote:
> Hi
>
> On Fri, Sep 30, 2022 at 11:49 PM Christian Schoenebeck
> <qemu_oss@crudebyte.com> wrote:
>> On Donnerstag, 29. September 2022 18:32:37 CEST Marc-André Lureau wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>
>>> If slirp is not found during compile-time, and not manually disabled,
>>> print a friendly error message, as suggested in the "If your networking
>>> is failing after updating to the latest git version of QEMU..." thread
>>> by various people.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>   meson.build |  4 ++++
>>>   net/net.c   | 19 +++++++++++++++++--
>>>   2 files changed, 21 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/meson.build b/meson.build
>>> index 8dc661363f..4f69d7d0b4 100644
>>> --- a/meson.build
>>> +++ b/meson.build
>>> @@ -657,6 +657,10 @@ if not get_option('slirp').auto() or have_system
>>>     endif
>>>   endif
>>>
>>> +if get_option('slirp').disabled()
>>> +  config_host_data.set('CONFIG_SLIRP_DISABLED', true)
>>> +endif
>>> +
>>>   vde = not_found
>>>   if not get_option('vde').auto() or have_system or have_tools
>>>     vde = cc.find_library('vdeplug', has_headers: ['libvdeplug.h'],
>>> diff --git a/net/net.c b/net/net.c
>>> index 2db160e063..e6072a5ddd 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -990,14 +990,29 @@ static int net_init_nic(const Netdev *netdev, const
>>> char *name, return idx;
>>>   }
>>>
>>> +#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
>>> +static int net_init_user(const Netdev *netdev, const char *name,
>>> +                         NetClientState *peer, Error **errp)
>>> +{
>>> +#ifdef CONFIG_SLIRP
>>> +    return net_init_slirp(netdev, name, peer, errp);
>>> +#else
>>> +    error_setg(errp,
>>> +               "Type 'user' is not a supported netdev backend by this QEMU
>>> build " +               "because the libslirp development files were not
>>> found during build " +               "of QEMU.");
>>> +#endif
>>> +    return -1;
>>> +}
>>> +#endif
>> I just tried this, but somehow it is not working for me. net_init_user() is
>> never called and therefore I don't get the error message. That should be
>> working if the user launched QEMU without any networking arg, right?
>>
> That's because vl.c has:
> if (default_net) {
> ...
> #ifdef CONFIG_SLIRP
>          qemu_opts_parse(net, "user", true, &error_abort);
> #endif
>
> Iow, it doesn't try to use slirp by default if it's not found at
> compile time. We can eventually change that, but that might break
> existing users who don't build with slirp.
>
> Alternatively, it could error out only if slirp was not explicitly
> disabled at configure time.
>
>> And still, I would find it better if there was also a clear build-time error
>> if there was no libslirp and slirp feature was not explicitly disabled.
> That's not the typical way we deal with dependencies, but I can try to
> do that as well.
Maybe change that ifdef section to report the error early instead of 
introducing the new
helper function, something like

  #ifdef CONFIG_SLIRP
          qemu_opts_parse(net, "user", true, &error_abort);
+        // Explicit error messages, because it is not obvious to users that
+        //     "user" networking is based on code from libslirp.
+#elif !defined(CONFIG_SLIRP_DISABLED))
+        some_error_function(
+            "Type 'user' is not a supported netdev backend by this QEMU build "
+            "because the libslirp development files were not found during build "
+            "of QEMU.");
+#else
+        some_error_function(
+            "Type 'user' is not a supported netdev backend by this QEMU build "
+            "because QEMU was explicitly built without libslirp");
  #endif

Also output these messages when the user backend is explicitly requested 
and not CONFIG_SLIRP.

>>>   static int (* const net_client_init_fun[NET_CLIENT_DRIVER__MAX])(
>>>       const Netdev *netdev,
>>>       const char *name,
>>>       NetClientState *peer, Error **errp) = {
>>>           [NET_CLIENT_DRIVER_NIC]       = net_init_nic,
>>> -#ifdef CONFIG_SLIRP
>>> -        [NET_CLIENT_DRIVER_USER]      = net_init_slirp,
>>> +#if (defined(CONFIG_SLIRP) || !defined(CONFIG_SLIRP_DISABLED))
>>> +        [NET_CLIENT_DRIVER_USER]      = net_init_user,
>>>   #endif
>>>           [NET_CLIENT_DRIVER_TAP]       = net_init_tap,
>>>           [NET_CLIENT_DRIVER_SOCKET]    = net_init_socket,
>>>


Enjoy

Jakob
-- 
Jakob Bohm, CIO, Partner, WiseMo A/S.  http://www.wisemo.com
Transformervej 29, 2860 Soborg, Denmark.  Direct +45 31 13 16 10
This public discussion message is non-binding and may contain errors.
WiseMo - Remote Service Management for PCs, Phones and Embedded