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

[Jason Wang <jasowang@redhat.com>]

在 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(-)