Re: [Qemu-trivial] [PATCH 1/9] l2tpv3: fix fd leak

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

15.11.2014 13:06, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> In this false branch, fd will leak when it is zero.
> Change the testing condition.

Why fd==0 is a concern here?  It is a very unlikely
situation that fd0 will be picked - firstly because
fd0 is almost always open, and second - even if it
isn't open, it will be picked much earlier than this
code path, ie, some other file will use fd0 before.

But even if the concern is real (after all, better
stay correct than spread bad code pattern, even if
in reality we don't care as this can't happen), why
not add 0 to the equality?

Why people especially compare with -1?  Any negative
value is illegal here and in lots of other places,
and many software packages used to return -errno in
error cases, which is definitely != -1.  I'm not
saying that comparing with -1 is bad in _this_
particular case, but why not do it generally in
all cases?

More, comparing with 0 is faster and shorter than
comparing with -1...

So if it were me, I'd change it to >= 0, not to
== -1.  Here and in all other millions of places
in qemu code where it compares with -1... ;)

Thanks,

/mjt
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  net/l2tpv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
> index 528d95b..b2b0fc0 100644
> --- a/net/l2tpv3.c
> +++ b/net/l2tpv3.c
> @@ -746,7 +746,7 @@ int net_init_l2tpv3(const NetClientOptions *opts,
>      return 0;
>  outerr:
>      qemu_del_net_client(nc);
> -    if (fd > 0) {
> +    if (fd != -1) {
>          close(fd);
>      }
>      if (result) {
>