On 2014/11/18 15:50, Markus Armbruster wrote: > Michael Tokarev <m...@tls.msk.ru> writes: > >> 15.11.2014 13:06, arei.gong...@huawei.com wrote: >>> From: Gonglei <arei.gong...@huawei.com> >>> >>> 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
