Re: [Qemu-devel] [PATCH 1/1] hw/audio/sb16.c: missing break statement
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
CC: John Arbuckle
CC: Gerd Hoffmann
---
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
On 8 February 2018 at 13:34, Philippe Mathieu-Daudé 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
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é 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
On 02/08/2018 10:01 AM, Peter Maydell wrote:
> On 8 February 2018 at 12:15, 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).
>
> 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
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
On 8 February 2018 at 12:15, 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). 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
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
> CC: John Arbuckle
> CC: Gerd Hoffmann
> ---
> 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);
>
