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

[Gonglei]

On 2014/11/18 15:50, Markus Armbruster wrote:

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

Thank you, guys.
Paolo had fixed it :)

Best regards,
-Gonglei