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

[Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>]

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