| Message ID | 20190405184648.17029-1-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | slirp: Gcc 9 -O3 fix | expand |
Patchew URL: https://patchew.org/QEMU/20190405184648.17029-1-dgilbert@redhat.com/ Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 20190405184648.17029-1-dgilbert@redhat.com Subject: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/20190405184648.17029-1-dgilbert@redhat.com -> patchew/20190405184648.17029-1-dgilbert@redhat.com Switched to a new branch 'test' c02eccdeb3 slirp: Gcc 9 -O3 fix === OUTPUT BEGIN === ERROR: code indent should never use tabs #22: FILE: slirp/src/socket.c:116: +^Iiov[0].iov_base = sb->sb_wptr;$ total: 1 errors, 0 warnings, 17 lines checked Commit c02eccdeb3e6 (slirp: Gcc 9 -O3 fix) has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20190405184648.17029-1-dgilbert@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Hello, Dr. David Alan Gilbert (git), le ven. 05 avril 2019 19:46:48 +0100, a ecrit: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Gcc 9 needs some convincing that sopreprbuf really is going to fill > in iov in the call from soreadbuf, even though the failure case > shouldn't happen; so swing the check around initialising the fields. While I can understand that setting iov[0].iov_len may help a compiler, I don't see why moving if (len <= 0) return 0; down? > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > slirp/src/socket.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/slirp/src/socket.c b/slirp/src/socket.c > index 4a3c935e25..4a2222a95f 100644 > --- a/slirp/src/socket.c > +++ b/slirp/src/socket.c > @@ -113,12 +113,14 @@ size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np) > DEBUG_CALL("sopreprbuf"); > DEBUG_ARG("so = %p", so); > > - if (len <= 0) > - return 0; > - > iov[0].iov_base = sb->sb_wptr; > + iov[0].iov_len = 0; > iov[1].iov_base = NULL; > iov[1].iov_len = 0; > + > + if (len <= 0) > + return 0; > + > if (sb->sb_wptr < sb->sb_rptr) { > iov[0].iov_len = sb->sb_rptr - sb->sb_wptr; > /* Should never succeed, but... */ > -- > 2.21.0 >
* Samuel Thibault (samuel.thibault@gnu.org) wrote: > Hello, > > Dr. David Alan Gilbert (git), le ven. 05 avril 2019 19:46:48 +0100, a ecrit: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Gcc 9 needs some convincing that sopreprbuf really is going to fill > > in iov in the call from soreadbuf, even though the failure case > > shouldn't happen; so swing the check around initialising the fields. > > While I can understand that setting iov[0].iov_len may help a compiler, > I don't see why moving if (len <= 0) return 0; down? The original errors are: /home/dgilbert/git/qemu/slirp/src/socket.c: In function ‘soread’: /home/dgilbert/git/qemu/slirp/src/socket.c:188:7: error: ‘iov.iov_base’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 188 | nn = recv(so->s, iov[0].iov_base, iov[0].iov_len,0); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/dgilbert/git/qemu/slirp/src/socket.c:188:7: error: ‘iov.iov_len’ may be used uninitialized in this function [-Werror=maybe-uninitialized] /home/dgilbert/git/qemu/slirp/src/socket.c:232:5: error: ‘n’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 232 | if (n == 2 && nn == iov[0].iov_len) { | ^ /home/dgilbert/git/qemu/slirp/src/socket.c:234:19: error: ‘*((void *)&iov+16).iov_len’ may be used uninitialized in this function [-Werror=maybe-uninitialized] (and a few more along the same idea). So the problem is actually in soread not sopreprbuf itself. 'soread' has the comment: /* * No need to check if there's enough room to read. * soread wouldn't have been called if there weren't */ sopreprbuf(so, iov, &n); the compiler doesn't realise that, and is moaning about the case where the if (len <=0) return happens and the following code tries to use iov. Dave > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > slirp/src/socket.c | 8 +++++--- > > 1 file changed, 5 insertions(+), 3 deletions(-) > > > > diff --git a/slirp/src/socket.c b/slirp/src/socket.c > > index 4a3c935e25..4a2222a95f 100644 > > --- a/slirp/src/socket.c > > +++ b/slirp/src/socket.c > > @@ -113,12 +113,14 @@ size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np) > > DEBUG_CALL("sopreprbuf"); > > DEBUG_ARG("so = %p", so); > > > > - if (len <= 0) > > - return 0; > > - > > iov[0].iov_base = sb->sb_wptr; > > + iov[0].iov_len = 0; > > iov[1].iov_base = NULL; > > iov[1].iov_len = 0; > > + > > + if (len <= 0) > > + return 0; > > + > > if (sb->sb_wptr < sb->sb_rptr) { > > iov[0].iov_len = sb->sb_rptr - sb->sb_wptr; > > /* Should never succeed, but... */ > > -- > > 2.21.0 > > > > -- > Samuel > FYLG> Tiens, vlà une URL qui va bien : > FYLG> ftp://127.0.0.1/WaReZ/NiouZeS/WinDoZe/NeWSMoNGeR/SuPeR > c'est gentil sauf que l'adresse ne fonctionne pas sa me fais une erreur > -+- Furtif in Guide du Neuneu Usenet : <MODE CERVEAU OFF> -+- -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Hello, Dr. David Alan Gilbert, le lun. 08 avril 2019 09:46:53 +0100, a ecrit: > 'soread' has the comment: > > /* > * No need to check if there's enough room to read. > * soread wouldn't have been called if there weren't > */ > sopreprbuf(so, iov, &n); > > the compiler doesn't realise that, and is moaning about the case > where the if (len <=0) return happens and the following > code tries to use iov. I see. Perhaps we should make this an assert then? In case this isn't true, i.e. soread() is called even if no room is available, returning 0 would probably just let the caller just try again, and we should rather just plainly crash than hang? Samuel
* Samuel Thibault (samuel.thibault@gnu.org) wrote: > Hello, > > Dr. David Alan Gilbert, le lun. 08 avril 2019 09:46:53 +0100, a ecrit: > > 'soread' has the comment: > > > > /* > > * No need to check if there's enough room to read. > > * soread wouldn't have been called if there weren't > > */ > > sopreprbuf(so, iov, &n); > > > > the compiler doesn't realise that, and is moaning about the case > > where the if (len <=0) return happens and the following > > code tries to use iov. > > I see. Perhaps we should make this an assert then? In case this isn't > true, i.e. soread() is called even if no room is available, returning 0 > would probably just let the caller just try again, and we should rather > just plainly crash than hang? Adding the assert in soread sorts that case out: assert(sopreprbuf(so, iov, &n) != 0); however, I also need to fix soreadbuf; is it legal to call that with a 0 size? Dave > Samuel -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Dr. David Alan Gilbert, le ven. 12 avril 2019 16:49:42 +0100, a ecrit: > * Samuel Thibault (samuel.thibault@gnu.org) wrote: > > Hello, > > > > Dr. David Alan Gilbert, le lun. 08 avril 2019 09:46:53 +0100, a ecrit: > > > 'soread' has the comment: > > > > > > /* > > > * No need to check if there's enough room to read. > > > * soread wouldn't have been called if there weren't > > > */ > > > sopreprbuf(so, iov, &n); > > > > > > the compiler doesn't realise that, and is moaning about the case > > > where the if (len <=0) return happens and the following > > > code tries to use iov. > > > > I see. Perhaps we should make this an assert then? In case this isn't > > true, i.e. soread() is called even if no room is available, returning 0 > > would probably just let the caller just try again, and we should rather > > just plainly crash than hang? > > Adding the assert in soread sorts that case out: > assert(sopreprbuf(so, iov, &n) != 0); > > however, I also need to fix soreadbuf; is it legal to call that with > a 0 size? It does not really make sense to, so an assert >0 should be fine. Samuel
* Samuel Thibault (samuel.thibault@gnu.org) wrote: > Dr. David Alan Gilbert, le ven. 12 avril 2019 16:49:42 +0100, a ecrit: > > * Samuel Thibault (samuel.thibault@gnu.org) wrote: > > > Hello, > > > > > > Dr. David Alan Gilbert, le lun. 08 avril 2019 09:46:53 +0100, a ecrit: > > > > 'soread' has the comment: > > > > > > > > /* > > > > * No need to check if there's enough room to read. > > > > * soread wouldn't have been called if there weren't > > > > */ > > > > sopreprbuf(so, iov, &n); > > > > > > > > the compiler doesn't realise that, and is moaning about the case > > > > where the if (len <=0) return happens and the following > > > > code tries to use iov. > > > > > > I see. Perhaps we should make this an assert then? In case this isn't > > > true, i.e. soread() is called even if no room is available, returning 0 > > > would probably just let the caller just try again, and we should rather > > > just plainly crash than hang? > > > > Adding the assert in soread sorts that case out: > > assert(sopreprbuf(so, iov, &n) != 0); > > > > however, I also need to fix soreadbuf; is it legal to call that with > > a 0 size? > > It does not really make sense to, so an assert >0 should be fine. OK, replacement patch with just a couple of asserts sent. Dave > Samuel -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/slirp/src/socket.c b/slirp/src/socket.c index 4a3c935e25..4a2222a95f 100644 --- a/slirp/src/socket.c +++ b/slirp/src/socket.c @@ -113,12 +113,14 @@ size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np) DEBUG_CALL("sopreprbuf"); DEBUG_ARG("so = %p", so); - if (len <= 0) - return 0; - iov[0].iov_base = sb->sb_wptr; + iov[0].iov_len = 0; iov[1].iov_base = NULL; iov[1].iov_len = 0; + + if (len <= 0) + return 0; + if (sb->sb_wptr < sb->sb_rptr) { iov[0].iov_len = sb->sb_rptr - sb->sb_wptr; /* Should never succeed, but... */