Re: [Qemu-devel] [PATCH 12/31] l2tpv3: Improve -netdev/netdev_add/-net/... error reporting

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
> On Mon, Oct 8, 2018 at 9:54 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> When -netdev l2tpv3 fails, it first reports a specific error, then a
>> generic one, like this:
>>
>>     $ qemu-system-x86_64 -netdev l2tpv3,id=foo,src=,dst=,txsession=1
>>     qemu-system-x86_64: -netdev l2tpv3,id=foo,src=,dst=,txsession=1: l2tpv3_open : could not resolve src, errno = Name or service not known
>>     qemu-system-x86_64: Device 'l2tpv3' could not be initialized
>>
>> With the command line, the messages go to stderr.  In HMP, they go to
>> the monitor.  In QMP, the second one becomes the error reply, and the
>> first one goes to stderr.
>>
>> Convert net_init_tap() to Error.  This suppresses the unwanted second
>> message, and makes the specific error the QMP error reply.
>>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  net/l2tpv3.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
>> index 6745b78990..0c5dd22ef7 100644
>> --- a/net/l2tpv3.c
>> +++ b/net/l2tpv3.c
>> @@ -28,6 +28,7 @@
>>  #include <netdb.h>
>>  #include "net/net.h"
>>  #include "clients.h"
>> +#include "qapi/error.h"
>>  #include "qemu-common.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/option.h"
>> @@ -528,7 +529,6 @@ int net_init_l2tpv3(const Netdev *netdev,
>>                      const char *name,
>>                      NetClientState *peer, Error **errp)
>>  {
>> -    /* FIXME error_setg(errp, ...) on failure */
>>      const NetdevL2TPv3Options *l2tpv3;
>>      NetL2TPV3State *s;
>>      NetClientState *nc;
>> @@ -555,7 +555,7 @@ int net_init_l2tpv3(const Netdev *netdev,
>>      }
>>
>>      if ((l2tpv3->has_offset) && (l2tpv3->offset > 256)) {
>> -        error_report("l2tpv3_open : offset must be less than 256 bytes");
>> +        error_setg(errp, "l2tpv3_open : offset must be less than 256 bytes");
>>          goto outerr;
>>      }
>>
>> @@ -563,6 +563,8 @@ int net_init_l2tpv3(const Netdev *netdev,
>>          if (l2tpv3->has_rxcookie && l2tpv3->has_txcookie) {
>>              s->cookie = true;
>>          } else {
>> +            error_setg(errp,
>> +                       "require both 'rxcookie' and 'txcookie' or neither");
>
> maybe for consistency it would be a good idea to remove the
> "l2tpv3_open : " prefix from the other messages while touching it?

Good idea, not least since the other net_init_FOO() don't use such
prefixes.

> looks good otherwise:
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!