Re: [PATCH v4 1/2] next-kbd: convert to use qemu_input_handler_register()

[Philippe Mathieu-Daudé]

On 8/11/24 13:13, BALATON Zoltan wrote:
> On Fri, 8 Nov 2024, Thomas Huth wrote:
> > On 06/11/2024 21.32, BALATON Zoltan wrote:
> > > On Wed, 6 Nov 2024, Philippe Mathieu-Daudé wrote:
> > > > On 6/11/24 13:00, BALATON Zoltan wrote:
> > > > > On Wed, 6 Nov 2024, Mark Cave-Ayland wrote:
> > > > > > Convert the next-kbd device from the legacy UI 
> > > > > > qemu_add_kbd_event_handler()
> > > > > > function to use qemu_input_handler_register().
> > > > > >
> > > > > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > > > > Reviewed-by: Thomas Huth <huth@tuxfamily.org>
> > > > > > ---
> > > > > > hw/m68k/next-kbd.c | 163 
> > > > > > ++++++++++++++++++++++++++++++---------------
> > > > > > 1 file changed, 108 insertions(+), 55 deletions(-)
> > > >
> > > >
> > > > > > -static const unsigned char next_keycodes[128] = {
> > > > > > -    0x00, 0x49, 0x4A, 0x4B, 0x4C, 0x4D, 0x50, 0x4F,
> > > > > > -    0x4E, 0x1E, 0x1F, 0x20, 0x1D, 0x1C, 0x1B, 0x00,
> > > > > > -    0x42, 0x43, 0x44, 0x45, 0x48, 0x47, 0x46, 0x06,
> > > > > > -    0x07, 0x08, 0x00, 0x00, 0x2A, 0x00, 0x39, 0x3A,
> > > > > > -    0x3B, 0x3C, 0x3D, 0x40, 0x3F, 0x3E, 0x2D, 0x2C,
> > > > > > -    0x2B, 0x26, 0x00, 0x00, 0x31, 0x32, 0x33, 0x34,
> > > > > > -    0x35, 0x37, 0x36, 0x2e, 0x2f, 0x30, 0x00, 0x00,
> > > > > > -    0x00, 0x38, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > > > > > -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > > > > > -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > > > > > -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
> > > > > > -    0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
> > > > > > +#define NEXTKBD_NO_KEY 0xff
> > > > >
> > > > > Now you don't need this 0xff define any more because you can use 0 
> > > > > as no key value then the [0 ... Q_KEY_CODE__MAX] init below can 
> > > > > also be dropped because static variables are 0 init automatically.
> > > >
> > > > Whether 0 or 0xff is best for NO_KEY, I don't know.
> > > > However, definitions are useful when reviewing ...
> > > >
> > > > > Regards,
> > > > > BALATON Zoltan
> > > > >
> > > > > > +static const int qcode_to_nextkbd_keycode[] = {
> > > > > > +    /* Make sure future additions are automatically set to 
> > > > > > NEXTKBD_NO_KEY */
> > > > > > +    [0 ... Q_KEY_CODE__MAX]    = NEXTKBD_NO_KEY,
> > > > > > +
> > > > > > +    [Q_KEY_CODE_ESC]           = 0x49,
> > > > > > +    [Q_KEY_CODE_1]             = 0x4a,
> > > > > > +    [Q_KEY_CODE_2]             = 0x4b,
> > > > > > +    [Q_KEY_CODE_3]             = 0x4c,
> > > > > > +    [Q_KEY_CODE_4]             = 0x4d,
> > > > [...]
> > > >
> > > > > > +static void nextkbd_event(DeviceState *dev, QemuConsole *src, 
> > > > > > InputEvent *evt)
> > > > > > +{
> > > > > > +    NextKBDState *s = NEXTKBD(dev);
> > > > > > +    int qcode, keycode;
> > > > > > +    bool key_down = evt->u.key.data->down;
> > > > > > +
> > > > > > +    qcode = qemu_input_key_value_to_qcode(evt->u.key.data->key);
> > > > > > +    if (qcode >= ARRAY_SIZE(qcode_to_nextkbd_keycode)) {
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    /* Shift key currently has no keycode, so handle separately */
> > > > > > +    if (qcode == Q_KEY_CODE_SHIFT) {
> > > > > > +        if (key_down) {
> > > > > > +            s->shift |= KD_LSHIFT;
> > > > > > +        } else {
> > > > > > +            s->shift &= ~KD_LSHIFT;
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > > +    if (qcode == Q_KEY_CODE_SHIFT_R) {
> > > > > > +        if (key_down) {
> > > > > > +            s->shift |= KD_RSHIFT;
> > > > > > +        } else {
> > > > > > +            s->shift &= ~KD_RSHIFT;
> > > > > > +        }
> > > > > > +    }
> > > > > > +
> > > > > > +    keycode = qcode_to_nextkbd_keycode[qcode];
> > > > > > +    if (keycode == NEXTKBD_NO_KEY) {
> > > >
> > > > ... here ^
> > >
> > > I this case !keycode is pretty self explanatory IMO.
> >
> > Ok, I'll pick up the patch with this change added on top:
> >
> > diff --git a/hw/m68k/next-kbd.c b/hw/m68k/next-kbd.c
> > --- a/hw/m68k/next-kbd.c
> > +++ b/hw/m68k/next-kbd.c
> > @@ -165,12 +165,7 @@ static const MemoryRegionOps kbd_ops = {
> >     .endianness = DEVICE_NATIVE_ENDIAN,
> > };
> > -#define NEXTKBD_NO_KEY 0xff
> > -
> > static const int qcode_to_nextkbd_keycode[] = {
> > -    /* Make sure future additions are automatically set to 
> > NEXTKBD_NO_KEY */
> > -    [0 ... Q_KEY_CODE__MAX]    = NEXTKBD_NO_KEY,
>
> Thinking about it more, removing this may make the array smaller so we'd 
> either need some max value define (or get it something like 
> qcode_to_nextkbd_keycode[ARRAY_SIZE(qcode_to_nextkbd_keycode) - 1] or 
> so) and check if qcode is not > than that or declare the array as 
> [Q_KEY_CODE__MAX] to make sure we're not trying to access values after 
> the end.  Maybe it's simplest to do 
> qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] as this is not much wasted 
> space, unless this can't overflow for some other reason I don't know about.

Agreed, qcode_to_nextkbd_keycode[Q_KEY_CODE__MAX] is future-proof.

> Regards,
> BALATON Zoltan
>
> > -
> >     [Q_KEY_CODE_ESC]           = 0x49,
> >     [Q_KEY_CODE_1]             = 0x4a,
> >     [Q_KEY_CODE_2]             = 0x4b,
> > @@ -276,7 +271,7 @@ static void nextkbd_event(DeviceState *dev, 
> > QemuConsole *src, InputEvent *evt)
> >     }
> >      keycode = qcode_to_nextkbd_keycode[qcode];
> > -    if (keycode == NEXTKBD_NO_KEY) {
> > +    if (!keycode) {
> >         return;
> >     }
> >  Thomas