Re: [PATCH 3/5] net/tap: tap_set_sndbuf(): return status

[Philippe Mathieu-Daudé]

On 12/21/20 8:06 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's recommended to return a value indicating success / failure for
> functions with errp parameter (see include/qapi/error.h). Let's update
> tap_set_sndbuf().

For simple "success/failure" a bool type is enough.

Preferably using bool type:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  net/tap_int.h   | 2 +-
>  net/tap-linux.c | 5 ++++-
>  net/tap-stub.c  | 2 +-
>  net/tap.c       | 6 +++---
>  4 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/net/tap_int.h b/net/tap_int.h
> index 225a49ea48..57567b9f24 100644
> --- a/net/tap_int.h
> +++ b/net/tap_int.h
> @@ -33,7 +33,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>  
>  ssize_t tap_read_packet(int tapfd, uint8_t *buf, int maxlen);
>  
> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp);
>  int tap_probe_vnet_hdr(int fd, Error **errp);
>  int tap_probe_vnet_hdr_len(int fd, int len);
>  int tap_probe_has_ufo(int fd);
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index b0635e9e32..c51bcdc2a3 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -130,7 +130,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>   */
>  #define TAP_DEFAULT_SNDBUF 0
>  
> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>  {
>      int sndbuf;
>  
> @@ -144,7 +144,10 @@ void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, 
> Error **errp)
>  
>      if (ioctl(fd, TUNSETSNDBUF, &sndbuf) == -1 && tap->has_sndbuf) {
>          error_setg_errno(errp, errno, "TUNSETSNDBUF ioctl failed");
> +        return -1;
>      }
> +
> +    return 0;
>  }
>  
>  int tap_probe_vnet_hdr(int fd, Error **errp)
> diff --git a/net/tap-stub.c b/net/tap-stub.c
> index 6fa130758b..473d5e4afe 100644
> --- a/net/tap-stub.c
> +++ b/net/tap-stub.c
> @@ -26,7 +26,7 @@
>  #include "qapi/error.h"
>  #include "tap_int.h"
>  
> -void tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
> +int tap_set_sndbuf(int fd, const NetdevTapOptions *tap, Error **errp)
>  {
>  }
>  
> diff --git a/net/tap.c b/net/tap.c
> index 75b01d54ee..33d749c7b6 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -661,10 +661,10 @@ static void net_init_tap_one(const NetdevTapOptions 
> *tap, NetClientState *peer,
>      Error *err = NULL;
>      TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
>      int vhostfd;
> +    int ret;
>  
> -    tap_set_sndbuf(s->fd, tap, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> +    ret = tap_set_sndbuf(s->fd, tap, errp);
> +    if (ret < 0) {
>          return;
>      }
>  
>