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é <phi...@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.sist...@oracle.com>
>> ---
>> net/tap.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
