Re: [Qemu-devel] [PATCH 2/2] net: netmap: use error_setg_errno() in place of error_report()

[Markus Armbruster]

Vincenzo Maffione <address@hidden> writes:

> This update was required to align netmap backend to the other
> net backends in terms of error reporting.

Thank you very much for helping to complete this job!

Recommend to point to commit a30ecde in your commit message.

> Signed-off-by: Vincenzo Maffione <address@hidden>
> ---
>  net/netmap.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/net/netmap.c b/net/netmap.c
> index 4197a9c..45290d8 100644
> --- a/net/netmap.c
> +++ b/net/netmap.c
> @@ -90,7 +90,7 @@ pkt_copy(const void *_src, void *_dst, int l)
>   * Open a netmap device. We assume there is only one queue
>   * (which is the case for the VALE bridge).
>   */
> -static int netmap_open(NetmapPriv *me)
> +static int netmap_open(NetmapPriv *me, Error **errp)
>  {
>      int fd;
>      int err;
> @@ -99,8 +99,8 @@ static int netmap_open(NetmapPriv *me)
>  
>      me->fd = fd = open(me->fdname, O_RDWR);
>      if (fd < 0) {
> -        error_report("Unable to open netmap device '%s' (%s)",
> -                        me->fdname, strerror(errno));
> +        error_setg_errno(errp, errno, "Unable to open netmap device '%s'",
> +                         me->fdname);
>          return -1;
>      }
>      memset(&req, 0, sizeof(req));
> @@ -109,15 +109,14 @@ static int netmap_open(NetmapPriv *me)
>      req.nr_version = NETMAP_API;
>      err = ioctl(fd, NIOCREGIF, &req);
>      if (err) {
> -        error_report("Unable to register %s: %s", me->ifname, 
> strerror(errno));
> +        error_setg_errno(errp, errno, "Unable to register %s", me->ifname);
>          goto error;
>      }
>      l = me->memsize = req.nr_memsize;
>  
>      me->mem = mmap(0, l, PROT_WRITE | PROT_READ, MAP_SHARED, fd, 0);
>      if (me->mem == MAP_FAILED) {
> -        error_report("Unable to mmap netmap shared memory: %s",
> -                        strerror(errno));
> +        error_setg_errno(errp, errno, "Unable to mmap netmap shared memory");
>          me->mem = NULL;
>          goto error;
>      }
> @@ -438,7 +437,6 @@ static NetClientInfo net_netmap_info = {
>  int net_init_netmap(const NetClientOptions *opts,
>                      const char *name, NetClientState *peer, Error **errp)
>  {
> -    /* FIXME error_setg(errp, ...) on failure */
>      const NetdevNetmapOptions *netmap_opts = opts->u.netmap;
>      NetClientState *nc;
>      NetmapPriv me;
> @@ -448,7 +446,7 @@ int net_init_netmap(const NetClientOptions *opts,
>          netmap_opts->has_devname ? netmap_opts->devname : "/dev/netmap");
>      /* Set default name for the port if not supplied. */
>      pstrcpy(me.ifname, sizeof(me.ifname), netmap_opts->ifname);
> -    if (netmap_open(&me)) {
> +    if (netmap_open(&me, errp)) {
>          return -1;
>      }
>      /* Create the object. */

netmap_open() returns 0 on success, -1 on failure.  For better or worse,
we avoid such returns for functions that reports errors via an Error **
parameter.  For an example, see commit 445f116.

Since errp can be null, you can't test *err, but have to use a local
variable, like this:

    netmap_open(&me, &err);
    if (err) {
        error_propagate(errp, err);
        return;
    }

See the big comment in include/qapi/error.h.

I actually like the way you did it, but consistency is more important
than what I like here.