| Message ID | 1268053115.2130.4.camel@localhost.localdomain |
|---|---|
| State | New |
| Headers | show |
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > usb-linux.c | 3 --- > 1 files changed, 0 insertions(+), 3 deletions(-) > > diff --git a/usb-linux.c b/usb-linux.c > index a9c15c6..23155dd 100644 > --- a/usb-linux.c > +++ b/usb-linux.c > @@ -846,9 +846,6 @@ static int usb_linux_update_endp_table(USBHostDevice > *s) case 0x03: > type = USBDEVFS_URB_TYPE_INTERRUPT; > break; > - default: > - DPRINTF("usb_host: malformed endpoint type\n"); > - type = USBDEVFS_URB_TYPE_BULK; > } > s->endp_table[(devep & 0xf) - 1].type = type; > s->endp_table[(devep & 0xf) - 1].halted = 0; > I'd be tempted to replace it by an abort(). If it's provable redundant then the compiler will remove it anyway. If not then we at least get a noisy failure when someone breaks it. Paul
On 03/08/2010 06:58 AM, Paul Bolle wrote: > Signed-off-by: Paul Bolle<pebolle@tiscali.nl> > Applied. Thanks. Regards, Anthony Liguori > --- > usb-linux.c | 3 --- > 1 files changed, 0 insertions(+), 3 deletions(-) > > diff --git a/usb-linux.c b/usb-linux.c > index a9c15c6..23155dd 100644 > --- a/usb-linux.c > +++ b/usb-linux.c > @@ -846,9 +846,6 @@ static int usb_linux_update_endp_table(USBHostDevice *s) > case 0x03: > type = USBDEVFS_URB_TYPE_INTERRUPT; > break; > - default: > - DPRINTF("usb_host: malformed endpoint type\n"); > - type = USBDEVFS_URB_TYPE_BULK; > } > s->endp_table[(devep& 0xf) - 1].type = type; > s->endp_table[(devep& 0xf) - 1].halted = 0; >
On Wed, 2010-03-17 at 10:59 -0500, Anthony Liguori wrote: > On 03/08/2010 06:58 AM, Paul Bolle wrote: > > Signed-off-by: Paul Bolle<pebolle@tiscali.nl> > > > Applied. Thanks. Paul Brook was "tempted to replace it by an abort()" (about one and a half week ago). Did you perhaps miss that message or weren't you tempted to do this? Regards, Paul Bolle
On 03/17/2010 11:14 AM, Paul Bolle wrote: > On Wed, 2010-03-17 at 10:59 -0500, Anthony Liguori wrote: > >> On 03/08/2010 06:58 AM, Paul Bolle wrote: >> >>> Signed-off-by: Paul Bolle<pebolle@tiscali.nl> >>> >>> >> Applied. Thanks. >> > Paul Brook was "tempted to replace it by an abort()" (about one and a > half week ago). Did you perhaps miss that message or weren't you tempted > to do this? > I missed it, but then again, I don't think the patch was wrong in the first place. I think we use too many aborts/exits in the device model that can potentially be triggered by guest code. Regards, Anthony Liguori > Regards, > > > Paul Bolle > >
> On 03/17/2010 11:14 AM, Paul Bolle wrote: > > On Wed, 2010-03-17 at 10:59 -0500, Anthony Liguori wrote: > >> On 03/08/2010 06:58 AM, Paul Bolle wrote: > >>> Signed-off-by: Paul Bolle<pebolle@tiscali.nl> > >> > >> Applied. Thanks. > > > > Paul Brook was "tempted to replace it by an abort()" (about one and a > > half week ago). Did you perhaps miss that message or weren't you tempted > > to do this? > > I missed it, but then again, I don't think the patch was wrong in the > first place. > > I think we use too many aborts/exits in the device model that can > potentially be triggered by guest code. If something should never happen (as in this case) then an abort/assert is completely appropriate. Once things get that screwed up there's no right answer, and the best thing we can do is terminate immediately to try and avoid further damage. If an assert/abort can be triggered by a guest then you obviously have a bug. Removing the assert is not the correct solution. You should either fix whatever caused the invalid state to occur, or replace it with an appropriate retry, fallback or guest visible failure. Paul
On 03/17/2010 12:08 PM, Paul Brook wrote: >> On 03/17/2010 11:14 AM, Paul Bolle wrote: >> >>> On Wed, 2010-03-17 at 10:59 -0500, Anthony Liguori wrote: >>> >>>> On 03/08/2010 06:58 AM, Paul Bolle wrote: >>>> >>>>> Signed-off-by: Paul Bolle<pebolle@tiscali.nl> >>>>> >>>> Applied. Thanks. >>>> >>> Paul Brook was "tempted to replace it by an abort()" (about one and a >>> half week ago). Did you perhaps miss that message or weren't you tempted >>> to do this? >>> >> I missed it, but then again, I don't think the patch was wrong in the >> first place. >> >> I think we use too many aborts/exits in the device model that can >> potentially be triggered by guest code. >> > If something should never happen (as in this case) then an abort/assert is > completely appropriate. Once things get that screwed up there's no right > answer, and the best thing we can do is terminate immediately to try and avoid > further damage. > This case was: switch (foo & 0x03) { case 0: case 1: case 2: case 3: default: } The default is unreachable. Having it there just introduces more code that serves no purpose. Unless someone does something totally foolish and changes the mask in the switch statement, there's no way it will ever be reachable. > If an assert/abort can be triggered by a guest then you obviously have a bug. > Agreed. > Removing the assert is not the correct solution. You should either fix > whatever caused the invalid state to occur, or replace it with an appropriate > retry, fallback or guest visible failure. > Also agreed. Regards, Anthony Liguori > Paul >
> > If something should never happen (as in this case) then an abort/assert > > is completely appropriate. Once things get that screwed up there's no > > right answer, and the best thing we can do is terminate immediately to > > try and avoid further damage. > > This case was: > > switch (foo & 0x03) { > case 0: case 1: case 2: case 3: > default: > } > > The default is unreachable. Having it there just introduces more code > that serves no purpose. Unless someone does something totally foolish > and changes the mask in the switch statement, there's no way it will > ever be reachable. I mistakenly remembered this was using a symbolic mask rather than a literal 0x03. In that case I'd argue it's much easier to make the dumb error you describe, and the assert can be a handy cluebat. I guess it's largely personal preference - I prefer to add the default case to make it clear that falling through is never gong to be the right answer. Paul
On 3/17/10, Paul Brook <paul@codesourcery.com> wrote: > > > If something should never happen (as in this case) then an abort/assert > > > is completely appropriate. Once things get that screwed up there's no > > > right answer, and the best thing we can do is terminate immediately to > > > try and avoid further damage. > > > > This case was: > > > > switch (foo & 0x03) { > > case 0: case 1: case 2: case 3: > > default: > > } > > > > The default is unreachable. Having it there just introduces more code > > that serves no purpose. Unless someone does something totally foolish > > and changes the mask in the switch statement, there's no way it will > > ever be reachable. > > > I mistakenly remembered this was using a symbolic mask rather than a literal > 0x03. In that case I'd argue it's much easier to make the dumb error you > describe, and the assert can be a handy cluebat. > > I guess it's largely personal preference - I prefer to add the default case to > make it clear that falling through is never gong to be the right answer. This breaks build (gcc 4.3.2): CC usb-linux.o cc1: warnings being treated as errors /src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table': /src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in this function
On 03/17/2010 03:15 PM, Blue Swirl wrote: > On 3/17/10, Paul Brook<paul@codesourcery.com> wrote: > >>>> If something should never happen (as in this case) then an abort/assert >>>> >> > > is completely appropriate. Once things get that screwed up there's no >> > > right answer, and the best thing we can do is terminate immediately to >> > > try and avoid further damage. >> > >> > This case was: >> > >> > switch (foo& 0x03) { >> > case 0: case 1: case 2: case 3: >> > default: >> > } >> > >> > The default is unreachable. Having it there just introduces more code >> > that serves no purpose. Unless someone does something totally foolish >> > and changes the mask in the switch statement, there's no way it will >> > ever be reachable. >> >> >> I mistakenly remembered this was using a symbolic mask rather than a literal >> 0x03. In that case I'd argue it's much easier to make the dumb error you >> describe, and the assert can be a handy cluebat. >> >> I guess it's largely personal preference - I prefer to add the default case to >> make it clear that falling through is never gong to be the right answer. >> > This breaks build (gcc 4.3.2): > CC usb-linux.o > cc1: warnings being treated as errors > /src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table': > /src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in > this function > That's unfortunate. I'll revert. Regards, Anthony Liguori
On Wed, 2010-03-17 at 15:41 -0500, Anthony Liguori wrote: > On 03/17/2010 03:15 PM, Blue Swirl wrote: > > This breaks build (gcc 4.3.2): > > CC usb-linux.o > > cc1: warnings being treated as errors > > /src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table': > > /src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in > > this function > > > > That's unfortunate. I'll revert. I can't reproduce this with gcc-4.4.3-8.fc13.i686 (which I'm currently running). The patch was tested and submitted when I was running gcc-4.4.3-6.fc13.i686. Regards, Paul Bolle
On 03/17/2010 03:56 PM, Paul Bolle wrote: > On Wed, 2010-03-17 at 15:41 -0500, Anthony Liguori wrote: > >> On 03/17/2010 03:15 PM, Blue Swirl wrote: >> >>> This breaks build (gcc 4.3.2): >>> CC usb-linux.o >>> cc1: warnings being treated as errors >>> /src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table': >>> /src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in >>> this function >>> >>> >> That's unfortunate. I'll revert. >> > I can't reproduce this with gcc-4.4.3-8.fc13.i686 (which I'm currently > running). The patch was tested and submitted when I was running > gcc-4.4.3-6.fc13.i686. > Yeah, it worked for me, but clearly older versions of gcc are less smart about calculating ranges. Regards, Anthony Liguori > Regards, > > > Paul Bolle > >
On 3/17/10, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 03/17/2010 03:56 PM, Paul Bolle wrote: > > > On Wed, 2010-03-17 at 15:41 -0500, Anthony Liguori wrote: > > > > > > > On 03/17/2010 03:15 PM, Blue Swirl wrote: > > > > > > > > > > This breaks build (gcc 4.3.2): > > > > CC usb-linux.o > > > > cc1: warnings being treated as errors > > > > /src/qemu/usb-linux.c: In function 'usb_linux_update_endp_table': > > > > /src/qemu/usb-linux.c:759: error: 'type' may be used uninitialized in > > > > this function > > > > > > > > > > > > > > > That's unfortunate. I'll revert. > > > > > > > > I can't reproduce this with gcc-4.4.3-8.fc13.i686 (which I'm currently > > running). The patch was tested and submitted when I was running > > gcc-4.4.3-6.fc13.i686. > > > > > > Yeah, it worked for me, but clearly older versions of gcc are less smart > about calculating ranges. Actually OpenBSD gcc (3.4.4) has no problems.
diff --git a/usb-linux.c b/usb-linux.c index a9c15c6..23155dd 100644 --- a/usb-linux.c +++ b/usb-linux.c @@ -846,9 +846,6 @@ static int usb_linux_update_endp_table(USBHostDevice *s) case 0x03: type = USBDEVFS_URB_TYPE_INTERRUPT; break; - default: - DPRINTF("usb_host: malformed endpoint type\n"); - type = USBDEVFS_URB_TYPE_BULK; } s->endp_table[(devep & 0xf) - 1].type = type; s->endp_table[(devep & 0xf) - 1].halted = 0;
Signed-off-by: Paul Bolle <pebolle@tiscali.nl> --- usb-linux.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-)