Re: [Qemu-devel] [Qemu-trivial] [PATCH 2/2] tap: fix possible fd leak

[Gonglei <arei.gonglei@huawei.com>]

On 2014/11/2 13:11, Michael Tokarev wrote:

> 31.10.2014 09:11, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> In hotplugging scenario, taking those true branch, the file
>> handler do not be closed. Adding cleanup logic for them.
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  net/tap.c | 12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 7bcd4c7..3cfbee8 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -796,7 +796,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>          if (net_init_tap_one(tap, peer, "bridge", name, ifname,
>>                               script, downscript, vhostfdname,
>>                               vnet_hdr, fd)) {
>> -            return -1;
>> +            goto fail;
>>          }
>>      } else {
>>          if (tap->has_vhostfds) {
>> @@ -823,7 +823,7 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>              if (queues > 1 && i == 0 && !tap->has_ifname) {
>>                  if (tap_fd_get_ifname(fd, ifname)) {
>>                      error_report("Fail to get ifname");
>> -                    return -1;
>> +                    goto fail;
>>                  }
>>              }
>>  
>> @@ -831,12 +831,18 @@ int net_init_tap(const NetClientOptions *opts, const char *name,
>>                                   i >= 1 ? "no" : script,
>>                                   i >= 1 ? "no" : downscript,
>>                                   vhostfdname, vnet_hdr, fd)) {
>> -                return -1;
>> +                goto fail;
>>              }
>>          }
>>      }
>>  
>>      return 0;
>> +
>> +fail:
>> +    if (fd != -1) {
>> +        close(fd);
>> +    }
>> +    return -1;
>>  }
> 
> I think, given the somewhat "hairy" nature of net_init_tap() function, which
> has many error returns which should not close fd and just 3 which should, it
> is better to add explicit close(fd) in these 3 places.
> 

Agree. v2 will do. Thanks!

> Besides, why do you check for fd != -1 in the fail path?  You added the goto
> into the 3 places, all of them has fd != -1, and there's no other ways to
> reach this place.  

Yes.

> Are you not certain that fd will be valid here?  If yes,
> I think this is yet another argument for adding close()s into the 3 places.
> 


Best regards,
-Gonglei