[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Gonglei
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak
Date: Tue, 18 Nov 2014 16:07:53 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20120327 Thunderbird/11.0.1

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,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]