[PATCH 1/3 v2'] xserver: Use lists and constants for matching modes in Match entries

[Dan Nicholson]

On Fri, Jun 17, 2011 at 6:53 AM, Oleh Nykyforchyn <oleh.nyk at gmail.com> wrote:
> Further polishing, commit message is the same. Comments, please.
>
> xserver: Use lists and constants for matching modes in Match entries
>
> Current implementation of xf86MatchGroup relies on the assumption that
> mode of matching of an input device attribute against a pattern is
> uniquely determined by the type on an attribute (product name, vendor etc).
> To combine different modes in one Match entry, we need to extend a pattern,
> which is char*, with a field that keeps a matching mode. In this aspect,
> this patch changes nothing in functionality and is preparatory for next
> changes.
>
> We also introduce an optional possibility to write a Match entry of the form
>    MatchProduct "foo|bar|anything|else"
> also in the forms
>    MatchProduct "foo" "bar" "anything" "else"
> or
>    MatchProduct "foo|bar" "anything|else"
> etc, which is useful for introduction of regular expressions in these statements.
>
> Signed-off-by: Oleh Nykyforchyn <oleh.nyk at gmail.com>

I'm not sure what to do here. I like the refactoring with the
enumerated match types. However, I have a few issues with this.

1. The commit subject talks about using lists and constants, but then
there's other stuff like using multiple arguments and changing the
parser that are unrelated. The "(implicit)" bugfix is completely
unrelated and should be in a separate commit if it's even needed at
all (see below).

2. I still don't think anyone is clamoring to have multiple arguments
when the same thing is allowed with '|'. People have been getting
along pretty well so far without them. I'd rather keep it simple and
just have one method to specify multiple values on one line.
Furthermore, the implementation causes you to have lists within lists
and makes the terminology (patterns vs. groups) even more difficult to
understand than it is now.

You've clearly spent a lot of time working on this, and there's some
good stuff here, so I hope we can find a way to land it.

> ---
>  hw/xfree86/common/xf86Xinput.c |  122 ++++++++-------
>  hw/xfree86/man/xorg.conf.man   |   31 +++-
>  hw/xfree86/parser/InputClass.c |  319 +++++++++++++++++++++++++---------------
>  hw/xfree86/parser/xf86Parser.h |   22 +++-
>  4 files changed, 310 insertions(+), 184 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
> index 26051ad..22e58a5 100644
> --- a/hw/xfree86/common/xf86Xinput.c
> +++ b/hw/xfree86/common/xf86Xinput.c
> @@ -443,77 +443,77 @@ HostOS(void)
>  #endif
>  }
>
> +/*
> + * Match an attribute against a pattern. Matching mode is
> + * determined by pattern->mode member.
> + */
>  static int
> -match_substring(const char *attr, const char *pattern)
> +match_token(const char *attr, xf86MatchPattern *pattern)
>  {
> -    return (strstr(attr, pattern)) ? 0 : -1;
> -}
> +    if (!pattern)
> +        return 0;
>
> +    switch (pattern->mode)
> +    {
> +        case MATCH_IS_INVALID:
> +            return 0;
> +        case MATCH_IS_STRCMP:
> +            return (strcmp(attr, pattern->str)) ? 0 : -1;
> +        case MATCH_IS_STRCASECMP:
> +            return (strcasecmp(attr, pattern->str)) ? 0 : -1;
> +        case MATCH_IS_STRSTR:
> +            return (strstr(attr, pattern->str)) ? -1 : 0;
> +        case MATCH_IS_STRCASESTR:
> +            return (strcasestr(attr, pattern->str)) ? -1 : 0;
>  #ifdef HAVE_FNMATCH_H
> -static int
> -match_pattern(const char *attr, const char *pattern)
> -{
> -    return fnmatch(pattern, attr, 0);
> -}
> +        case MATCH_IS_FILENAME:
> +            return (fnmatch(pattern->str, attr, 0)) ? 0 : -1;
> +        case MATCH_IS_PATHNAME:
> +            return (fnmatch(pattern->str, attr, FNM_PATHNAME)) ? 0 : -1;
>  #else
> -#define match_pattern match_substring
> +        case MATCH_IS_FILENAME:
> +            return (strstr(attr, pattern->str)) ? -1 : 0;
> +        case MATCH_IS_PATHNAME:
> +            return (strstr(attr, pattern->str)) ? -1 : 0;
>  #endif
> -
> -#ifdef HAVE_FNMATCH_H
> -static int
> -match_path_pattern(const char *attr, const char *pattern)
> -{
> -    return fnmatch(pattern, attr, FNM_PATHNAME);
> -}
> -#else
> -#define match_path_pattern match_substring
> -#endif
> -
> -/*
> - * If no Layout section is found, xf86ServerLayout.id becomes "(implicit)"
> - * It is convenient that "" in patterns means "no explicit layout"
> - */
> -static int
> -match_string_implicit(const char *attr, const char *pattern)
> -{
> -    if (strlen(pattern)) {
> -        return strcmp(attr, pattern);
> -    } else {
> -        return strcmp(attr,"(implicit)");
> +        default:
> +        /* Impossible */
> +            return 0;
>     }
>  }
>
>  /*
> - * Match an attribute against a list of NULL terminated arrays of patterns.
> - * If a pattern in each list entry is matched, return TRUE.
> + * Match an attribute against a list of xf86MatchGroup's.
> + * Return TRUE only if each list entry is successful.
>  */
>  static Bool
> -MatchAttrToken(const char *attr, struct list *patterns,
> -               int (*compare)(const char *attr, const char *pattern))
> +MatchAttrToken(const char *attr, struct list *groups)
>  {
> -    const xf86MatchGroup *group;
> +    xf86MatchGroup *group;
> +    xf86MatchPattern *pattern;
>
> -    /* If there are no patterns, accept the match */
> -    if (list_is_empty(patterns))
> +    /* If there are no groups, accept the match */
> +    if (list_is_empty(groups))
>         return TRUE;
>
> -    /* If there are patterns but no attribute, reject the match */
> +    /* If there are groups but no attribute, reject the match */
>     if (!attr)
>         return FALSE;
>
>     /*
> -     * Otherwise, iterate the list of patterns ensuring each entry has a
> -     * match. Each list entry is a separate Match line of the same type.
> +     * Otherwise, iterate the list of groups ensuring each entry has a
> +     * match. Each list entry is a list of patterns obtained from
> +     * a separate Match line.
>      */
> -    list_for_each_entry(group, patterns, entry) {
> -        char * const *cur;
> +    list_for_each_entry(group, groups, entry) {
>         Bool match = FALSE;
>
> -        for (cur = group->values; *cur; cur++)
> -            if ((*compare)(attr, *cur) == 0) {
> +        list_for_each_entry(pattern, &group->patterns, entry) {
> +            if (match_token(attr, pattern)) {
>                 match = TRUE;
>                 break;
>             }
> +        }
>         if (!match)
>             return FALSE;
>     }
> @@ -531,31 +531,31 @@ InputClassMatches(const XF86ConfInputClassPtr iclass, const InputInfoPtr idev,
>                   const InputAttributes *attrs)
>  {
>     /* MatchProduct substring */
> -    if (!MatchAttrToken(attrs->product, &iclass->match_product, match_substring))
> +    if (!MatchAttrToken(attrs->product, &iclass->match_product))
>         return FALSE;
>
>     /* MatchVendor substring */
> -    if (!MatchAttrToken(attrs->vendor, &iclass->match_vendor, match_substring))
> +    if (!MatchAttrToken(attrs->vendor, &iclass->match_vendor))
>         return FALSE;
>
>     /* MatchDevicePath pattern */
> -    if (!MatchAttrToken(attrs->device, &iclass->match_device, match_path_pattern))
> +    if (!MatchAttrToken(attrs->device, &iclass->match_device))
>         return FALSE;
>
>     /* MatchOS case-insensitive string */
> -    if (!MatchAttrToken(HostOS(), &iclass->match_os, strcasecmp))
> +    if (!MatchAttrToken(HostOS(), &iclass->match_os))
>         return FALSE;
>
>     /* MatchPnPID pattern */
> -    if (!MatchAttrToken(attrs->pnp_id, &iclass->match_pnpid, match_pattern))
> +    if (!MatchAttrToken(attrs->pnp_id, &iclass->match_pnpid))
>         return FALSE;
>
>     /* MatchUSBID pattern */
> -    if (!MatchAttrToken(attrs->usb_id, &iclass->match_usbid, match_pattern))
> +    if (!MatchAttrToken(attrs->usb_id, &iclass->match_usbid))
>         return FALSE;
>
>     /* MatchDriver string */
> -    if (!MatchAttrToken(idev->driver, &iclass->match_driver, strcmp))
> +    if (!MatchAttrToken(idev->driver, &iclass->match_driver))
>         return FALSE;
>
>     /*
> @@ -569,7 +569,7 @@ InputClassMatches(const XF86ConfInputClassPtr iclass, const InputInfoPtr idev,
>         if (!attrs->tags)
>             return FALSE;
>         for (tag = attrs->tags, match = FALSE; *tag; tag++) {
> -            if (MatchAttrToken(*tag, &iclass->match_tag, strcmp)) {
> +            if (MatchAttrToken(*tag, &iclass->match_tag)) {
>                 match = TRUE;
>                 break;
>             }
> @@ -578,12 +578,18 @@ InputClassMatches(const XF86ConfInputClassPtr iclass, const InputInfoPtr idev,
>             return FALSE;
>     }
>
> -    /* MatchLayout string */
> -    if (!list_is_empty(&iclass->match_layout)) {
> -        if (!MatchAttrToken(xf86ConfigLayout.id,
> -                            &iclass->match_layout, match_string_implicit))
> +    /* MatchLayout string
> +     *
> +     * If no Layout section is found, xf86ServerLayout.id becomes "(implicit)"
> +     * It is convenient that "" in patterns means "no explicit layout"
> +     */
> +    const char *layout;
> +    if (strcmp(xf86ConfigLayout.id,"(implicit)"))
> +        layout = xf86ConfigLayout.id;
> +    else
> +        layout = "";
> +    if (!MatchAttrToken(layout, &iclass->match_layout))
>             return FALSE;

Later I thought about this more. If you're using MatchLayout, by
definition you're specifying what ServerLayout you want to use. If
you're not using a ServerLayout, then you have no need for
MatchLayout. I think it would be better to just document in the
manpage that if you don't specify a ServerLayout, the name
"(implicit)" is used.

--
Dan