Re: Handling of large keycodes

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sat, Jul 31, 2010 at 01:03:26PM +0200, Stefan Richter wrote:
> Dmitry Torokhov wrote:
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -56,22 +56,35 @@ struct input_absinfo {
> >  	__s32 resolution;
> >  };
> >  
> > -struct keycode_table_entry {
> > -	__u32 keycode;		/* e.g. KEY_A */
> > -	__u32 index;            /* Index for the given scan/key table, on EVIOCGKEYCODEBIG */
> > -	__u32 len;		/* Length of the scancode */
> > -	__u32 reserved[2];	/* Reserved for future usage */
> > -	char *scancode;		/* scancode, in machine-endian */
> > +/**
> > + * struct keymap_entry - used by EVIOCGKEYCODE/EVIOCSKEYCODE ioctls
> > + * @scancode: scancode represented in machine-endian form.
> > + * @len: length of the scancode that resides in @scancode buffer.
> > + * @index: index in the keymap, may be used instead of scancode
> > + * @by_index: boolean value indicating that kernel should perform
> > + *	lookup in keymap by @index instead of @scancode
> > + * @keycode: key code assigned to this scancode
> > + *
> > + * The structure is used to retrieve and modify keymap data. Users have
> > + * of performing lookup either by @scancode itself or by @index in
> > + * keymap entry. EVIOCGKEYCODE will also return scancode or index
> > + * (depending on which element was used to perform lookup).
> > + */
> > +struct keymap_entry {
> > +	__u8  len;
> > +	__u8  by_index;
> > +	__u16 index;
> > +	__u32 keycode;
> > +	__u8  scancode[32];
> >  };
> 
> I agree with Dimitry; don't put a pointer typed member into a userspace
> ABI struct.
> 
> Two remarks:
> 
>   - Presently, <linux/input.h> defines three structs named input_... for
>     userspace, two structs named input_... for kernelspace, and a few
>     structs named ff_... specially for force-feedback stuff.  How about
>     calling struct keymap_entry perhaps struct input_keymap_entry
>     instead, to keep namespaces tidy?

Good idea, changed.

> 
>   - I take it from your description that scan codes are fundamentally
>     variable-length data.  How about defining it as __u8 scancode[0]?

In addition to difficulty in adding members (as you mentioned) it also
makes it more difficult for users to allocate such variables on stack or
make them global (anything but malloc with additional memory buffer)...

-- 
Dmitry