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

[Michael Tokarev <mjt@tls.msk.ru>]

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.

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.  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.

Thanks,

/mjt