| Message ID | 1416046008-7880-2-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Nov 15, 2014 at 06:06:40PM +0800, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > In this false branch, fd will leak when it is zero. > Change the testing condition. > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > net/l2tpv3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
15.11.2014 13:06, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@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... ;) Thanks, /mjt > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > net/l2tpv3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/l2tpv3.c b/net/l2tpv3.c > index 528d95b..b2b0fc0 100644 > --- a/net/l2tpv3.c > +++ b/net/l2tpv3.c > @@ -746,7 +746,7 @@ int net_init_l2tpv3(const NetClientOptions *opts, > return 0; > outerr: > qemu_del_net_client(nc); > - if (fd > 0) { > + if (fd != -1) { > close(fd); > } > if (result) { >
Michael Tokarev <mjt@tls.msk.ru> writes: > 15.11.2014 13:06, arei.gonglei@huawei.com wrote: >> From: Gonglei <arei.gonglei@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.
On 2014/11/18 15:50, Markus Armbruster wrote: > Michael Tokarev <mjt@tls.msk.ru> writes: > >> 15.11.2014 13:06, arei.gonglei@huawei.com wrote: >>> From: Gonglei <arei.gonglei@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
diff --git a/net/l2tpv3.c b/net/l2tpv3.c index 528d95b..b2b0fc0 100644 --- a/net/l2tpv3.c +++ b/net/l2tpv3.c @@ -746,7 +746,7 @@ int net_init_l2tpv3(const NetClientOptions *opts, return 0; outerr: qemu_del_net_client(nc); - if (fd > 0) { + if (fd != -1) { close(fd); } if (result) {