[PATCH libevdev 3/5] Add libevdev_event_type/code_from_name() resolvers
David Herrmann
dh.herrmann at gmail.com
Mon Oct 7 04:22:32 PDT 2013
Hi
On Mon, Oct 7, 2013 at 3:17 AM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> 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").
Yepp, that's the use-case I had in mind. Think of a
configuration-option that allows to configure a button-mapping. To
avoid the redundant BTN_ prefix, I support the prefix-less mode. But
to allow users to also support additional options like
"btn-xy-map=off" I want to avoid matching this against some imaginary
BTN_OFF. Yes the application should catch this before passing it to
libevdev, but what if it is supposed to be btn-xy-map=disabled and the
user just wrote the wrong option..
On the other hand, all my code does is to do the case-insensitive
search and then do the strcmp() afterwards. We could actually leave
that to the caller.. I'm fine with both.
>> 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.
You mean like this:
enum libevdev_name_flag {
/** Require the resolver to compare names case sensitive */
LIBEVDEV_NAME_FLAG_CASE_SENSITIVE = 1,
/** Allow skipping the prefix */
LIBEVDEV_NAME_FLAG_GUESS_PREFIX = 2,
/** Default behavior for name resolution (no flags passed). */
LIBEVDEV_NAME_FLAG_DEFAULT =
LIBEVDEV_NAME_FLAG_GUESS_PREFIX,
};
Note that "0" is not invalid. If you want "case-insensitive" but
require a prefix, you would pass 0 now.
> 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.
Ok, string+len is a quite common API, I think that's fine. The
LIBEVDEV_GUESS_TYPE looks good, I can add that.
>> 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.
So only lookup button names if the BTN_ prefix is passed? Hmmm, makes
the whole prefix-less thing mostly useless. But yeah, I get your
point. Will change it.
>> 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.
-1 is handled as strlen() directly in the function-heads so this is always >0.
>> +};
>> +
>> +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.
ups, you're right.
>> + }
>> +
>> + 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.
Yepp, changed.
>> +
>> +/**
>> + * @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.
Added.
>> + * @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"
Fixed.
>> + * @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.
Added.
>> + *
>> + * @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.
Fixed.
>> + * 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.
So you're saying like this:
int libevdev_event_code_from_name(int type, const char *name, ssize_t len,
enum libevdev_resolver_flag flags);
Thanks for the review!
David
More information about the Input-tools
mailing list