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

[Oleh Nykyforchyn]

On Wed, 8 Jun 2011 10:39:40 -0700
Dan Nicholson <dbn.lists at gmail.com> wrote:

> On Sat, Jun 4, 2011 at 10:48 PM, Oleh Nykyforchyn <oleh.nyk at gmail.com> wrote:
> > Use lists and constants for matching modes in Match entries
> 
> You're leaving out some critical information here. Particularly that
> each Match entry would take multiple arguments.
You are right, but it MAY take multiple arguments, and this possibility
will not be used my most users. Most people simply will not notice
anything and their "foo|bar" will work as before. For others "regex:^USB.*Mouse$"
will be quite sufficient.

My reason to keep this option is the following: sometimes it is necessary
to interleave positive and negated conditions (I have had such an experience
in my multiseat setups, when input devices was to be distributed between seats).
Any of these conditions can be regexes, and RE have no builtin negation, hence
it is locical to accept multiple (negated and non-negated) RE's in a line. With a single
argument, to glue RE's via '|', each '\' must be doubled, and all '|' in RE
must be escaped, which is not very readable and convenient (imagine an explanation
in man page). This option adds no computational overhead and does not make the code
much more complicated. Why should we restrict ourselves to "many simple patterns or
one regex"?

> I'm going to ignore
> those details for now since I don't think we've agreed on it.
But yes, commit messages will be expanded when we agree on details.
> 

> > +/*
> > + * Match an attribute against a pattern. Matching mode is
> > + * determined by pattern->mode member.
> > + */
> >  static int
> > -match_substring(const char *attr, const char *pattern)
> > +multi_match(const char *attr, xf86MatchPattern *pattern)
> 
> I like this function a lot now with the switch, but multi_match may
> not be the best name. Maybe match_token?
OK

> I'm also pretty sure pattern
> could be const.
No, read the third patch. A pattern can contain a regex_t, which is
dynamically allocated before the first use (probably never).

> 
> >  {
> > -    return (strstr(attr, pattern)) ? 0 : -1;
> > -}
> > +    if (!pattern) return 0;
> 
> I'm not sure others feel strongly about this, but I much prefer
> expressions to be on a separate line from the conditional.
No problem
> >
> > -                            &iclass->match_layout, match_string_implicit))
> > +    if (!MatchAttrToken(xf86ConfigLayout.id, &iclass->match_layout))
> >             return FALSE;
> >     }
> 
> 1. This brace appears to be hanging. Did this compile?
Yes, it was a last minute change, which seemed to be obvious and not compiled,
and was wrong.

> 
> 2. The handling of "(implicit)" with the new MATCH_IS_STRIMPLICIT
> seems not very ideal. Instead of having a new type, why don't we just
> sanitize xf86ConfigLayout.id before passing it to MatchAttrToken?
> 
> const char *layout;
> if (strcmp(xf86ConfigLayout.id, "(implicit)") == 0)
>     layout = "";
> else
>     layout = xf86ConfigLayout.id;
> if (!MatchAttrToken(layout, &iclass->match_layout))
>     return FALSE;
> 
> And then MATCH_IS_STRCMP is can be used normally.
OK, nice idea.

> 
> > +    if (!str) return;
> 
> Same comment as above.
OK

> 
> > -    group = malloc(sizeof(*group));
> > -    if (group) {
> > -        group->values = values;
> > -        list_add(&group->entry, head);
> > +    if (!*group) {
> > +        *group = malloc(sizeof(**group));
> > +        if (!*group) return;
> > +        list_init(&(*group)->patterns);
> >     }
> 
> I think it would be a lot cleaner to initialize the group in
> xf86parseInputClassSection instead of magically doing it here.
Multiple arguments: sometimes we start a new group, and sometimes
we add to an existing one.

> 
> > +
> > +  again:
> > +    /* start new pattern */
> > +    if ((pattern = calloc(sizeof(*pattern),1)) == NULL)
> > +        return;
> > +    list_add(&pattern->entry, &(*group)->patterns);
> > +
> > +    pattern->mode = pref_mode;
> > +
> > +    if ((next = index(str, TOKEN_SEP)))
> > +        n = next-str;
> > +    else
> > +        n = strlen(str);
> 
> Why use this index loop when strtok will already do it for you? I'd
> rather not start building a parser here.
Because strtok (as You noticed) gobbles empty tokens, which does not
allow to use "" for "(implicit)" layout. BTW, this superparser is much
shorter that xstrtokenize().
> 
> > +
> > +    if ((pattern->str = strndup(str, n)) == NULL) {
> > +        pattern->mode = MATCH_IS_INVALID;
> 
> If strndup failed, I don't think calling it invalid is correct.
Suggestions? I don't like an idea to exit initialization process. 

> 
> > +    }
> >         case MATCH_PRODUCT:
> > -            if (xf86getSubToken(&(ptr->comment)) != STRING)
> > +            while ((token=xf86getSubTokenWithTab(&(ptr->comment),InputClassTab)) == STRING) {
> 
> Please add spaces around = and after ,. Makes the code much easier to read.
OK
> 
> > +                add_to_group(&group, val.str, MATCH_IS_STRSTR);
> > +            }
> > +            xf86unGetToken(token);
> > +            if (group)
> > +                list_add(&group->entry, &ptr->match_product);
> > +            else
> >                 Error(QUOTE_MSG, "MatchProduct");
> > -            add_group_entry(&ptr->match_product,
> > -                            xstrtokenize(val.str, TOKEN_SEP));
> > -            break;
> > +            continue;
> 
> Why did you change all the breaks to continues? If there was code
> below the switch you'd be skipping it.
Multiple arguments

> 
> Why do you need to add the list of patterns to xf86MatchGroup? It
> seems like all that's needed is to add xf86MatchMode to xf86MatchGroup
> in this patch.
Because further we will have patterns with different modes in one group.


-- 
Oleh Nykyforchyn <oleh.nyk at gmail.com>