[Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Daniel Henrique Barboza]

This patch adds a break in the switch() statement of complete(),
value 0x42:

    case 0x42:              /* FT2 sets output freq with this, go figure */
        qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
                      " should\n");
        break; <-------
    case 0x41:

The issue was found by Coverity (#1385841):

    CID 1385841:  Control flow issues  (MISSING_BREAK)
    The case for value "66" is not terminated by a 'break' statement.

Fixes: 8ec660b80e ("hw/audio/sb16.c: change dolog() to qemu_log_mask()")
Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
CC: John Arbuckle <programmingkidx@gmail.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/audio/sb16.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index 31de264ab7..b2fdcd8437 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -744,6 +744,7 @@ static void complete (SB16State *s)
         case 0x42:              /* FT2 sets output freq with this, go figure */
             qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
                           " should\n");
+            break;
         case 0x41:
             s->freq = dsp_get_hilo (s);
             ldebug ("set freq %d\n", s->freq);
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Philippe Mathieu-Daudé]

Hi Daniel,

On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote:
> This patch adds a break in the switch() statement of complete(),
> value 0x42:
> 
>     case 0x42:              /* FT2 sets output freq with this, go figure */
>         qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
>                       " should\n");
>         break; <-------
>     case 0x41:

It seems this is an intentional fallthrough, I understand cmd 0x42 is
expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415).

> 
> The issue was found by Coverity (#1385841):
> 
>     CID 1385841:  Control flow issues  (MISSING_BREAK)
>     The case for value "66" is not terminated by a 'break' statement.
> 
> Fixes: 8ec660b80e ("hw/audio/sb16.c: change dolog() to qemu_log_mask()")
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> CC: John Arbuckle <programmingkidx@gmail.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/audio/sb16.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
> index 31de264ab7..b2fdcd8437 100644
> --- a/hw/audio/sb16.c
> +++ b/hw/audio/sb16.c
> @@ -744,6 +744,7 @@ static void complete (SB16State *s)
>          case 0x42:              /* FT2 sets output freq with this, go figure */
>              qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
>                            " should\n");
> +            break;
>          case 0x41:
>              s->freq = dsp_get_hilo (s);
>              ldebug ("set freq %d\n", s->freq);
> 

Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Peter Maydell]

On 8 February 2018 at 12:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Daniel,
>
> On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote:
>> This patch adds a break in the switch() statement of complete(),
>> value 0x42:
>>
>>     case 0x42:              /* FT2 sets output freq with this, go figure */
>>         qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
>>                       " should\n");
>>         break; <-------
>>     case 0x41:
>
> It seems this is an intentional fallthrough, I understand cmd 0x42 is
> expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415).

Yes, I agree; I wrote a bit about this in this thread:
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02081.html

(though my guess is that actually 0x42 is supposed to do exactly
what 0x41 does, and that the LOG_UNIMP should maybe just be removed).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Daniel P. Berrangé]

On Thu, Feb 08, 2018 at 09:15:10AM -0300, Philippe Mathieu-Daudé wrote:
> Hi Daniel,
> 
> On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote:
> > This patch adds a break in the switch() statement of complete(),
> > value 0x42:
> > 
> >     case 0x42:              /* FT2 sets output freq with this, go figure */
> >         qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
> >                       " should\n");
> >         break; <-------
> >     case 0x41:
> 
> It seems this is an intentional fallthrough, I understand cmd 0x42 is
> expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415).

It might be nice to turn on -Wimplicit-fallthrough and then annotate
valid locations like this in qemu with  /* fallthrough */

Although GCC has an __attribute((fallthrough)), the warning flag impl
also looks for that magic comment, and the magic comment is portable
to clang too.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Philippe Mathieu-Daudé]

On 02/08/2018 10:01 AM, Peter Maydell wrote:
> On 8 February 2018 at 12:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Daniel,
>>
>> On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote:
>>> This patch adds a break in the switch() statement of complete(),
>>> value 0x42:
>>>
>>>     case 0x42:              /* FT2 sets output freq with this, go figure */
>>>         qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
>>>                       " should\n");
>>>         break; <-------
>>>     case 0x41:
>>
>> It seems this is an intentional fallthrough, I understand cmd 0x42 is
>> expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415).
> 
> Yes, I agree; I wrote a bit about this in this thread:
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02081.html

Oh, very useful link!

> (though my guess is that actually 0x42 is supposed to do exactly
> what 0x41 does, and that the LOG_UNIMP should maybe just be removed).

I now understand 0x42 sets the dsp input sampling freq, the model seems
to be designed with output in mind, then added input support (using same
freq as output).

So imho the simpler/safer fix would be:

  case 0x42:
      if (dsp_get_hilo(s) != s->freq) {
          qemu_log_mask(LOG_UNIMP,
                        "input sampling freq different than "
                        "output not implemented");
      }
      /* fallthrough */
  case 0x41:
      ...

