[Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Bolle]

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;
-- 
1.7.0.1

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Brook]

> 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

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

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;
>    

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Bolle]

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

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

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
>
>    

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Brook]

> 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

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

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
>    

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Brook]

> > 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

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Blue Swirl]

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

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Paul Bolle]

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

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Anthony Liguori]

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
>
>    

Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement

[Blue Swirl]

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.