[RFC/PATCH v1 1/3] fix decoder of ioctl EVIOCGBIT

[Zhibin Li]

On Sun, Aug 19, 2018 at 4:38 PM Dmitry V. Levin <ldv at altlinux.org> wrote:

> On Fri, Aug 10, 2018 at 07:12:42PM +0800, Zhibin Li wrote:
> > EV_SYN in EVIOCGBIT should be decoded to get other event types EV_*, not
> > the precise event codes SYN_*.
> >
> > * evdev.c: Include "xlat/evdev_ev.h" and remove "xlat/evdev_sync.h".
> > (decode_bitset_): Fix the incorrect comparison between tcp->u_rval and
> > max_nr.
> > (bit_ioctl) <case EV_SYN>: Use evdev_ev and XT_NORMAL in decode_bitset
> > invocation instead.
> > * ioctl.c: Print 0 instead of EV_SYN if nr equals to 0x20.
> > ---
> >  evdev.c | 10 +++++-----
> >  ioctl.c |  6 +++++-
> >  2 files changed, 10 insertions(+), 6 deletions(-)
> >
> > diff --git a/evdev.c b/evdev.c
> > index 7ca15c9d..34732a2d 100644
> > --- a/evdev.c
> > +++ b/evdev.c
> > @@ -37,6 +37,7 @@
> >  # include <linux/input.h>
> >
> >  # include "xlat/evdev_autorepeat.h"
> > +# include "xlat/evdev_ev.h"
>
> This way "xlat/evdev_ev.h" would be included by two different source
> files, which would end up with static evdev_ev[] being defined twice.
>
I didn't notice that. Thanks for pointing it out :)

>
> This case looks similar to evdev_abs.  Let's avoid this duplication
> by adding a declaration to defs.h and making evdev.c the place where
> evdev_ev is defined.
>
> For example,
>
> Subject: [PATCH] evdev: fix decoding of EVIOCGBIT(0, ...)
>
> There is a comment in drivers/input/evdev.c which says:
> /* EV_SYN==0 is EV_CNT, _not_ SYN_CNT, see EVIOCGBIT */
>
> That is, EVIOCGBIT(0, ...) should return a bit mask with supported
> event types instead of SYN_* event codes.
>
> * defs.h (evdev_ev): New prototype.
> * evdev.c: Include "xlat/evdev_ev.h" and remove "xlat/evdev_sync.h".
> (bit_ioctl) <case EV_SYN>: Replace EV_SYN with 0, use evdev_ev
> with XT_SORTED in decode_bitset invocation instead.
> * ioctl.c: Do not include "xlat/evdev_ev.h".
> (evdev_decode_number): Print nr == 0x20 as "0" instead of "EV_SYN".
> * tests/ioctl_evdev.c (main): Use 0 instead of EV_SYN in EVIOCGBIT
> output.
> * xlat/evdev_sync.in: Remove.
>
> diff --git a/defs.h b/defs.h
> index 0d4bf82b3..2c19dd3a4 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -330,6 +330,7 @@ extern const struct xlat evdev_abs[];
>  /** Number of elements in evdev_abs array without the terminating record.
> */
>  extern const size_t evdev_abs_size;
>
> +extern const struct xlat evdev_ev[];
>  extern const struct xlat iffflags[];
>  extern const struct xlat ip_type_of_services[];
>  extern const struct xlat ipc_private[];
> diff --git a/evdev.c b/evdev.c
> index 3c1aaa8e2..cae2ef154 100644
> --- a/evdev.c
> +++ b/evdev.c
> @@ -30,6 +30,7 @@
>  #include "defs.h"
>
>  #include "xlat/evdev_abs.h"
> +#include "xlat/evdev_ev.h"
>
>  #ifdef HAVE_LINUX_INPUT_H
>
> @@ -47,7 +48,6 @@
>  # include "xlat/evdev_relative_axes.h"
>  # include "xlat/evdev_snd.h"
>  # include "xlat/evdev_switch.h"
> -# include "xlat/evdev_sync.h"
>
>  # ifndef SYN_MAX
>  #  define SYN_MAX 0xf
> @@ -258,9 +258,9 @@ bit_ioctl(struct tcb *const tcp, const unsigned int
> ev_nr,
>           const kernel_ulong_t arg)
>  {
>         switch (ev_nr) {
> -               case EV_SYN:
> -                       return decode_bitset(tcp, arg, evdev_sync,
> -                                            SYN_MAX, "SYN_???",
> XT_INDEXED);
> +               case 0:
> +                       return decode_bitset(tcp, arg, evdev_ev,
> +                                            EV_MAX, "EV_???", XT_SORTED);
>                 case EV_KEY:
>                         return decode_bitset(tcp, arg, evdev_keycode,
>                                              KEY_MAX, "KEY_???",
> XT_INDEXED);
> diff --git a/ioctl.c b/ioctl.c
> index 93fb52631..66b10ec4c 100644
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -33,8 +33,6 @@
>  #include <linux/ioctl.h>
>  #include "xlat/ioctl_dirs.h"
>
> -#include "xlat/evdev_ev.h"
> -
>  static int
>  compare(const void *a, const void *b)
>  {
> @@ -99,7 +97,10 @@ evdev_decode_number(const unsigned int code)
>
>         if (nr >= 0x20 && nr <= 0x20 + 0x1f) {
>                 tprints("EVIOCGBIT(");
> -               printxval(evdev_ev, nr - 0x20, "EV_???");
> +               if (nr == 0x20)
> +                       tprintf("0");
> +               else
> +                       printxval(evdev_ev, nr - 0x20, "EV_???");
>
BTW, a few days ago eSyr said in the mail thread that this part doesn't go
well with the concept of xlat styles. Should I modified xlat/evdev_ev.in
instead? But simply changing EV_SYN to 0 won't work because 0 isn't an
identifier which can be used as a macro name. Any possible solution that I
can check?

>                 tprintf(", %u)", _IOC_SIZE(code));
>                 return 1;
>         } else if (nr >= 0x40 && nr <= 0x40 + 0x3f) {
> diff --git a/tests/ioctl_evdev.c b/tests/ioctl_evdev.c
> index 5eacac024..071b6b0dd 100644
> --- a/tests/ioctl_evdev.c
> +++ b/tests/ioctl_evdev.c
> @@ -138,7 +138,7 @@ main(void)
>         TEST_NULL_ARG_EX(EVIOCGABS(0x3f), "EVIOCGABS(0x3f /* ABS_??? */)");
>         TEST_NULL_ARG_EX(EVIOCSABS(0x3f), "EVIOCSABS(0x3f /* ABS_??? */)");
>
> -       TEST_NULL_ARG(EVIOCGBIT(EV_SYN, 0));
> +       TEST_NULL_ARG(EVIOCGBIT(0, 0));
>         TEST_NULL_ARG(EVIOCGBIT(EV_KEY, 1));
>         TEST_NULL_ARG(EVIOCGBIT(EV_REL, 2));
>         TEST_NULL_ARG(EVIOCGBIT(EV_ABS, 3));
> diff --git a/xlat/evdev_sync.in b/xlat/evdev_sync.in
> deleted file mode 100644
> index ca9ea5035..000000000
> --- a/xlat/evdev_sync.in
> +++ /dev/null
> @@ -1,5 +0,0 @@
> -#value_indexed
> -SYN_REPORT     0
> -SYN_CONFIG     1
> -SYN_MT_REPORT  2
> -SYN_DROPPED    3
>
> What do you think?
>
>
> --
> ldv
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20180819/3377fd6d/attachment.html>