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