| Message ID | 1680624004-154390-1-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tap: fix net_init_tap() return code | expand |
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': -- >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(-)
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. - 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(-)
在 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. > > - Steve Note that the "free_fail" label is kind of ambiguous. It is used even if we succeed if I was not wrong. Thanks >> -- >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(-)
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? - 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(-) >
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(-) > >
diff --git a/net/tap.c b/net/tap.c index 1bf085d..1f3e927 100644 --- a/net/tap.c +++ b/net/tap.c @@ -906,8 +906,8 @@ int net_init_tap(const Netdev *netdev, const char *name, goto free_fail; } - ret = g_unix_set_fd_nonblocking(fd, true, NULL); - if (!ret) { + if (!g_unix_set_fd_nonblocking(fd, true, NULL)) { + ret = -1; error_setg_errno(errp, errno, "%s: Can't use file descriptor %d", name, fd); goto free_fail;
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 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. 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(-)