and the correct fix would be split s->freq in {s->freq_in, s->freq_out}
but nobody ever required this during at least 14 years.

Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Philippe Mathieu-Daudé]

On 02/08/2018 10:16 AM, Philippe Mathieu-Daudé wrote:
> On 02/08/2018 10:01 AM, Peter Maydell wrote:
>> On 8 February 2018 at 12:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Hi Daniel,
>>>
>>> On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote:
>>>> This patch adds a break in the switch() statement of complete(),
>>>> value 0x42:
>>>>
>>>>     case 0x42:              /* FT2 sets output freq with this, go figure */
>>>>         qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
>>>>                       " should\n");
>>>>         break; <-------
>>>>     case 0x41:
>>>
>>> It seems this is an intentional fallthrough, I understand cmd 0x42 is
>>> expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415).
>>
>> Yes, I agree; I wrote a bit about this in this thread:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02081.html
> 
> Oh, very useful link!
> 
>> (though my guess is that actually 0x42 is supposed to do exactly
>> what 0x41 does, and that the LOG_UNIMP should maybe just be removed).
> 
> I now understand 0x42 sets the dsp input sampling freq, the model seems
> to be designed with output in mind, then added input support (using same
> freq as output).

Now I see Fabrice comment "FT2 sets output freq with this, go figure"
and agree with him.

I like to think this is a bug in Fast Tracker 2, so Peter suggestion
about using LOG_GUEST_ERROR here might be clever.

> 
> So imho the simpler/safer fix would be:
> 
>   case 0x42:
>       if (dsp_get_hilo(s) != s->freq) {
>           qemu_log_mask(LOG_UNIMP,
>                         "input sampling freq different than "
>                         "output not implemented");
>       }
>       /* fallthrough */
>   case 0x41:
>       ...
> 
> and the correct fix would be split s->freq in {s->freq_in, s->freq_out}
> but nobody ever required this during at least 14 years.
> 

Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Peter Maydell]

On 8 February 2018 at 13:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Now I see Fabrice comment "FT2 sets output freq with this, go figure"
> and agree with him.
>
> I like to think this is a bug in Fast Tracker 2, so Peter suggestion
> about using LOG_GUEST_ERROR here might be clever.
>
>>
>> So imho the simpler/safer fix would be:
>>
>>   case 0x42:
>>       if (dsp_get_hilo(s) != s->freq) {
>>           qemu_log_mask(LOG_UNIMP,
>>                         "input sampling freq different than "
>>                         "output not implemented");
>>       }
>>       /* fallthrough */
>>   case 0x41:
>>       ...

Wouldn't this falsely report a warning for guest code that really
is trying to set the input sampling frequency and doesn't care
about output?

>> and the correct fix would be split s->freq in {s->freq_in, s->freq_out}

...but that would differ from the hardware implementation, which
(apparently) uses a single frequency for both.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement

[Daniel Henrique Barboza]

Hey,

On 02/08/2018 10:15 AM, Philippe Mathieu-Daudé wrote:
> Hi Daniel,
>
> On 02/08/2018 07:57 AM, Daniel Henrique Barboza wrote:
>> This patch adds a break in the switch() statement of complete(),
>> value 0x42:
>>
>>      case 0x42:              /* FT2 sets output freq with this, go figure */
>>          qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
>>                        " should\n");
>>          break; <-------
>>      case 0x41:
> It seems this is an intentional fallthrough, I understand cmd 0x42 is
> expected to do the same of 0x41 and _a bit more_ (see commit 85571bc7415).

Perhaps it should be more explicit then? Something like

         case 0x40:
             s->time_const = dsp_get_data (s);
             ldebug ("set time const %d\n", s->time_const);
             break;

         case 0x41:
         case 0x42:
              if (s->cmd == 0x42) {
                   /* FT2 sets output freq with this, go figure */
                 qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it 
think it"
                                          " should\n");
             }
             s->freq = dsp_get_hilo (s);
             ldebug ("set freq %d\n", s->freq);
             break;

         case 0x48:



>
>> The issue was found by Coverity (#1385841):
>>
>>      CID 1385841:  Control flow issues  (MISSING_BREAK)
>>      The case for value "66" is not terminated by a 'break' statement.
>>
>> Fixes: 8ec660b80e ("hw/audio/sb16.c: change dolog() to qemu_log_mask()")
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
>> CC: John Arbuckle <programmingkidx@gmail.com>
>> CC: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>   hw/audio/sb16.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
>> index 31de264ab7..b2fdcd8437 100644
>> --- a/hw/audio/sb16.c
>> +++ b/hw/audio/sb16.c
>> @@ -744,6 +744,7 @@ static void complete (SB16State *s)
>>           case 0x42:              /* FT2 sets output freq with this, go figure */
>>               qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
>>                             " should\n");
>> +            break;
>>           case 0x41:
>>               s->freq = dsp_get_hilo (s);
>>               ldebug ("set freq %d\n", s->freq);
>>