[PATCH libevdev 3/5] Add libevdev_event_type/code_from_name() resolvers

[Peter Hutterer]

On Fri, Oct 04, 2013 at 08:37:33PM +0200, David Herrmann wrote:
> Two new helpers are added:
> (1) libevdev_event_type_from_name() takes a string describing an EV_*
> event type and returns the given event-type constant.
> (2) libevdev_event_code_from_name() takes a string describing an event
> code and returns the given event-code constant.
> 
> Both functions share common behavior:
>  - Return -1 if the string is not found.
>  - Accept a string-length argument (if <0 zero-terminated string is
>    assumed). This is highly useful for lexers. If you mmap() a file it is
>    usually read-only and a lexer cannot assume an identifier is
>    zero-terminated. So without an explicit size-argument, a lexer would
>    have to copy the string before passing it to the resolver.
>  - Accept a flags argument. Two flags exist: One flag to make the resolver
>    case-sensitive (case-insensitive is the default). The other flag allows

what's the use-case of having case-sensitive matching? these defines are always
uppercase and I doubt that'll change soon. the only reason for
case-sensitive matching I can see is if the caller doesn't quite know if
something is an event code or not and want's to decide based on the case
(e.g. "trigger" in a text isn't BTN_TRIGGER). and that's more likely to go
wrong than right anyway ("a" vs "A").

>    the caller to force the resolver to fail if the code prefix is omitted.
>    By default, the resolver allows omitting prefixes like "EV_", "KEY_"
>    and so on to simplify configuration files where this is obvious,
>    anyway. However, this might be ambigious or misleading in some
>    use-cases so this flag allows to disable the prefix-guessing.
> 
> libevdev_event_code_from_name() has one additional argument which
> specifies the type/group of the code. If it is -1, the resolver tries to
> deduce it from the string prefix. Otherwise, only the given group is
> searched.

I'm not a big fan of the API just yet, tbh. I'd set the DEFAULT flags enum
to set a specific value so that 0 is an invalid value.
and a define/enum for type unset would be better, because right now this is
a valid call:
   libevdev_event_code_from_name("A", -1, -1, 0);
and quite frankly, that's not easy to read. A better approach is something
like this:
   libevdev_event_code_from_name("A", -1, LIBEVDEV_GUESS_TYPE,
                                 LIBEVDEV_RESOLVER_FLAGS_DEFAULT);
still leaves the -1 for the size, but defining that to be some constant
seems over the top.

> Another special behavior is BTN_* parsing. The BTN_* constants may overlap
> with KEY_* constants and thus, if the prefix is omitted, the parser cannot
> know which to return. We explicitly prefer KEY_* codes in that case so
> users have to add the prefix explicitly if they want the BTN_* code of a
> conflicting EV_KEY code.

I don't think this behaviouri is a good idea, it's likely to produce the
wrong result when people don't check the exact behaviour and a lookup
failure for BTN_A is a lot easier to debug than a wrong code because
we returned KEY_A instead.

better not to allow it and require users to use BTN_ as prefix for button
lookups.
 
> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
> ---
>  libevdev/Makefile.am      |   1 +
>  libevdev/libevdev-names.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++
>  libevdev/libevdev.h       |  55 +++++++++++++
>  test/Makefile.am          |   1 +
>  4 files changed, 251 insertions(+)
>  create mode 100644 libevdev/libevdev-names.c
> 
> diff --git a/libevdev/Makefile.am b/libevdev/Makefile.am
> index 552df70..861be66 100644
> --- a/libevdev/Makefile.am
> +++ b/libevdev/Makefile.am
> @@ -9,6 +9,7 @@ libevdev_la_SOURCES = \
>                     libevdev-uinput.c \
>                     libevdev-uinput.h \
>                     libevdev-uinput-int.h \
> +                   libevdev-names.c \
>                     libevdev.c
>  
>  libevdev_la_LDFLAGS = \
> diff --git a/libevdev/libevdev-names.c b/libevdev/libevdev-names.c
> new file mode 100644
> index 0000000..ad63d64
> --- /dev/null
> +++ b/libevdev/libevdev-names.c
> @@ -0,0 +1,194 @@
> +/*
> + * Copyright © 2013 David Herrmann <dh.herrmann at gmail.com>
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission.  The copyright holders make no representations
> + * about the suitability of this software for any purpose.  It is provided "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#include <config.h>
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <strings.h>
> +
> +#include "libevdev.h"
> +#include "libevdev-int.h"
> +#include "libevdev-util.h"
> +#include "event-names.h"
> +
> +struct name_lookup {
> +	const char *name;
> +	size_t len;

this should be ssize_t if you're using -1 as value.

> +};
> +
> +static int cmp_entry_case(const void *vlookup, const void *ventry)
> +{
> +	const struct name_lookup *lookup = vlookup;
> +	const struct name_entry *entry = ventry;
> +	int r;
> +
> +	r = strncasecmp(lookup->name, entry->name, lookup->len);
> +	if (!r) {
> +		if (entry->name[lookup->len])
> +			r = -1;
> +		else
> +			r = 0;

r is already 0 here, this can be compressed.

> +	}
> +
> +	return r;
> +}
> +
> +static bool
> +fill_lookup(struct name_lookup *lookup, const char *name, ssize_t len,
> +	    const char *prefix, size_t plen, enum libevdev_resolver_flag flags)
> +{
> +	lookup->name = name;
> +	lookup->len = (len < 0) ? strlen(name) : (size_t)len;
> +
> +	if (startswith_case(lookup->name, lookup->len, prefix, plen)) {
> +		if (!(flags & LIBEVDEV_RESOLVER_FLAG_CASE_SENSITIVE) ||
> +		    startswith(lookup->name, lookup->len, prefix, plen)) {
> +			lookup->name = &lookup->name[plen];
> +			lookup->len -= plen;
> +			return true;
> +		}
> +	}
> +
> +	return !(flags & LIBEVDEV_RESOLVER_FLAG_REQUIRE_PREFIX);
> +}
> +
> +static const struct name_entry*
> +lookup_name(const struct name_entry *array, size_t asize,
> +	    struct name_lookup *lookup, enum libevdev_resolver_flag flags)
> +{
> +	const struct name_entry *entry;
> +
> +	entry = bsearch(lookup, array, asize, sizeof(*array), cmp_entry_case);
> +	if (!entry)
> +		return NULL;
> +
> +	if (flags & LIBEVDEV_RESOLVER_FLAG_CASE_SENSITIVE)
> +		if (strncmp(entry->name, lookup->name, lookup->len))
> +			return NULL;
> +
> +	return entry;
> +}
> +
> +LIBEVDEV_EXPORT int
> +libevdev_event_type_from_name(const char *name, ssize_t len,
> +			      enum libevdev_resolver_flag flags)
> +{
> +	struct name_lookup lookup;
> +	const struct name_entry *entry;
> +
> +	if (!fill_lookup(&lookup, name, len, "EV_", 3, flags))
> +		return -1;
> +
> +	entry = lookup_name(ev_names, ARRAY_LENGTH(ev_names), &lookup, flags);
> +
> +	return entry ? (int)entry->value : -1;
> +}
> +
> +LIBEVDEV_EXPORT int
> +libevdev_event_code_from_name(const char *name, ssize_t len, int type,
> +			      enum libevdev_resolver_flag flags)
> +{
> +	struct name_lookup lookup;
> +	const struct name_entry *entry, *table;
> +	size_t table_size, i;
> +	bool try_btn = false;
> +
> +	len = (len < 0) ? strlen(name) : (size_t)len;
> +
> +	if (type < 0) {
> +		for (i = 0; i < EV_MAX + 1; ++i) {
> +			if (!ev_map[i])
> +				continue;
> +			if (startswith_case(name, len, &ev_map[i][3],
> +					    strlen(&ev_map[i][3]))) {
> +				type = i;
> +				break;
> +			}
> +		}
> +
> +		if (startswith_case(name, len, "BTN_", 4))
> +			type = EV_KEY;
> +
> +		if (type < 0)
> +			return -1;
> +	}
> +
> +#define TYPE_ENTRY(_code, _prefix, _plen, _array) \
> +	case _code: \
> +		if (!fill_lookup(&lookup, name, len, _prefix, _plen, flags)) \
> +			return -1; \
> +		table = _array; \
> +		table_size = ARRAY_LENGTH(_array); \
> +		break
> +
> +	switch (type) {
> +		TYPE_ENTRY(EV_SYN, "SYN_", 4, syn_names);
> +		TYPE_ENTRY(EV_REL, "REL_", 4, rel_names);
> +		TYPE_ENTRY(EV_ABS, "ABS_", 4, abs_names);
> +		TYPE_ENTRY(EV_MSC, "MSC_", 4, msc_names);
> +		TYPE_ENTRY(EV_SND, "SND_", 4, snd_names);
> +		TYPE_ENTRY(EV_SW,  "SW_",  3, sw_names);
> +		TYPE_ENTRY(EV_LED, "LED_", 4, led_names);
> +		TYPE_ENTRY(EV_REP, "REP_", 4, rep_names);
> +		TYPE_ENTRY(EV_FF,  "FF_",  3, ff_names);
> +	case EV_KEY:
> +		table = key_names;
> +		table_size = ARRAY_LENGTH(key_names);
> +		lookup.name = name;
> +		lookup.len = len;
> +
> +		if (startswith_case(name, len, "KEY_", 4)) {
> +			if (!(flags & LIBEVDEV_RESOLVER_FLAG_CASE_SENSITIVE) ||
> +			    startswith(name, len, "KEY_", 4)) {
> +				lookup.name = &name[4];
> +				lookup.len = len - 4;
> +				break;
> +			}
> +		} else if (startswith_case(name, len, "BTN_", 4)) {
> +			if (!(flags & LIBEVDEV_RESOLVER_FLAG_CASE_SENSITIVE) ||
> +			    startswith(name, len, "BTN_", 4)) {
> +				lookup.name = &name[4];
> +				lookup.len = len - 4;
> +				try_btn = true;
> +				break;
> +			}
> +		}
> +
> +		if (flags & LIBEVDEV_RESOLVER_FLAG_REQUIRE_PREFIX)
> +			return -1;
> +
> +		try_btn = true;
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +#undef TYPE_ENTRY
> +
> +	entry = lookup_name(table, table_size, &lookup, flags);
> +	if (!entry && try_btn)
> +		entry = lookup_name(btn_names, ARRAY_LENGTH(btn_names),
> +				    &lookup, flags);
> +
> +	return entry ? (int)entry->value : -1;
> +}
> diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> index 22d1e3d..07d9f7d 100644
> --- a/libevdev/libevdev.h
> +++ b/libevdev/libevdev.h
> @@ -1356,6 +1356,7 @@ int libevdev_event_is_code(const struct input_event *ev, unsigned int type, unsi
>   * defines for new properties libevdev will not automatically pick these up.
>   */
>  const char * libevdev_event_type_get_name(unsigned int type);
> +
>  /**
>   * @ingroup misc
>   *
> @@ -1400,6 +1401,60 @@ const char* libevdev_property_get_name(unsigned int prop);
>  int libevdev_event_type_get_max(unsigned int type);
>  
>  /**
> + * @ingroup misc
> + */
> +enum libevdev_resolver_flag {
> +	/** Default behavior for name resolution (no flags passed). */
> +	LIBEVDEV_RESOLVER_FLAG_DEFAULT			= 0,
> +	/** Require the resolver to compare names case sensitive */
> +	LIBEVDEV_RESOLVER_FLAG_CASE_SENSITIVE		= 1,
> +	/** Require the common prefix on the passed string */
> +	LIBEVDEV_RESOLVER_FLAG_REQUIRE_PREFIX		= 2,
> +};

I'd use LIBEVDEV_NAME_* since there is no use of "resolve" in the function
names themselves.

> +
> +/**
> + * @ingroup misc
> + *

can we have a one or two sentence summary of what the function does here? As
simple "convert from a string representation of an event type into the
matching type" would be enough.

> + * @param name A non-NULL string describing an input-event type ("EV_KEY",
> + * "EV_ABS", ...). The leading "EV_" prefix may be omitted.
> + * @param len The length of the passed string excluding any terminating 0
> + * character. If <0 the string is assumed to be zero-terminated.

"less than zero"

> + * @param flags A list of flags to control the name resolution.
> + *
> + * @return The given type constant for the passed name or -1 if not found.
> + *
> + * @note EV_MAX is also recognized.
> + */
> +int libevdev_event_type_from_name(const char *name, ssize_t len,
> +				  enum libevdev_resolver_flag flags);
> +
> +/**
> + * @ingroup misc

summary here too please.

> + *
> + * @param name A non-NULL string describing an input-event code (KEY_A,
> + * ABS_X, BTN_Y, ...). The leading "XYZ_" prefix may be omitted.
> + * @param len The length of the passed string excluding any terminating 0
> + * character. If <0 the string is assumed to be zero-terminated.
> + * @param type The EV_XYZ input event type to specify the group to search. If
> + * <0 then the prefix of @name is used to detect the type. If the prefix is not

this needs to be @p name, for the proper highlighting.

> + * specified, this will fail.
> + * @param flags A list of flags to control the name resolution.
> + *
> + * @return The given code constant for the passed name or -1 if not found.
> + *
> + * @note *_MAX codes are also recognized. Supported types currently are SYN,
> + * KEY, REL, ABS, MSC, SND, SW, LED, REP, FF. Note that BTN_* codes belong to
> + * the EV_KEY group.
> + *
> + * @note If you specify EV_KEY as type and omit the prefix of your name, this
> + * will first try to match against KEY_* names and then against BTN_* names.
> + * Hence, if your name exists in both groups, the KEY_* name is returned. You
> + * have to include the prefix if you pass ambigious names.

> + */
> +int libevdev_event_code_from_name(const char *name, ssize_t len, int type,
> +				  enum libevdev_resolver_flag flags);

all the *event_code* functions have the arguments in order type, code,
should be the same here.

Cheers,
   Peter

> +
> +/**
>   * @ingroup bits
>   *
>   * Get the repeat delay and repeat period values for this device.
> diff --git a/test/Makefile.am b/test/Makefile.am
> index 8454329..258af9e 100644
> --- a/test/Makefile.am
> +++ b/test/Makefile.am
> @@ -5,6 +5,7 @@ TESTS = $(noinst_PROGRAMS)
>  
>  libevdev_sources = $(top_srcdir)/libevdev/libevdev.c \
>  		   $(top_srcdir)/libevdev/libevdev.h \
> +		   $(top_srcdir)/libevdev/libevdev-names.c \
>  		   $(top_srcdir)/libevdev/libevdev-uinput.h \
>  		   $(top_srcdir)/libevdev/libevdev-uinput.c \
>  		   $(top_srcdir)/libevdev/libevdev-uinput-int.h \
> -- 
> 1.8.4
>