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