On Fri, Dec 11, 2015 at 10:42 AM, Peter Hutterer <[email protected]> wrote: > On Fri, Dec 11, 2015 at 09:58:34AM +0100, Benjamin Tissoires wrote: >> Hi Peter, >> >> On Fri, Dec 11, 2015 at 12:58 AM, Peter Hutterer >> <[email protected]> wrote: >> > InputClass sections use various MatchFoo directives to decide which device >> > to >> > apply to. This usually works fine for specific snippets but has drawbacks >> > for >> > snippets that apply more generally to a multitude of devices. >> > >> > This patch adds a NoMatchFoo directive to negate a match, thus allowing >> > snippets that only apply if a given condition is not set. Specifically, >> > this >> > allows for more flexible fallback driver matching, it is now possible to >> > use a >> > snippet that says "assign driver foo, but only if driver bar wasn't already >> > assigned to it". For example: >> > >> > Section "InputClass" >> > Identifier "libinput for tablets" >> > MatchIsTablet "true" >> > NoMatchDriver "wacom" >> > Driver "libinput" >> > EndSection >> > >> > The above only assigns libinput to tablet devices if wacom isn't already >> > assigned to this device, making it possible to select a specific driver by >> > installing/uninstalling it. >> > >> > Signed-off-by: Peter Hutterer <[email protected]> >> >> Thanks, that was quicker than expected :) >> >> Just 2 nitpicks/questions: >> >> > --- >> > hw/xfree86/common/xf86Xinput.c | 15 +++++---- >> > hw/xfree86/man/xorg.conf.man | 15 +++++++++ >> > hw/xfree86/parser/InputClass.c | 76 >> > ++++++++++++++++++++++++++++++++++++------ >> > hw/xfree86/parser/xf86Parser.h | 1 + >> > hw/xfree86/parser/xf86tokens.h | 12 ++++++- >> > 5 files changed, 102 insertions(+), 17 deletions(-) >> > >> > diff --git a/hw/xfree86/common/xf86Xinput.c >> > b/hw/xfree86/common/xf86Xinput.c >> > index c56a2b9..fead782 100644 >> > --- a/hw/xfree86/common/xf86Xinput.c >> > +++ b/hw/xfree86/common/xf86Xinput.c >> > @@ -540,21 +540,24 @@ MatchAttrToken(const char *attr, struct xorg_list >> > *patterns, >> > if (xorg_list_is_empty(patterns)) >> > return TRUE; >> > >> > - /* If there are patterns but no attribute, reject the match */ >> > - if (!attr) >> > - return FALSE; >> > - >> > /* >> > * Otherwise, iterate the list of patterns ensuring each entry has a >> >> If we remove the previous comment, we should probably remove the >> "Otherwise" here. > > fixed locally, thanks. > >> >> > * match. Each list entry is a separate Match line of the same type. >> > */ >> > xorg_list_for_each_entry(group, patterns, entry) { >> > char *const *cur; >> > - Bool match = FALSE; >> > + Bool is_negated = group->is_negated; >> > + Bool match = is_negated; >> > + >> > + /* If there's a pattern but no attribute, we reject the match for >> > a >> > + * MatchFoo directive, and accept it for a NoMatchFoo directive >> > + */ >> >> I don't follow the logic here. Can you explain why a MatchFoo would be >> rejected while a NoMatchFoo won't? I can't find an example for this >> situation. > > If you have a snippet that says "Match on foo" and you have NULL, we reject > because foo isn't NULL. If you have a snippet that says "Dont match on foo" > and you have NULL, we accept because NULL isn't foo :) > > I hit this case with this snippet: > Section "InputClass" > NoMatchDriver "libinput" > Driver "evdev" > ... > EndSection > > If libinput isn't installed and the udev backend doesn't set the driver > (which it usually doesn't except on some old debian LTS systems), we would > always reject the match and thus never assign a driver. >
Oh I see my mistake now. I thought the 'attr' was the string "libinput" in this example, while it is the currently stored attribute for this particular matching pattern. The patch is then: Reviewed-by: Benjamin Tissoires <[email protected]> Cheers, Benjamin > >> > + if (!attr) >> > + return is_negated; >> > >> > for (cur = group->values; *cur; cur++) >> > if ((*compare) (attr, *cur) == 0) { >> > - match = TRUE; >> > + match = !is_negated; >> > break; >> > } >> > if (!match) >> > diff --git a/hw/xfree86/man/xorg.conf.man b/hw/xfree86/man/xorg.conf.man >> > index 08eb7a9..d794df4 100644 >> > --- a/hw/xfree86/man/xorg.conf.man >> > +++ b/hw/xfree86/man/xorg.conf.man >> > @@ -1092,6 +1092,7 @@ attribute. For example: >> > .B " # either gizmo or gadget >> > .B " MatchProduct \*qexample\*q >> > .B " MatchProduct \*qgizmo|gadget\*q >> > +.B " NoMatchDriver \*qdrivername\*q >> > .I " ..." >> > .B "EndSection" >> > .fi >> > @@ -1160,6 +1161,20 @@ if no named >> > .B ServerLayout >> > sections have been found. >> > .PP >> > +The above directives have equivalents for negative matching with the >> > +.B NoMatchProduct, >> > +.B NoMatchVendor, >> > +.B NoMatchDevicePath, >> > +.B NoMatchOS, >> > +.B NoMatchPnPID, >> > +.B NoMatchUSBID, >> > +.B NoMatchDriver, >> > +.B NoMatchTag, >> > +and >> > +.B NoMatchLayout >> > +directives. These NoMatch directives match if the subsequent match is not >> > +met by the device. >> > +.PP >> > The second type of entry is used to match device types. These entries >> > take a >> > boolean argument similar to >> > .B Option >> > diff --git a/hw/xfree86/parser/InputClass.c >> > b/hw/xfree86/parser/InputClass.c >> > index 2b68aaa..5d3b59c 100644 >> > --- a/hw/xfree86/parser/InputClass.c >> > +++ b/hw/xfree86/parser/InputClass.c >> > @@ -55,6 +55,15 @@ xf86ConfigSymTabRec InputClassTab[] = { >> > {MATCH_IS_TABLET, "matchistablet"}, >> > {MATCH_IS_TOUCHPAD, "matchistouchpad"}, >> > {MATCH_IS_TOUCHSCREEN, "matchistouchscreen"}, >> > + {NOMATCH_PRODUCT, "nomatchproduct"}, >> > + {NOMATCH_VENDOR, "nomatchvendor"}, >> > + {NOMATCH_DEVICE_PATH, "nomatchdevicepath"}, >> > + {NOMATCH_OS, "nomatchos"}, >> > + {NOMATCH_PNPID, "nomatchpnpid"}, >> > + {NOMATCH_USBID, "nomatchusbid"}, >> > + {NOMATCH_DRIVER, "nomatchdriver"}, >> > + {NOMATCH_TAG, "nomatchtag"}, >> > + {NOMATCH_LAYOUT, "nomatchlayout"}, >> > {-1, ""}, >> > }; >> > >> > @@ -138,13 +147,19 @@ xf86freeInputClassList(XF86ConfInputClassPtr ptr) >> > >> > #define TOKEN_SEP "|" >> > >> > +enum MatchType { >> > + MATCH_NORMAL, >> > + MATCH_NEGATED, >> > +}; >> > + >> > static void >> > -add_group_entry(struct xorg_list *head, char **values) >> > +add_group_entry(struct xorg_list *head, char **values, enum MatchType >> > type) >> > { >> > xf86MatchGroup *group; >> > >> > group = malloc(sizeof(*group)); >> > if (group) { >> > + group->is_negated = (type == MATCH_NEGATED); >> > group->values = values; >> > xorg_list_add(&group->entry, head); >> > } >> > @@ -155,6 +170,7 @@ xf86parseInputClassSection(void) >> > { >> > int has_ident = FALSE; >> > int token; >> > + enum MatchType matchtype; >> > >> > parsePrologue(XF86ConfInputClassPtr, XF86ConfInputClassRec) >> > >> > @@ -170,6 +186,8 @@ xf86parseInputClassSection(void) >> > xorg_list_init(&ptr->match_layout); >> > >> > while ((token = xf86getToken(InputClassTab)) != ENDSECTION) { >> > + matchtype = MATCH_NORMAL; >> > + >> > switch (token) { >> > case COMMENT: >> > ptr->comment = xf86addComment(ptr->comment, xf86_lex_val.str); >> > @@ -195,65 +213,103 @@ xf86parseInputClassSection(void) >> > case OPTION: >> > ptr->option_lst = xf86parseOption(ptr->option_lst); >> > break; >> > + case NOMATCH_PRODUCT: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_PRODUCT: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchProduct"); >> > add_group_entry(&ptr->match_product, >> > - xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + xstrtokenize(xf86_lex_val.str, TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > + case NOMATCH_VENDOR: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_VENDOR: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchVendor"); >> > add_group_entry(&ptr->match_vendor, >> > - xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + xstrtokenize(xf86_lex_val.str, TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > + case NOMATCH_DEVICE_PATH: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_DEVICE_PATH: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchDevicePath"); >> > add_group_entry(&ptr->match_device, >> > - xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + xstrtokenize(xf86_lex_val.str, TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > + case NOMATCH_OS: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_OS: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchOS"); >> > - add_group_entry(&ptr->match_os, >> > xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + add_group_entry(&ptr->match_os, xstrtokenize(xf86_lex_val.str, >> > + TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > + case NOMATCH_PNPID: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_PNPID: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchPnPID"); >> > add_group_entry(&ptr->match_pnpid, >> > - xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + xstrtokenize(xf86_lex_val.str, TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > + case NOMATCH_USBID: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_USBID: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchUSBID"); >> > add_group_entry(&ptr->match_usbid, >> > - xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + xstrtokenize(xf86_lex_val.str, TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > + case NOMATCH_DRIVER: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_DRIVER: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchDriver"); >> > add_group_entry(&ptr->match_driver, >> > - xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + xstrtokenize(xf86_lex_val.str, TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > + case NOMATCH_TAG: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_TAG: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchTag"); >> > - add_group_entry(&ptr->match_tag, >> > xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + add_group_entry(&ptr->match_tag, >> > xstrtokenize(xf86_lex_val.str, >> > + TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > + case NOMATCH_LAYOUT: >> > + matchtype = MATCH_NEGATED; >> > + /* fallthrough */ >> > case MATCH_LAYOUT: >> > if (xf86getSubToken(&(ptr->comment)) != STRING) >> > Error(QUOTE_MSG, "MatchLayout"); >> > add_group_entry(&ptr->match_layout, >> > - xstrtokenize(xf86_lex_val.str, TOKEN_SEP)); >> > + xstrtokenize(xf86_lex_val.str, TOKEN_SEP), >> > + matchtype); >> > free(xf86_lex_val.str); >> > break; >> > case MATCH_IS_KEYBOARD: >> > diff --git a/hw/xfree86/parser/xf86Parser.h >> > b/hw/xfree86/parser/xf86Parser.h >> > index b3a50e5..a038f9e 100644 >> > --- a/hw/xfree86/parser/xf86Parser.h >> > +++ b/hw/xfree86/parser/xf86Parser.h >> > @@ -306,6 +306,7 @@ typedef struct { >> > typedef struct { >> > struct xorg_list entry; >> > char **values; >> > + Bool is_negated; >> > } xf86MatchGroup; >> > >> > typedef struct { >> > diff --git a/hw/xfree86/parser/xf86tokens.h >> > b/hw/xfree86/parser/xf86tokens.h >> > index bbd6b90..f955af0 100644 >> > --- a/hw/xfree86/parser/xf86tokens.h >> > +++ b/hw/xfree86/parser/xf86tokens.h >> > @@ -286,7 +286,17 @@ typedef enum { >> > MATCH_IS_JOYSTICK, >> > MATCH_IS_TABLET, >> > MATCH_IS_TOUCHPAD, >> > - MATCH_IS_TOUCHSCREEN >> > + MATCH_IS_TOUCHSCREEN, >> > + >> > + NOMATCH_PRODUCT, >> > + NOMATCH_VENDOR, >> > + NOMATCH_DEVICE_PATH, >> > + NOMATCH_OS, >> > + NOMATCH_PNPID, >> > + NOMATCH_USBID, >> > + NOMATCH_DRIVER, >> > + NOMATCH_TAG, >> > + NOMATCH_LAYOUT, >> > } ParserTokens; >> > >> > #endif /* _xf86_tokens_h */ >> > -- >> > 2.5.0 >> > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
