[RFC,13/14] hw/char/terminal3270: Explicit ignored QEMUChrEvent in IOEventHandler

Commit Message

Philippe Mathieu-Daudé Dec. 17, 2019, 4:38 p.m. UTC

The Chardev events are listed in the QEMUChrEvent enum. To be
able to use this enum in the IOEventHandler typedef, we need to
explicit when frontends ignore some events, to silent GCC the
following warnings:

    CC      s390x-softmmu/hw/char/terminal3270.o
  hw/char/terminal3270.c: In function ‘chr_event’:
  hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_BREAK’ not handled in switch [-Werror=switch]
    156 |     switch (event) {
        |     ^~~~~~
  hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_MUX_IN’ not handled in switch [-Werror=switch]
  hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_MUX_OUT’ not handled in switch [-Werror=switch]
  cc1: all warnings being treated as errors

Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
---
Cc: Cornelia Huck <[e-mail]>
Cc: Halil Pasic <[e-mail]>
Cc: Christian Borntraeger <[e-mail]>
Cc: "Marc-André Lureau" <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Cc: [e-mail]
---
 hw/char/terminal3270.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Markus Armbruster Dec. 19, 2019, 6:52 a.m. UTC | #1

Philippe Mathieu-Daudé <[e-mail]> writes:

> The Chardev events are listed in the QEMUChrEvent enum. To be
> able to use this enum in the IOEventHandler typedef, we need to
> explicit when frontends ignore some events, to silent GCC the
> following warnings:
>
>     CC      s390x-softmmu/hw/char/terminal3270.o
>   hw/char/terminal3270.c: In function ‘chr_event’:
>   hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_BREAK’ not handled in switch [-Werror=switch]
>     156 |     switch (event) {
>         |     ^~~~~~
>   hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_MUX_IN’ not handled in switch [-Werror=switch]
>   hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_MUX_OUT’ not handled in switch [-Werror=switch]
>   cc1: all warnings being treated as errors
>
> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
> ---
> Cc: Cornelia Huck <[e-mail]>
> Cc: Halil Pasic <[e-mail]>
> Cc: Christian Borntraeger <[e-mail]>
> Cc: "Marc-André Lureau" <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Cc: [e-mail]
> ---
>  hw/char/terminal3270.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
> index 6859c1bcb2..9e59a2d92b 100644
> --- a/hw/char/terminal3270.c
> +++ b/hw/char/terminal3270.c
> @@ -166,6 +166,9 @@ static void chr_event(void *opaque, int event)
>          sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
>          css_conditional_io_interrupt(sch);
>          break;
> +    default:
> +        /* Ignore */
> +        break;
>      }
>  }

I doubt the /* Ignore */ comment is worth its keep.

Splitting PATCH 02-13 feels excessive to me.

Philippe Mathieu-Daudé Dec. 19, 2019, 1:09 p.m. UTC | #2

Hi Markus,

On 12/19/19 7:52 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <[e-mail]> writes:
> 
>> The Chardev events are listed in the QEMUChrEvent enum. To be
>> able to use this enum in the IOEventHandler typedef, we need to
>> explicit when frontends ignore some events, to silent GCC the
>> following warnings:
>>
>>      CC      s390x-softmmu/hw/char/terminal3270.o
>>    hw/char/terminal3270.c: In function ‘chr_event’:
>>    hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_BREAK’ not handled in switch [-Werror=switch]
>>      156 |     switch (event) {
>>          |     ^~~~~~
>>    hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_MUX_IN’ not handled in switch [-Werror=switch]
>>    hw/char/terminal3270.c:156:5: error: enumeration value ‘CHR_EVENT_MUX_OUT’ not handled in switch [-Werror=switch]
>>    cc1: all warnings being treated as errors
>>
>> Signed-off-by: Philippe Mathieu-Daudé <[e-mail]>
>> ---
>> Cc: Cornelia Huck <[e-mail]>
>> Cc: Halil Pasic <[e-mail]>
>> Cc: Christian Borntraeger <[e-mail]>
>> Cc: "Marc-André Lureau" <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>> Cc: [e-mail]
>> ---
>>   hw/char/terminal3270.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
>> index 6859c1bcb2..9e59a2d92b 100644
>> --- a/hw/char/terminal3270.c
>> +++ b/hw/char/terminal3270.c
>> @@ -166,6 +166,9 @@ static void chr_event(void *opaque, int event)
>>           sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
>>           css_conditional_io_interrupt(sch);
>>           break;
>> +    default:
>> +        /* Ignore */
>> +        break;
>>       }
>>   }
> 
> I doubt the /* Ignore */ comment is worth its keep.

OK I don't mind dropping it.

> Splitting PATCH 02-13 feels excessive to me.

I agree, but I have the feeling when a patch touch many subsystems, we 
don't wait for all the maintainers to Ack it, we are fine with 2 or 3.
In this case, maybe a subsystem neglected a QEMUChrEvent case, so I 
prefer to have each of them to confirm we can ignore the missing cases.

In v2 I don't replace by a 'default' entry, all the cases are explicit.

Regards,

Phil.

Patch

diff --git a/hw/char/terminal3270.c b/hw/char/terminal3270.c
index 6859c1bcb2..9e59a2d92b 100644
--- a/hw/char/terminal3270.c
+++ b/hw/char/terminal3270.c
@@ -166,6 +166,9 @@  static void chr_event(void *opaque, int event)
         sch->curr_status.scsw.dstat = SCSW_DSTAT_DEVICE_END;
         css_conditional_io_interrupt(sch);
         break;
+    default:
+        /* Ignore */
+        break;
     }
 }