[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: Markus Armbruster
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak
Date: Tue, 18 Nov 2014 08:50:42 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)

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


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.

reply via email to

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