All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
@ 2010-03-08 12:58 Paul Bolle
  2010-03-08 13:31 ` Paul Brook
  2010-03-17 15:59 ` Anthony Liguori
  0 siblings, 2 replies; 13+ messages in thread
From: Paul Bolle @ 2010-03-08 12:58 UTC (permalink / raw)
  To: qemu-devel

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-08 12:58 [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement Paul Bolle
@ 2010-03-08 13:31 ` Paul Brook
  2010-03-17 15:59 ` Anthony Liguori
  1 sibling, 0 replies; 13+ messages in thread
From: Paul Brook @ 2010-03-08 13:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: 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;
> 

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-08 12:58 [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement Paul Bolle
  2010-03-08 13:31 ` Paul Brook
@ 2010-03-17 15:59 ` Anthony Liguori
  2010-03-17 16:14   ` Paul Bolle
  1 sibling, 1 reply; 13+ messages in thread
From: Anthony Liguori @ 2010-03-17 15:59 UTC (permalink / raw)
  To: Paul Bolle; +Cc: qemu-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 15:59 ` Anthony Liguori
@ 2010-03-17 16:14   ` Paul Bolle
  2010-03-17 16:39     ` Anthony Liguori
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Bolle @ 2010-03-17 16:14 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: qemu-devel, Paul Brook

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 16:14   ` Paul Bolle
@ 2010-03-17 16:39     ` Anthony Liguori
  2010-03-17 17:08       ` Paul Brook
  0 siblings, 1 reply; 13+ messages in thread
From: Anthony Liguori @ 2010-03-17 16:39 UTC (permalink / raw)
  To: Paul Bolle; +Cc: qemu-devel, 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.

Regards,

Anthony Liguori

> Regards,
>
>
> Paul Bolle
>
>    

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 16:39     ` Anthony Liguori
@ 2010-03-17 17:08       ` Paul Brook
  2010-03-17 17:15         ` Anthony Liguori
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Brook @ 2010-03-17 17:08 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Paul Bolle, qemu-devel

> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 17:08       ` Paul Brook
@ 2010-03-17 17:15         ` Anthony Liguori
  2010-03-17 17:43           ` Paul Brook
  0 siblings, 1 reply; 13+ messages in thread
From: Anthony Liguori @ 2010-03-17 17:15 UTC (permalink / raw)
  To: Paul Brook; +Cc: Paul Bolle, qemu-devel

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
>    

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 17:15         ` Anthony Liguori
@ 2010-03-17 17:43           ` Paul Brook
  2010-03-17 20:15             ` Blue Swirl
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Brook @ 2010-03-17 17:43 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Paul Bolle, qemu-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 17:43           ` Paul Brook
@ 2010-03-17 20:15             ` Blue Swirl
  2010-03-17 20:41               ` Anthony Liguori
  0 siblings, 1 reply; 13+ messages in thread
From: Blue Swirl @ 2010-03-17 20:15 UTC (permalink / raw)
  To: Paul Brook; +Cc: Paul Bolle, qemu-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 20:15             ` Blue Swirl
@ 2010-03-17 20:41               ` Anthony Liguori
  2010-03-17 20:56                 ` Paul Bolle
  0 siblings, 1 reply; 13+ messages in thread
From: Anthony Liguori @ 2010-03-17 20:41 UTC (permalink / raw)
  To: Blue Swirl; +Cc: Paul Bolle, Paul Brook, qemu-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 20:41               ` Anthony Liguori
@ 2010-03-17 20:56                 ` Paul Bolle
  2010-03-17 20:59                   ` Anthony Liguori
  0 siblings, 1 reply; 13+ messages in thread
From: Paul Bolle @ 2010-03-17 20:56 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Blue Swirl, Paul Brook, qemu-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 20:56                 ` Paul Bolle
@ 2010-03-17 20:59                   ` Anthony Liguori
  2010-03-17 21:05                     ` Blue Swirl
  0 siblings, 1 reply; 13+ messages in thread
From: Anthony Liguori @ 2010-03-17 20:59 UTC (permalink / raw)
  To: Paul Bolle; +Cc: Blue Swirl, Paul Brook, qemu-devel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement
  2010-03-17 20:59                   ` Anthony Liguori
@ 2010-03-17 21:05                     ` Blue Swirl
  0 siblings, 0 replies; 13+ messages in thread
From: Blue Swirl @ 2010-03-17 21:05 UTC (permalink / raw)
  To: Anthony Liguori; +Cc: Paul Bolle, Paul Brook, qemu-devel

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2010-03-17 21:05 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-03-08 12:58 [Qemu-devel] [PATCH] [TRIVIAL] usb-linux: remove unreachable default in switch statement Paul Bolle
2010-03-08 13:31 ` Paul Brook
2010-03-17 15:59 ` Anthony Liguori
2010-03-17 16:14   ` Paul Bolle
2010-03-17 16:39     ` Anthony Liguori
2010-03-17 17:08       ` Paul Brook
2010-03-17 17:15         ` Anthony Liguori
2010-03-17 17:43           ` Paul Brook
2010-03-17 20:15             ` Blue Swirl
2010-03-17 20:41               ` Anthony Liguori
2010-03-17 20:56                 ` Paul Bolle
2010-03-17 20:59                   ` Anthony Liguori
2010-03-17 21:05                     ` Blue Swirl

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.