* [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-05 18:46 Dr. David Alan Gilbert (git)
2019-04-05 19:08 ` no-reply
2019-04-05 21:25 ` Samuel Thibault
0 siblings, 2 replies; 15+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2019-04-05 18:46 UTC (permalink / raw)
To: qemu-devel, samuel.thibault, jan.kiszka
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.
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-05 19:08 ` no-reply
0 siblings, 0 replies; 15+ messages in thread
From: no-reply @ 2019-04-05 19:08 UTC (permalink / raw)
To: dgilbert; +Cc: fam, qemu-devel, samuel.thibault, jan.kiszka
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-05 19:08 ` no-reply
0 siblings, 0 replies; 15+ messages in thread
From: no-reply @ 2019-04-05 19:08 UTC (permalink / raw)
To: dgilbert; +Cc: fam, samuel.thibault, qemu-devel, jan.kiszka
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-05 21:25 ` Samuel Thibault
0 siblings, 0 replies; 15+ messages in thread
From: Samuel Thibault @ 2019-04-05 21:25 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, jan.kiszka
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
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> -+-
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-05 21:25 ` Samuel Thibault
0 siblings, 0 replies; 15+ messages in thread
From: Samuel Thibault @ 2019-04-05 21:25 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: jan.kiszka, qemu-devel
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
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> -+-
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-08 8:46 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert @ 2019-04-08 8:46 UTC (permalink / raw)
To: Samuel Thibault; +Cc: qemu-devel, jan.kiszka
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-08 8:46 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert @ 2019-04-08 8:46 UTC (permalink / raw)
To: Samuel Thibault; +Cc: jan.kiszka, qemu-devel
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-11 18:45 ` Samuel Thibault
0 siblings, 0 replies; 15+ messages in thread
From: Samuel Thibault @ 2019-04-11 18:45 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, jan.kiszka
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-11 18:45 ` Samuel Thibault
0 siblings, 0 replies; 15+ messages in thread
From: Samuel Thibault @ 2019-04-11 18:45 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: jan.kiszka, qemu-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-12 15:49 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert @ 2019-04-12 15:49 UTC (permalink / raw)
To: Samuel Thibault; +Cc: qemu-devel, jan.kiszka
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-12 15:49 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert @ 2019-04-12 15:49 UTC (permalink / raw)
To: Samuel Thibault; +Cc: jan.kiszka, qemu-devel
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-12 21:16 ` Samuel Thibault
0 siblings, 0 replies; 15+ messages in thread
From: Samuel Thibault @ 2019-04-12 21:16 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: qemu-devel, jan.kiszka
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-12 21:16 ` Samuel Thibault
0 siblings, 0 replies; 15+ messages in thread
From: Samuel Thibault @ 2019-04-12 21:16 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: jan.kiszka, qemu-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-15 12:02 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert @ 2019-04-15 12:02 UTC (permalink / raw)
To: Samuel Thibault; +Cc: qemu-devel, jan.kiszka
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
@ 2019-04-15 12:02 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 15+ messages in thread
From: Dr. David Alan Gilbert @ 2019-04-15 12:02 UTC (permalink / raw)
To: Samuel Thibault; +Cc: jan.kiszka, qemu-devel
* 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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-04-15 12:04 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-05 18:46 [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix Dr. David Alan Gilbert (git)
2019-04-05 19:08 ` no-reply
2019-04-05 19:08 ` no-reply
2019-04-05 21:25 ` Samuel Thibault
2019-04-05 21:25 ` Samuel Thibault
2019-04-08 8:46 ` Dr. David Alan Gilbert
2019-04-08 8:46 ` Dr. David Alan Gilbert
2019-04-11 18:45 ` Samuel Thibault
2019-04-11 18:45 ` Samuel Thibault
2019-04-12 15:49 ` Dr. David Alan Gilbert
2019-04-12 15:49 ` Dr. David Alan Gilbert
2019-04-12 21:16 ` Samuel Thibault
2019-04-12 21:16 ` Samuel Thibault
2019-04-15 12:02 ` Dr. David Alan Gilbert
2019-04-15 12:02 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.