On Thu, Jul 14, 2011 at 05:53:17PM +0100, Daniel Drake wrote:
> Add functionality to query evdev state of a specific key, switch, button,
> LED or sound event. This is useful in programs such as powerd
> (http://wiki.laptop.org/go/Powerd) which need to query things like the
> state of the laptop lid switch from shell code.
>
> Original monitor-mode functionality is left unchanged and is still
> activated by default. New usage modes are explained in the man page.
>
> Signed-off-by: Daniel Drake <[email protected]>
Merged patch 1/2, thanks.
For this one, a few comments:
evtest.c: In function ‘get_keycode’:
evtest.c:541:2: warning: enumeration value ‘MODE_GRAB’ not handled in switch
[-Wswitch]
evtest.c: In function ‘do_query’:
evtest.c:856:2: warning: enumeration value ‘MODE_GRAB’ not handled in switch
[-Wswitch]
> ---
> evtest.c | 267
> ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> evtest.txt | 31 ++++++-
> 2 files changed, 257 insertions(+), 41 deletions(-)
>
> diff --git a/evtest.c b/evtest.c
> index 5cc8505..39e403f 100644
> --- a/evtest.c
> +++ b/evtest.c
> @@ -49,6 +49,8 @@
> #include <stdlib.h>
> #include <dirent.h>
> #include <errno.h>
> +#include <getopt.h>
> +#include <ctype.h>
>
> #define BITS_PER_LONG (sizeof(long) * 8)
> #define NBITS(x) ((((x)-1)/BITS_PER_LONG)+1)
> @@ -56,6 +58,7 @@
> #define BIT(x) (1UL<<OFF(x))
> #define LONG(x) ((x)/BITS_PER_LONG)
> #define test_bit(bit, array) ((array[LONG(bit)] >> OFF(bit)) & 1)
> +#define ARRAY_SIZE(i) (sizeof(i) / sizeof(*i))
>
> #define DEV_INPUT_EVENT "/dev/input"
> #define EVENT_DEV_NAME "event"
> @@ -69,6 +72,14 @@
>
> #define NAME_ELEMENT(element) [element] = #element
>
> +enum evtest_mode {
> + MODE_GRAB,
probaby better to name it MODE_DEFAULT, or MODE_PRINT (see my comments on
do_grab)
> + MODE_QUERY_SW,
> + MODE_QUERY_KEY,
> + MODE_QUERY_LED,
> + MODE_QUERY_SND,
> +};
> +
> static const char * const events[EV_MAX + 1] = {
> [0 ... EV_MAX] = NULL,
> NAME_ELEMENT(EV_SYN), NAME_ELEMENT(EV_KEY),
> @@ -480,6 +491,68 @@ static const char * const * const names[EV_MAX + 1] = {
> };
>
> /**
> + * Search for the code of a specific key/snd/led/sw bit, based on its textual
> + * representation (e.g. SW_DOCK).
> + *
> + * @param type The EV_ event type being queried
> + * @param kstr The string to parse and convert
> + *
> + * @return The requested code's numerical value, or negative on error.
> + */
> +static int key_name_to_code(int type, int max, const char *kstr)
> +{
> + const char * const *keynames = names[type];
> + int i;
> +
> + for (i = 0; i < max; i++) {
> + const char *name = keynames[i];
> + if (!name)
> + continue;
> + if (strcasecmp(name, kstr) == 0)
> + return i;
please instead use:
if (name && strcasecmp() == 0)
...
> + }
> +
> + return -1;
> +}
> +
> +/**
> + * Convert a string to a specific key/snd/led/sw code. The string can either
> + * be the name of the key in question (e.g. "SW_CODE") or the numerical
> + * value, either as decimal (e.g. "5") or as hex (e.g. "0x5").
> + *
> + * @param mode The mode being queried (key, snd, led, sw)
> + * @param kstr The string to parse and convert
> + *
> + * @return The requested code's numerical value, or negative on error.
> + */
> +static int get_keycode(enum evtest_mode mode, const char *kstr)
> +{
> + if (isdigit(kstr[0])) {
> + unsigned long val;
> + errno = 0;
> + val = strtoul(kstr, NULL, 0);
> + if (errno) {
> + fprintf(stderr, "Could not interpret value %s\n", kstr);
> + return -1;
> + }
> + return (int) val;
> + }
> +
> + switch (mode) {
> + case MODE_QUERY_SW:
> + return key_name_to_code(EV_SW, SW_CNT, kstr);
> + case MODE_QUERY_KEY:
> + return key_name_to_code(EV_KEY, KEY_CNT, kstr);
> + case MODE_QUERY_SND:
> + return key_name_to_code(EV_SND, SND_CNT, kstr);
> + case MODE_QUERY_LED:
> + return key_name_to_code(EV_LED, LED_CNT, kstr);
> + }
> +
> + return -1;
> +}
> +
> +/**
> * Filter for the AutoDevProbe scandir on /dev/input.
> *
> * @param dir The current directory entry provided by scandir.
> @@ -544,38 +617,26 @@ static char* scan_devices(void)
> /**
> * Print usage information.
> */
> -static void usage(void)
> +static void __attribute__((noreturn)) usage(void)
> {
> - printf("Usage: evtest /dev/input/eventX\n");
> - printf("Where X = input device number\n");
> -}
> -
> -/**
> - * Parse the commandline arguments.
> - *
> - * @return The filename of the device file to open, or NULL in case of
> - * error. This string is allocated and must be freed by the caller.
> - */
> -static char* parse_args(int argc, char **argv)
> -{
> - char *filename;
> -
> - if (argc < 2) {
> - fprintf(stderr, "No device specified, trying to scan all of
> %s/%s*\n",
> - DEV_INPUT_EVENT, EVENT_DEV_NAME);
> -
> - if (getuid() != 0)
> - fprintf(stderr, "Not running as root, no devices may be
> available.\n");
> -
> - filename = scan_devices();
> - if (!filename) {
> - usage();
> - return NULL;
> - }
> - } else
> - filename = strdup(argv[argc - 1]);
> -
> - return filename;
> + printf("USAGE:\n");
> + printf(" Grab mode:\n");
> + printf(" %s /dev/input/eventX\n", program_invocation_short_name);
heh. thanks, hadn't heard of program_invocation_short_name yet.
> + printf("\n");
> + printf(" Query mode: (check exit code)\n");
> + printf(" %s --query-sw <value> /dev/input/eventX\n",
I'd prefer the equiv of "evtest --query EV_SW SW_DOCK rather than
having separate --query-sw, --query-key, etc.
> + program_invocation_short_name);
> + printf(" %s --query-key <value> /dev/input/eventX\n",
> + program_invocation_short_name);
> + printf(" %s --query-led <value> /dev/input/eventX\n",
> + program_invocation_short_name);
> + printf(" %s --query-snd <value> /dev/input/eventX\n",
> + program_invocation_short_name);
> +
> + printf("\n");
> + printf("<value> can either be a numerical value, or the textual name of
> the\n");
> + printf("key/switch/LED/sound being queried (e.g. SW_DOCK).\n");
> + _exit(EXIT_FAILURE);
why not just return 1 and change the caller to "return usage()"?
does _exit() give us any real benefits here?
> }
>
> /**
> @@ -708,17 +769,42 @@ static int test_grab(int fd)
> return rc;
> }
>
> -int main (int argc, char **argv)
> +static const struct option long_options[] = {
> + { "query-sw", required_argument, NULL, MODE_QUERY_SW },
> + { "query-snd", required_argument, NULL, MODE_QUERY_SND },
> + { "query-key", required_argument, NULL, MODE_QUERY_KEY },
> + { "query-led", required_argument, NULL, MODE_QUERY_LED },
> + { 0, },
> +};
> +
> +/**
> + * Enter grab mode. The requested event device will be grabbed and any
> + * captured events will be decoded and printed on the console.
> + *
> + * @param device The device to grab, or NULL if the user should be prompted.
> + * @return 0 on success, non-zero on error.
> + */
> +static int do_grab(const char *device)
this isn't quite correct. if you check the code again, you'll see that all
we do here is to grab and immediately ungrab the device. Simple reason: I
got too many logs that lacked any events, so I naïvely thought that might
stop users from doing that. so "do_grab" is a bit of a misnomer.
In regards to the patch, I'd prefer it if you can split this out.
Re-indentations are notoriously bad to review. Having one patch that just
moves this code into a separate function (i.e. _just_ moving + whitespace
changes) and then your patch on top would make it much less confusing to see
what you added.
> {
> int fd;
> char *filename;
>
> - filename = parse_args(argc, argv);
> + if (device) {
> + filename = strdup(device);
> + } else {
> + fprintf(stderr, "No device specified, trying to scan all of
> %s/%s*\n",
> + DEV_INPUT_EVENT, EVENT_DEV_NAME);
>
> - if (!filename)
> - return 1;
> + if (getuid() != 0)
> + fprintf(stderr, "Not running as root, no devices may be
> available.\n");
>
> - if ((fd = open(filename, O_RDONLY)) < 0) {
> + filename = scan_devices();
> + if (!filename)
> + usage();
> + }
> +
> + fd = open(filename, O_RDONLY);
> + if (fd < 0) {
> perror("evtest");
> if (errno == EACCES && getuid() != 0)
> fprintf(stderr, "You do not have access to %s. Try "
> @@ -751,4 +837,113 @@ int main (int argc, char **argv)
> return print_events(fd);
> }
>
> +/**
> + * Enter query mode. The requested event device will be queried for the state
> + * of a particular switch/key/sound/LED.
> + *
> + * @param device The device to query.
> + * @param mode The mode (event type) that is to be queried (snd, sw, key,
> led)
> + * @param keycode The key code to query the state of.
> + * @return 0 if the state bit is unset, 10 if the state bit is set.
> + */
> +static int do_query(const char *device, enum evtest_mode mode, int keycode)
> +{
> + int rq;
> + int max;
> + int fd;
> + int r;
> +
> + switch (mode) {
> + case MODE_QUERY_SW:
> + max = SW_MAX;
> + rq = EVIOCGSW(SW_MAX);
> + break;
> + case MODE_QUERY_KEY:
> + max = KEY_MAX;
> + rq = EVIOCGKEY(KEY_MAX);
> + break;
> + case MODE_QUERY_SND:
> + max = SND_MAX;
> + rq = EVIOCGSND(SND_MAX);
> + break;
> + case MODE_QUERY_LED:
> + max = SND_MAX;
> + rq = EVIOCGLED(LED_MAX);
> + break;
> + }
> +
> + if (keycode > max) {
> + fprintf(stderr, "Key 0x%x out of bounds.\n", keycode);
> + _exit(EXIT_FAILURE);
> + }
> +
> + unsigned char state[NBITS(max)];
move this up to the beginning of the function please.
for this function as well, is the _exit() really beneficial over a simple
return EXIT_FAILURE?
> +
> + fd = open(device, O_RDONLY);
> + if (fd < 0) {
> + perror("open");
> + _exit(EXIT_FAILURE);
> + }
> +
> + memset(state, 0, sizeof(state));
> + r = ioctl(fd, rq, state);
> + close(fd);
> +
> + if (r == -1) {
> + perror("ioctl");
> + _exit(EXIT_FAILURE);
> + }
> +
> + if (test_bit(keycode, state))
> + return 10; /* different from EXIT_FAILURE */
> + else
> + return 0;
> +}
> +
> +int main (int argc, char **argv)
> +{
> + const char *device = NULL;
> + const char *keyname;
> + enum evtest_mode mode = MODE_GRAB;
> + int keycode;
> +
> + while (1) {
> + int option_index = 0;
> + int c = getopt_long(argc, argv, "", long_options,
> &option_index);
> + if (c == -1)
> + break;
> + switch (c) {
> + case MODE_QUERY_SW:
> + case MODE_QUERY_KEY:
> + case MODE_QUERY_LED:
> + case MODE_QUERY_SND:
> + mode = c;
> + keyname = optarg;
> + keycode = get_keycode(mode, keyname);
> + break;
> + default:
> + usage();
> + break;
> + }
> + }
> +
> + if (optind < argc)
> + device = argv[optind++];
> +
> + if (mode == MODE_GRAB)
> + return do_grab(device);
> +
> + if (keycode < 0) {
> + fprintf(stderr, "Unrecognised key name: %s\n", keyname);
> + usage();
> + }
> +
> + if (!device) {
> + fprintf(stderr, "Device argument is required for query.\n");
> + usage();
> + }
> +
> + return do_query(device, mode, keycode);
> +}
> +
> /* vim: set noexpandtab tabstop=8 shiftwidth=8: */
> diff --git a/evtest.txt b/evtest.txt
> index 685a4de..f255edd 100644
> --- a/evtest.txt
> +++ b/evtest.txt
> @@ -4,17 +4,31 @@ EVTEST(1)
> NAME
> ----
>
> - evtest - Input device event monitor
> + evtest - Input device event monitor and query tool
>
> SYNOPSIS
> --------
> - evtest "/dev/input/eventX"
> + evtest /dev/input/eventX
> +
> + evtest --query-sw <value> /dev/input/eventX
> + evtest --query-key <value> /dev/input/eventX
> + evtest --query-snd <value> /dev/input/eventX
> + evtest --query-led <value> /dev/input/eventX
>
> DESCRIPTION
> -----------
> -evtest displays information on the input device specified on the command
> -line, including all the events supported by the device. It then monitors the
> -device and displays all the events layer events generated.
> +The first invocation type displayed above causes evtest to display
> +information about the specified input device, including all the events
> +supported by the device. It then monitors the device and displays all the
> +events layer events generated.
> +
> +In the other 4 invocation types, evtest performs a one-shot query of the
> state
> +of a specific switch (sw), key, sound (snd) or LED, specified by *value*. If
> +the state bit is set (key pressed, switch on, ...), evtest exits with
> +code 0. If the state bit is unset (key depressed, switch off, ...), evtest
> +exits with code 10. No other output is generated. *value* can be either a
> +decimal representation (e.g. 44), hex (e.g. 0x2c), or the constant name (e.g.
> +KEY_Z).
>
> evtest needs to be able to read from the device; in most cases this means it
> must be run as root.
> @@ -32,6 +46,13 @@ when debugging a synaptics device from within X. VT
> switching to a TTY or
> shutting down the X server terminates this grab and synaptics devices can be
> debugged.
>
> +EXIT CODE
> +---------
> +evtest returns 1 on error.
> +
> +When used to query state, evtest returns 0 if the state bit is unset and
> +11 if the state bit is set.
s/11/10/
Cheers,
Peter
> +
> SEE ALSO
> --------
> inputattach(1)
> --
> 1.7.6
>
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
