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

[Vladimir Sementsov-Ogievskiy]

21.12.2020 23:12, Philippe Mathieu-Daudé wrote:
> 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>

0/-1 return status is highly used in net/tap, so I decided to follow it.

> > 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;
> >       }
> >   


--
Best regards,
Vladimir