[Qemu-devel] [PATCH 0/2] tap: add cleanup logic avoiding leaking fd

[Qemu-devel] [PATCH 0/2] tap: add cleanup logic avoiding leaking fd

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Gonglei (2):
  tap: remove close(fd)
  tap: fix possible fd leak

 net/tap.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/2] tap: remove close(fd)

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

commit 5193e5fb (tap: factor out common tap initialization)
introduce net_init_tap_one(). But it's inapposite that close
fd in net_init_tap_one(), we should lay it in the caller,
becuase some caller needn't to close it if we get the fd
by monitor_handle_fd_param().

On the other hand, in other exceptional branch fd doesn't
be closed, so that's incomplete.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 net/tap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/tap.c b/net/tap.c
index a40f7f0..7bcd4c7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -598,7 +598,6 @@ static int net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
 
     s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
     if (!s) {
-        close(fd);
         return -1;
     }
 
-- 
1.7.12.4

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

[arei.gonglei]

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;
 }
 
 VHostNetState *tap_get_vhost_net(NetClientState *nc)
-- 
1.7.12.4

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/2] tap: remove close(fd)

[Michael Tokarev]

31.10.2014 09:11, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> commit 5193e5fb (tap: factor out common tap initialization)
> introduce net_init_tap_one(). But it's inapposite that close
> fd in net_init_tap_one(), we should lay it in the caller,
> becuase some caller needn't to close it if we get the fd
> by monitor_handle_fd_param().
> 
> On the other hand, in other exceptional branch fd doesn't
> be closed, so that's incomplete.

Applied to -trivial, with grammar tweaking in commit message
and with slightly better (in my opinion) subject, like this:

    tap: do not close(fd) in net_init_tap_one()

    commit 5193e5fb (tap: factor out common tap initialization)
    introduce net_init_tap_one(). But it's inappropriate that
    we close fd in net_init_tap_one(), we should lay it in the
    caller, becuase some callers needn't to close it if we get
    the fd by monitor_handle_fd_param().

    On the other hand, in other exceptional branches fd isn't
    closed, so that's incomplete anyway.

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH 1/2] tap: remove close(fd)

[Gonglei]

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

> 31.10.2014 09:11, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> commit 5193e5fb (tap: factor out common tap initialization)
>> introduce net_init_tap_one(). But it's inapposite that close
>> fd in net_init_tap_one(), we should lay it in the caller,
>> becuase some caller needn't to close it if we get the fd
>> by monitor_handle_fd_param().
>>
>> On the other hand, in other exceptional branch fd doesn't
>> be closed, so that's incomplete.
> 
> Applied to -trivial, with grammar tweaking in commit message
> and with slightly better (in my opinion) subject, like this:
> 

Ack. Thanks for your work. :)

Best regards,
-Gonglei

>     tap: do not close(fd) in net_init_tap_one()
> 
>     commit 5193e5fb (tap: factor out common tap initialization)
>     introduce net_init_tap_one(). But it's inappropriate that
>     we close fd in net_init_tap_one(), we should lay it in the
>     caller, becuase some callers needn't to close it if we get
>     the fd by monitor_handle_fd_param().
> 
>     On the other hand, in other exceptional branches fd isn't
>     closed, so that's incomplete anyway.
> 
> Thanks,
> 
> /mjt

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

[Michael Tokarev]

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

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

[Gonglei]

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

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

[Michael Tokarev]

02.11.2014 08:21, Gonglei wrote:
[]
> Agree. v2 will do. Thanks!

Also, I think it's good idea to add " in net_init_tap" to subject.
(And please don't resend both patches, as I already applied first :)

Thanks,

/mjt