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

[Markus Armbruster]

Michael Tokarev <address@hidden> writes:

> 15.11.2014 13:06, address@hidden wrote:
>> From: Gonglei <address@hidden>
>> 
>> 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... ;)

Yup.

In the case of close(), I wouldn't even bother to check, except Coverity
gets excited when it sees an invalid fd flow into close().  Rightfully
so when the invalid fd is non-negative, overeager when it's negative.