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

[Marc-André Lureau]

Hi
On Mon, Oct 8, 2018 at 9:54 PM Markus Armbruster <address@hidden> 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 <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  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?

looks good otherwise:
Reviewed-by: Marc-André Lureau <address@hidden>

>              goto outerr;
>          }
>      } else {
> @@ -578,7 +580,8 @@ int net_init_l2tpv3(const Netdev *netdev,
>      if (l2tpv3->has_udp && l2tpv3->udp) {
>          s->udp = true;
>          if (!(l2tpv3->has_srcport && l2tpv3->has_dstport)) {
> -            error_report("l2tpv3_open : need both src and dst port for udp");
> +            error_setg(errp,
> +                       "l2tpv3_open : need both src and dst port for udp");
>              goto outerr;
>          } else {
>              srcport = l2tpv3->srcport;
> @@ -639,20 +642,19 @@ int net_init_l2tpv3(const Netdev *netdev,
>      gairet = getaddrinfo(l2tpv3->src, srcport, &hints, &result);
>
>      if ((gairet != 0) || (result == NULL)) {
> -        error_report(
> -            "l2tpv3_open : could not resolve src, errno = %s",
> -            gai_strerror(gairet)
> -        );
> +        error_setg(errp, "l2tpv3_open : could not resolve src, errno = %s",
> +                   gai_strerror(gairet));
>          goto outerr;
>      }
>      fd = socket(result->ai_family, result->ai_socktype, result->ai_protocol);
>      if (fd == -1) {
>          fd = -errno;
> -        error_report("l2tpv3_open : socket creation failed, errno = %d", 
> -fd);
> +        error_setg(errp, "l2tpv3_open : socket creation failed, errno = %d",
> +                   -fd);
>          goto outerr;
>      }
>      if (bind(fd, (struct sockaddr *) result->ai_addr, result->ai_addrlen)) {
> -        error_report("l2tpv3_open :  could not bind socket err=%i", errno);
> +        error_setg(errp, "l2tpv3_open :  could not bind socket err=%i", 
> errno);
>          goto outerr;
>      }
>      if (result) {
> @@ -677,10 +679,8 @@ int net_init_l2tpv3(const Netdev *netdev,
>      result = NULL;
>      gairet = getaddrinfo(l2tpv3->dst, dstport, &hints, &result);
>      if ((gairet != 0) || (result == NULL)) {
> -        error_report(
> -            "l2tpv3_open : could not resolve dst, error = %s",
> -            gai_strerror(gairet)
> -        );
> +        error_setg(errp, "l2tpv3_open : could not resolve dst, error = %s",
> +                   gai_strerror(gairet));
>          goto outerr;
>      }
>
> --
> 2.17.1
>
>


-- 
Marc-André Lureau