| Message ID | 20221003100612.596845-1-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: improve error message for missing netdev backend | expand |
Hi On Mon, Oct 3, 2022 at 2:06 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > The current message when using '-net user...' with SLIRP disabled at > compile time is: > > qemu-system-x86_64: -net user: Parameter 'type' expects a net backend type (maybe it is not compiled into this binary) > > An observation is that we're using the 'netdev->type' field here which > is an enum value, produced after QAPI has converted from its string > form. > > IOW, at this point in the code, we know that the user's specified > type name was a valid network backend. The only possible scenario that > can make the backend init function be NULL, is if support for that > backend was disabled at build time. Given this, we don't need to caveat > our error message with a 'maybe' hint, we can be totally explicit. > > The use of QERR_INVALID_PARAMETER_VALUE doesn't really lend itself to > user friendly error message text. Since this is not used to set a > specific QAPI error class, we can simply stop using this pre-formatted > error text and provide something better. > > Thus the new message is: > > qemu-system-x86_64: -net user: network backend 'user' is not compiled into this binary > > The case of passing 'hubport' for -net is also given a message reminding > people they should have used -netdev/-nic instead, as this backend type > is only valid for the modern syntax. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> lgtm Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > > NB, this does not make any difference to people who were relying on the > QEMU built-in default hub that was created if you don't list any -net / > -netdev / -nic argument, only those using explicit args. > > net/net.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/net/net.c b/net/net.c > index 2db160e063..8ddafacf13 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1036,19 +1036,23 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp) > if (is_netdev) { > if (netdev->type == NET_CLIENT_DRIVER_NIC || > !net_client_init_fun[netdev->type]) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a netdev backend type"); > + error_setg(errp, "network backend '%s' is not compiled into this binary", > + NetClientDriver_str(netdev->type)); > return -1; > } > } else { > if (netdev->type == NET_CLIENT_DRIVER_NONE) { > return 0; /* nothing to do */ > } > - if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || > - !net_client_init_fun[netdev->type]) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a net backend type (maybe it is not compiled " > - "into this binary)"); > + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT) { > + error_setg(errp, "network backend '%s' is only supported with -netdev/-nic", > + NetClientDriver_str(netdev->type)); > + return -1; > + } > + > + if (!net_client_init_fun[netdev->type]) { > + error_setg(errp, "network backend '%s' is not compiled into this binary", > + NetClientDriver_str(netdev->type)); > return -1; > } > > -- > 2.37.3 >
On Montag, 3. Oktober 2022 12:06:12 CEST Daniel P. Berrangé wrote: > The current message when using '-net user...' with SLIRP disabled at > compile time is: > > qemu-system-x86_64: -net user: Parameter 'type' expects a net backend type > (maybe it is not compiled into this binary) Is this intended as alternative to Marc-André's previous patch? If yes, then same applies here: what about people not passing any networking arg to QEMU? They would not get any error message at all, right? https://lore.kernel.org/all/2973900.g0HVWOepMQ@silver/ > An observation is that we're using the 'netdev->type' field here which > is an enum value, produced after QAPI has converted from its string > form. > > IOW, at this point in the code, we know that the user's specified > type name was a valid network backend. The only possible scenario that > can make the backend init function be NULL, is if support for that > backend was disabled at build time. Given this, we don't need to caveat > our error message with a 'maybe' hint, we can be totally explicit. > > The use of QERR_INVALID_PARAMETER_VALUE doesn't really lend itself to > user friendly error message text. Since this is not used to set a > specific QAPI error class, we can simply stop using this pre-formatted > error text and provide something better. > > Thus the new message is: > > qemu-system-x86_64: -net user: network backend 'user' is not compiled into > this binary And why not naming the child, i.e. that QEMU was built without slirp? > The case of passing 'hubport' for -net is also given a message reminding > people they should have used -netdev/-nic instead, as this backend type > is only valid for the modern syntax. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > > NB, this does not make any difference to people who were relying on the > QEMU built-in default hub that was created if you don't list any -net / > -netdev / -nic argument, only those using explicit args. > > net/net.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/net/net.c b/net/net.c > index 2db160e063..8ddafacf13 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1036,19 +1036,23 @@ static int net_client_init1(const Netdev *netdev, > bool is_netdev, Error **errp) if (is_netdev) { > if (netdev->type == NET_CLIENT_DRIVER_NIC || > !net_client_init_fun[netdev->type]) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a netdev backend type"); > + error_setg(errp, "network backend '%s' is not compiled into > this binary", + NetClientDriver_str(netdev->type)); > return -1; > } > } else { > if (netdev->type == NET_CLIENT_DRIVER_NONE) { > return 0; /* nothing to do */ > } > - if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || > - !net_client_init_fun[netdev->type]) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a net backend type (maybe it is not compiled " > - "into this binary)"); > + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT) { > + error_setg(errp, "network backend '%s' is only supported with > -netdev/-nic", + NetClientDriver_str(netdev->type)); > + return -1; > + } > + > + if (!net_client_init_fun[netdev->type]) { > + error_setg(errp, "network backend '%s' is not compiled into > this binary", + NetClientDriver_str(netdev->type)); > return -1; > }
On Mon, Oct 03, 2022 at 02:46:04PM +0200, Christian Schoenebeck wrote: > On Montag, 3. Oktober 2022 12:06:12 CEST Daniel P. Berrangé wrote: > > The current message when using '-net user...' with SLIRP disabled at > > compile time is: > > > > qemu-system-x86_64: -net user: Parameter 'type' expects a net backend type > > (maybe it is not compiled into this binary) > > Is this intended as alternative to Marc-André's previous patch? This is a patch that should be applied regardless of any other change, because the error message we report here today is awful and needs improving. > If yes, then > same applies here: what about people not passing any networking arg to QEMU? > They would not get any error message at all, right? Yes, I mentioned that in the text that you've quoted below.... > > An observation is that we're using the 'netdev->type' field here which > > is an enum value, produced after QAPI has converted from its string > > form. > > > > IOW, at this point in the code, we know that the user's specified > > type name was a valid network backend. The only possible scenario that > > can make the backend init function be NULL, is if support for that > > backend was disabled at build time. Given this, we don't need to caveat > > our error message with a 'maybe' hint, we can be totally explicit. > > > > The use of QERR_INVALID_PARAMETER_VALUE doesn't really lend itself to > > user friendly error message text. Since this is not used to set a > > specific QAPI error class, we can simply stop using this pre-formatted > > error text and provide something better. > > > > Thus the new message is: > > > > qemu-system-x86_64: -net user: network backend 'user' is not compiled into > > this binary > > And why not naming the child, i.e. that QEMU was built without slirp? There are several network backends that can be conditionally disabled at build time, and IMHO its overkill to give a different message for each one. This message is sufficient to show users where to go next. > > > The case of passing 'hubport' for -net is also given a message reminding > > people they should have used -netdev/-nic instead, as this backend type > > is only valid for the modern syntax. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > > > NB, this does not make any difference to people who were relying on the > > QEMU built-in default hub that was created if you don't list any -net / > > -netdev / -nic argument, only those using explicit args. .... here. With regards, Daniel
On Montag, 3. Oktober 2022 14:50:04 CEST Daniel P. Berrangé wrote: > On Mon, Oct 03, 2022 at 02:46:04PM +0200, Christian Schoenebeck wrote: > > On Montag, 3. Oktober 2022 12:06:12 CEST Daniel P. Berrangé wrote: > > > The current message when using '-net user...' with SLIRP disabled at > > > > > > compile time is: > > > qemu-system-x86_64: -net user: Parameter 'type' expects a net backend > > > type > > > > > > (maybe it is not compiled into this binary) > > > > Is this intended as alternative to Marc-André's previous patch? > > This is a patch that should be applied regardless of any other change, > because the error message we report here today is awful and needs > improving. > > > If yes, > > then > > > > same applies here: what about people not passing any networking arg to > > QEMU? They would not get any error message at all, right? > > Yes, I mentioned that in the text that you've quoted below.... Yeah, missed that one, sorry. > > > An observation is that we're using the 'netdev->type' field here which > > > is an enum value, produced after QAPI has converted from its string > > > form. > > > > > > IOW, at this point in the code, we know that the user's specified > > > type name was a valid network backend. The only possible scenario that > > > can make the backend init function be NULL, is if support for that > > > backend was disabled at build time. Given this, we don't need to caveat > > > our error message with a 'maybe' hint, we can be totally explicit. > > > > > > The use of QERR_INVALID_PARAMETER_VALUE doesn't really lend itself to > > > user friendly error message text. Since this is not used to set a > > > specific QAPI error class, we can simply stop using this pre-formatted > > > error text and provide something better. > > > > > > Thus the new message is: > > > qemu-system-x86_64: -net user: network backend 'user' is not compiled > > > into > > > > > > this binary > > > > And why not naming the child, i.e. that QEMU was built without slirp? > > There are several network backends that can be conditionally disabled > at build time, and IMHO its overkill to give a different message for > each one. This message is sufficient to show users where to go next. Yes, but that is not a user friendly error message, especially for people who never dealt with QEMU's networking options before. That message does not make it obvious how to find the solution IMO. What about a web link to the QEMU networking docs where this issue could then be clarified in a more user friendly manner? #anchors_are_cheap > > > The case of passing 'hubport' for -net is also given a message reminding > > > people they should have used -netdev/-nic instead, as this backend type > > > is only valid for the modern syntax. > > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > > --- > > > > > > NB, this does not make any difference to people who were relying on the > > > QEMU built-in default hub that was created if you don't list any -net / > > > -netdev / -nic argument, only those using explicit args. > > .... here. > > > > With regards, > Daniel
On 03/10/2022 12.06, Daniel P. Berrangé wrote: > The current message when using '-net user...' with SLIRP disabled at > compile time is: > > qemu-system-x86_64: -net user: Parameter 'type' expects a net backend type (maybe it is not compiled into this binary) > > An observation is that we're using the 'netdev->type' field here which > is an enum value, produced after QAPI has converted from its string > form. > > IOW, at this point in the code, we know that the user's specified > type name was a valid network backend. The only possible scenario that > can make the backend init function be NULL, is if support for that > backend was disabled at build time. Given this, we don't need to caveat > our error message with a 'maybe' hint, we can be totally explicit. > > The use of QERR_INVALID_PARAMETER_VALUE doesn't really lend itself to > user friendly error message text. Since this is not used to set a > specific QAPI error class, we can simply stop using this pre-formatted > error text and provide something better. > > Thus the new message is: > > qemu-system-x86_64: -net user: network backend 'user' is not compiled into this binary > > The case of passing 'hubport' for -net is also given a message reminding > people they should have used -netdev/-nic instead, as this backend type > is only valid for the modern syntax. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > > NB, this does not make any difference to people who were relying on the > QEMU built-in default hub that was created if you don't list any -net / > -netdev / -nic argument, only those using explicit args. > > net/net.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/net/net.c b/net/net.c > index 2db160e063..8ddafacf13 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1036,19 +1036,23 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp) > if (is_netdev) { > if (netdev->type == NET_CLIENT_DRIVER_NIC || > !net_client_init_fun[netdev->type]) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a netdev backend type"); > + error_setg(errp, "network backend '%s' is not compiled into this binary", > + NetClientDriver_str(netdev->type)); > return -1; > } > } else { > if (netdev->type == NET_CLIENT_DRIVER_NONE) { > return 0; /* nothing to do */ > } > - if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || > - !net_client_init_fun[netdev->type]) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a net backend type (maybe it is not compiled " > - "into this binary)"); > + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT) { > + error_setg(errp, "network backend '%s' is only supported with -netdev/-nic", > + NetClientDriver_str(netdev->type)); > + return -1; > + } > + > + if (!net_client_init_fun[netdev->type]) { > + error_setg(errp, "network backend '%s' is not compiled into this binary", > + NetClientDriver_str(netdev->type)); > return -1; > } > Reviewed-by: Thomas Huth <thuth@redhat.com>
ping: Jason, are you willing to queue this since it has two positive reviews. On Mon, Oct 03, 2022 at 11:06:12AM +0100, Daniel P. Berrangé wrote: > The current message when using '-net user...' with SLIRP disabled at > compile time is: > > qemu-system-x86_64: -net user: Parameter 'type' expects a net backend type (maybe it is not compiled into this binary) > > An observation is that we're using the 'netdev->type' field here which > is an enum value, produced after QAPI has converted from its string > form. > > IOW, at this point in the code, we know that the user's specified > type name was a valid network backend. The only possible scenario that > can make the backend init function be NULL, is if support for that > backend was disabled at build time. Given this, we don't need to caveat > our error message with a 'maybe' hint, we can be totally explicit. > > The use of QERR_INVALID_PARAMETER_VALUE doesn't really lend itself to > user friendly error message text. Since this is not used to set a > specific QAPI error class, we can simply stop using this pre-formatted > error text and provide something better. > > Thus the new message is: > > qemu-system-x86_64: -net user: network backend 'user' is not compiled into this binary > > The case of passing 'hubport' for -net is also given a message reminding > people they should have used -netdev/-nic instead, as this backend type > is only valid for the modern syntax. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > > NB, this does not make any difference to people who were relying on the > QEMU built-in default hub that was created if you don't list any -net / > -netdev / -nic argument, only those using explicit args. > > net/net.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/net/net.c b/net/net.c > index 2db160e063..8ddafacf13 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1036,19 +1036,23 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp) > if (is_netdev) { > if (netdev->type == NET_CLIENT_DRIVER_NIC || > !net_client_init_fun[netdev->type]) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a netdev backend type"); > + error_setg(errp, "network backend '%s' is not compiled into this binary", > + NetClientDriver_str(netdev->type)); > return -1; > } > } else { > if (netdev->type == NET_CLIENT_DRIVER_NONE) { > return 0; /* nothing to do */ > } > - if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || > - !net_client_init_fun[netdev->type]) { > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > - "a net backend type (maybe it is not compiled " > - "into this binary)"); > + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT) { > + error_setg(errp, "network backend '%s' is only supported with -netdev/-nic", > + NetClientDriver_str(netdev->type)); > + return -1; > + } > + > + if (!net_client_init_fun[netdev->type]) { > + error_setg(errp, "network backend '%s' is not compiled into this binary", > + NetClientDriver_str(netdev->type)); > return -1; > } > > -- > 2.37.3 > With regards, Daniel
On Thu, Oct 27, 2022 at 6:52 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > ping: Jason, are you willing to queue this since it has two > positive reviews. > Yes, I've queued this. Thanks > On Mon, Oct 03, 2022 at 11:06:12AM +0100, Daniel P. Berrangé wrote: > > The current message when using '-net user...' with SLIRP disabled at > > compile time is: > > > > qemu-system-x86_64: -net user: Parameter 'type' expects a net backend type (maybe it is not compiled into this binary) > > > > An observation is that we're using the 'netdev->type' field here which > > is an enum value, produced after QAPI has converted from its string > > form. > > > > IOW, at this point in the code, we know that the user's specified > > type name was a valid network backend. The only possible scenario that > > can make the backend init function be NULL, is if support for that > > backend was disabled at build time. Given this, we don't need to caveat > > our error message with a 'maybe' hint, we can be totally explicit. > > > > The use of QERR_INVALID_PARAMETER_VALUE doesn't really lend itself to > > user friendly error message text. Since this is not used to set a > > specific QAPI error class, we can simply stop using this pre-formatted > > error text and provide something better. > > > > Thus the new message is: > > > > qemu-system-x86_64: -net user: network backend 'user' is not compiled into this binary > > > > The case of passing 'hubport' for -net is also given a message reminding > > people they should have used -netdev/-nic instead, as this backend type > > is only valid for the modern syntax. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > > > NB, this does not make any difference to people who were relying on the > > QEMU built-in default hub that was created if you don't list any -net / > > -netdev / -nic argument, only those using explicit args. > > > > net/net.c | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/net/net.c b/net/net.c > > index 2db160e063..8ddafacf13 100644 > > --- a/net/net.c > > +++ b/net/net.c > > @@ -1036,19 +1036,23 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp) > > if (is_netdev) { > > if (netdev->type == NET_CLIENT_DRIVER_NIC || > > !net_client_init_fun[netdev->type]) { > > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > > - "a netdev backend type"); > > + error_setg(errp, "network backend '%s' is not compiled into this binary", > > + NetClientDriver_str(netdev->type)); > > return -1; > > } > > } else { > > if (netdev->type == NET_CLIENT_DRIVER_NONE) { > > return 0; /* nothing to do */ > > } > > - if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || > > - !net_client_init_fun[netdev->type]) { > > - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", > > - "a net backend type (maybe it is not compiled " > > - "into this binary)"); > > + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT) { > > + error_setg(errp, "network backend '%s' is only supported with -netdev/-nic", > > + NetClientDriver_str(netdev->type)); > > + return -1; > > + } > > + > > + if (!net_client_init_fun[netdev->type]) { > > + error_setg(errp, "network backend '%s' is not compiled into this binary", > > + NetClientDriver_str(netdev->type)); > > return -1; > > } > > > > -- > > 2.37.3 > > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
diff --git a/net/net.c b/net/net.c index 2db160e063..8ddafacf13 100644 --- a/net/net.c +++ b/net/net.c @@ -1036,19 +1036,23 @@ static int net_client_init1(const Netdev *netdev, bool is_netdev, Error **errp) if (is_netdev) { if (netdev->type == NET_CLIENT_DRIVER_NIC || !net_client_init_fun[netdev->type]) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", - "a netdev backend type"); + error_setg(errp, "network backend '%s' is not compiled into this binary", + NetClientDriver_str(netdev->type)); return -1; } } else { if (netdev->type == NET_CLIENT_DRIVER_NONE) { return 0; /* nothing to do */ } - if (netdev->type == NET_CLIENT_DRIVER_HUBPORT || - !net_client_init_fun[netdev->type]) { - error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "type", - "a net backend type (maybe it is not compiled " - "into this binary)"); + if (netdev->type == NET_CLIENT_DRIVER_HUBPORT) { + error_setg(errp, "network backend '%s' is only supported with -netdev/-nic", + NetClientDriver_str(netdev->type)); + return -1; + } + + if (!net_client_init_fun[netdev->type]) { + error_setg(errp, "network backend '%s' is not compiled into this binary", + NetClientDriver_str(netdev->type)); return -1; }
The current message when using '-net user...' with SLIRP disabled at compile time is: qemu-system-x86_64: -net user: Parameter 'type' expects a net backend type (maybe it is not compiled into this binary) An observation is that we're using the 'netdev->type' field here which is an enum value, produced after QAPI has converted from its string form. IOW, at this point in the code, we know that the user's specified type name was a valid network backend. The only possible scenario that can make the backend init function be NULL, is if support for that backend was disabled at build time. Given this, we don't need to caveat our error message with a 'maybe' hint, we can be totally explicit. The use of QERR_INVALID_PARAMETER_VALUE doesn't really lend itself to user friendly error message text. Since this is not used to set a specific QAPI error class, we can simply stop using this pre-formatted error text and provide something better. Thus the new message is: qemu-system-x86_64: -net user: network backend 'user' is not compiled into this binary The case of passing 'hubport' for -net is also given a message reminding people they should have used -netdev/-nic instead, as this backend type is only valid for the modern syntax. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- NB, this does not make any difference to people who were relying on the QEMU built-in default hub that was created if you don't list any -net / -netdev / -nic argument, only those using explicit args. net/net.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)