From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56731) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xqdrf-0006gy-Dc for qemu-devel@nongnu.org; Tue, 18 Nov 2014 03:10:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xqdrb-0006vU-1J for qemu-devel@nongnu.org; Tue, 18 Nov 2014 03:10:19 -0500 Message-ID: <546AFE59.6040208@huawei.com> Date: Tue, 18 Nov 2014 16:07:53 +0800 From: Gonglei MIME-Version: 1.0 References: <1416046008-7880-1-git-send-email-arei.gonglei@huawei.com> <1416046008-7880-2-git-send-email-arei.gonglei@huawei.com> <546A2931.20209@msgid.tls.msk.ru> <87y4r9vtf1.fsf@blackfin.pond.sub.org> In-Reply-To: <87y4r9vtf1.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="GB2312" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Huangweidong (C)" , Zhanghailiang , "qemu-trivial@nongnu.org" , Michael Tokarev , "qemu-devel@nongnu.org" , "Huangpeng (Peter)" , "pbonzini@redhat.com" On 2014/11/18 15:50, Markus Armbruster wrote: > Michael Tokarev writes: > >> 15.11.2014 13:06, arei.gonglei@huawei.com wrote: >>> From: Gonglei >>> >>> 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