Re: [PATCH] tap: fix net_init_tap() return code

[Jason Wang]

On Tue, Apr 11, 2023 at 9:10 PM Steven Sistare
<steven.sistare@oracle.com> wrote:
>
> On 4/11/2023 2:32 AM, Jason Wang wrote:
> > 在 2023/4/5 23:38, Steven Sistare 写道:
> >> On 4/4/2023 6:00 PM, Philippe Mathieu-Daudé wrote:
> >>> On 4/4/23 18:00, Steve Sistare wrote:
> >>>> When net_init_tap() succeeds for a multi-queue device, it returns a
> >>>> non-zero ret=1 code to its caller, because of this code where ret becomes
> >>> Indeed g_unix_set_fd_nonblocking() returns TRUE on success.
> >>>
> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>
> >>>> 1 when g_unix_set_fd_nonblocking succeeds.  Luckily, the only current 
> >>>> call
> >>>> site checks for negative, rather than non-zero.
> >>>>
> >>>>       ret = g_unix_set_fd_nonblocking(fd, true, NULL);
> >>>>       if (!ret) {
> >>>>           ...
> >>>>           goto free_fail;
> >>>>
> >>>> Also, if g_unix_set_fd_nonblocking fails (though unlikely), ret=0 is 
> >>>> returned,
> >>>> and the caller will use a broken interface.
> >>> We should return -1 from free_fail, not trying to propagate 'ret':
> >> Thanks for the review.  I will add your "return -1" changes if Jason 
> >> agrees.
> >
> > Note that the "free_fail" label is kind of ambiguous. It is used even if we 
> > succeed if I was not wrong.
>
> Yes, good catch.  We must return 0 from free_fail on success.
> I could delete all uses of ret, test errp in free_fail, and return either -1 
> or 0.
> Or, you could accept my initial small patch.
>
> What do you prefer?

I've queued your patch for 8.1.

Thanks

>
> - Steve
>
> >>> -- >8 --
> >>> diff --git a/net/tap.c b/net/tap.c
> >>> index 1bf085d422..e59238bda0 100644
> >>> --- a/net/tap.c
> >>> +++ b/net/tap.c
> >>> @@ -821,3 +821,2 @@ int net_init_tap(const Netdev *netdev, const char 
> >>> *name,
> >>>       char ifname[128];
> >>> -    int ret = 0;
> >>>
> >>> @@ -896,3 +895,2 @@ int net_init_tap(const Netdev *netdev, const char 
> >>> *name,
> >>>                              "the number of vhostfds passed");
> >>> -                ret = -1;
> >>>                   goto free_fail;
> >>> @@ -904,3 +902,2 @@ int net_init_tap(const Netdev *netdev, const char 
> >>> *name,
> >>>               if (fd == -1) {
> >>> -                ret = -1;
> >>>                   goto free_fail;
> >>> @@ -908,4 +905,3 @@ int net_init_tap(const Netdev *netdev, const char 
> >>> *name,
> >>>
> >>> -            ret = g_unix_set_fd_nonblocking(fd, true, NULL);
> >>> -            if (!ret) {
> >>> +            if (!g_unix_set_fd_nonblocking(fd, true, NULL)) {
> >>>                   error_setg_errno(errp, errno, "%s: Can't use file 
> >>> descriptor %d",
> >>> @@ -918,3 +914,2 @@ int net_init_tap(const Netdev *netdev, const char 
> >>> *name,
> >>>                   if (vnet_hdr < 0) {
> >>> -                    ret = -1;
> >>>                       goto free_fail;
> >>> @@ -924,3 +919,2 @@ int net_init_tap(const Netdev *netdev, const char 
> >>> *name,
> >>>                              "vnet_hdr not consistent across given tap 
> >>> fds");
> >>> -                ret = -1;
> >>>                   goto free_fail;
> >>> @@ -934,3 +928,2 @@ int net_init_tap(const Netdev *netdev, const char 
> >>> *name,
> >>>                   error_propagate(errp, err);
> >>> -                ret = -1;
> >>>                   goto free_fail;
> >>> @@ -948,3 +941,3 @@ free_fail:
> >>>           g_free(vhost_fds);
> >>> -        return ret;
> >>> +        return -1;
> >>>       } else if (tap->helper) {
> >>> ---
> >>>
> >>>> Fixes: a8208626ba89.. ("net: replace qemu_set_nonblock()")
> >>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >>>> ---
> >>>>    net/tap.c | 4 ++--
> >>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >