On 2014/11/2 13:11, Michael Tokarev wrote:
> 31.10.2014 09:11, arei.gong...@huawei.com wrote:
>> From: Gonglei <arei.gong...@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.gong...@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
