include/list.h
Peter Hutterer
peter.hutterer at who-t.net
Thu Jun 10 22:24:47 PDT 2010
On Thu, Jun 10, 2010 at 10:13:16PM -0700, Keith Packard wrote:
>
> Argh! A recent commit to the xf86 config parsing code added "list.h" to
> xf86Parser.h, which is included by drivers to look stuff up. Because of
> this, the intel driver no longer builds against master (would that the
> drivers were in the server tree...). I have an RC1 tar file sitting here
> and decided that I really should be running those bits before pushing it
> out, and I really think I should at least try running it first.
>
> To my mind, list.h really isn't needed in xf86Parser -- list elements
> are only ever added to the head and then the whole list freed at once.
>
> Here's an open coded replacement; shorter and has no casts.
>
> From 0192d160c51f58fbdf566ebf2b7e05faba51e91f Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp at keithp.com>
> Date: Thu, 10 Jun 2010 22:08:46 -0700
> Subject: [PATCH] Use open-coded lists for Input match groups
>
> There's no reason to use the complicated list macros for something as
> simple as a singly linked list; switching to open coded list walking
> reduces the code considerably, and also eliminates exposing the list
> macros in a header file required by drivers.
isn't the whole point of the list macros that we don't have dozens of
different list implementations, all with their own bugs?
you're just moving the problem, not fixing it.
fwiw, the xf86printMatchGroup and xf86freeMatchGroup would be useful even
if we keep the list implementation, they're the main cleanup here.
Cheers,
Peter
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
> hw/xfree86/common/xf86Xinput.c | 24 +++---
> hw/xfree86/parser/InputClass.c | 205 ++++++++++++++--------------------------
> hw/xfree86/parser/xf86Parser.h | 21 ++--
> 3 files changed, 91 insertions(+), 159 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
> index 76d2d00..cd5f4ce 100644
> --- a/hw/xfree86/common/xf86Xinput.c
> +++ b/hw/xfree86/common/xf86Xinput.c
> @@ -555,13 +555,13 @@ match_path_pattern(const char *attr, const char *pattern)
> * If a pattern in each list entry is matched, return TRUE.
> */
> static Bool
> -MatchAttrToken(const char *attr, struct list *patterns,
> +MatchAttrToken(const char *attr, xf86MatchGroup *patterns,
> int (*compare)(const char *attr, const char *pattern))
> {
> const xf86MatchGroup *group;
>
> /* If there are no patterns, accept the match */
> - if (list_is_empty(patterns))
> + if (!patterns)
> return TRUE;
>
> /* If there are patterns but no attribute, reject the match */
> @@ -572,7 +572,7 @@ MatchAttrToken(const char *attr, struct list *patterns,
> * Otherwise, iterate the list of patterns ensuring each entry has a
> * match. Each list entry is a separate Match line of the same type.
> */
> - list_for_each_entry(group, patterns, entry) {
> + for (group = patterns; group; group = group->next) {
> char * const *cur;
> Bool match = FALSE;
>
> @@ -598,45 +598,45 @@ InputClassMatches(const XF86ConfInputClassPtr iclass, const IDevPtr idev,
> const InputAttributes *attrs)
> {
> /* MatchProduct substring */
> - if (!MatchAttrToken(attrs->product, &iclass->match_product, match_substring))
> + if (!MatchAttrToken(attrs->product, iclass->match_product, match_substring))
> return FALSE;
>
> /* MatchVendor substring */
> - if (!MatchAttrToken(attrs->vendor, &iclass->match_vendor, match_substring))
> + if (!MatchAttrToken(attrs->vendor, iclass->match_vendor, match_substring))
> return FALSE;
>
> /* MatchDevicePath pattern */
> - if (!MatchAttrToken(attrs->device, &iclass->match_device, match_path_pattern))
> + if (!MatchAttrToken(attrs->device, iclass->match_device, match_path_pattern))
> return FALSE;
>
> /* MatchOS case-insensitive string */
> - if (!MatchAttrToken(HostOS(), &iclass->match_os, strcasecmp))
> + if (!MatchAttrToken(HostOS(), iclass->match_os, strcasecmp))
> return FALSE;
>
> /* MatchPnPID pattern */
> - if (!MatchAttrToken(attrs->pnp_id, &iclass->match_pnpid, match_pattern))
> + if (!MatchAttrToken(attrs->pnp_id, iclass->match_pnpid, match_pattern))
> return FALSE;
>
> /* MatchUSBID pattern */
> - if (!MatchAttrToken(attrs->usb_id, &iclass->match_usbid, match_pattern))
> + if (!MatchAttrToken(attrs->usb_id, iclass->match_usbid, match_pattern))
> return FALSE;
>
> /* MatchDriver string */
> - if (!MatchAttrToken(idev->driver, &iclass->match_driver, strcmp))
> + if (!MatchAttrToken(idev->driver, iclass->match_driver, strcmp))
> return FALSE;
>
> /*
> * MatchTag string
> * See if any of the device's tags match any of the MatchTag tokens.
> */
> - if (!list_is_empty(&iclass->match_tag)) {
> + if (iclass->match_tag) {
> char * const *tag;
> Bool match;
>
> 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, strcmp)) {
> match = TRUE;
> break;
> }
> diff --git a/hw/xfree86/parser/InputClass.c b/hw/xfree86/parser/InputClass.c
> index ce611d9..95a4431 100644
> --- a/hw/xfree86/parser/InputClass.c
> +++ b/hw/xfree86/parser/InputClass.c
> @@ -66,14 +66,28 @@ xf86ConfigSymTabRec InputClassTab[] =
> #define TOKEN_SEP "|"
>
> static void
> -add_group_entry(struct list *head, char **values)
> +xf86addMatchGroupEntry(xf86MatchGroup **head, char **values)
> {
> xf86MatchGroup *group;
>
> group = malloc(sizeof(*group));
> if (group) {
> group->values = values;
> - list_add(&group->entry, head);
> + group->next = *head;
> + *head = group;
> + }
> +}
> +
> +static void
> +xf86freeMatchGroup(xf86MatchGroup **head)
> +{
> + char **value;
> + xf86MatchGroup *group;
> + while ((group = *head)) {
> + *head = group->next;
> + for (value = group->values; *value; value++)
> + free(*value);
> + free(group);
> }
> }
>
> @@ -86,14 +100,14 @@ xf86parseInputClassSection(void)
> parsePrologue(XF86ConfInputClassPtr, XF86ConfInputClassRec)
>
> /* Initialize MatchGroup lists */
> - list_init(&ptr->match_product);
> - list_init(&ptr->match_vendor);
> - list_init(&ptr->match_device);
> - list_init(&ptr->match_os);
> - list_init(&ptr->match_pnpid);
> - list_init(&ptr->match_usbid);
> - list_init(&ptr->match_driver);
> - list_init(&ptr->match_tag);
> + ptr->match_product = NULL;
> + ptr->match_vendor = NULL;
> + ptr->match_device = NULL;
> + ptr->match_os = NULL;
> + ptr->match_pnpid = NULL;
> + ptr->match_usbid = NULL;
> + ptr->match_driver = NULL;
> + ptr->match_tag = NULL;
>
> while ((token = xf86getToken(InputClassTab)) != ENDSECTION) {
> switch (token) {
> @@ -122,50 +136,50 @@ xf86parseInputClassSection(void)
> case MATCH_PRODUCT:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> Error(QUOTE_MSG, "MatchProduct");
> - add_group_entry(&ptr->match_product,
> - xstrtokenize(val.str, TOKEN_SEP));
> + xf86addMatchGroupEntry(&ptr->match_product,
> + xstrtokenize(val.str, TOKEN_SEP));
> break;
> case MATCH_VENDOR:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> Error(QUOTE_MSG, "MatchVendor");
> - add_group_entry(&ptr->match_vendor,
> - xstrtokenize(val.str, TOKEN_SEP));
> + xf86addMatchGroupEntry(&ptr->match_vendor,
> + xstrtokenize(val.str, TOKEN_SEP));
> break;
> case MATCH_DEVICE_PATH:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> Error(QUOTE_MSG, "MatchDevicePath");
> - add_group_entry(&ptr->match_device,
> - xstrtokenize(val.str, TOKEN_SEP));
> + xf86addMatchGroupEntry(&ptr->match_device,
> + xstrtokenize(val.str, TOKEN_SEP));
> break;
> case MATCH_OS:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> Error(QUOTE_MSG, "MatchOS");
> - add_group_entry(&ptr->match_os,
> - xstrtokenize(val.str, TOKEN_SEP));
> + xf86addMatchGroupEntry(&ptr->match_os,
> + xstrtokenize(val.str, TOKEN_SEP));
> break;
> case MATCH_PNPID:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> Error(QUOTE_MSG, "MatchPnPID");
> - add_group_entry(&ptr->match_pnpid,
> - xstrtokenize(val.str, TOKEN_SEP));
> + xf86addMatchGroupEntry(&ptr->match_pnpid,
> + xstrtokenize(val.str, TOKEN_SEP));
> break;
> case MATCH_USBID:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> Error(QUOTE_MSG, "MatchUSBID");
> - add_group_entry(&ptr->match_usbid,
> - xstrtokenize(val.str, TOKEN_SEP));
> + xf86addMatchGroupEntry(&ptr->match_usbid,
> + xstrtokenize(val.str, TOKEN_SEP));
> break;
> case MATCH_DRIVER:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> Error(QUOTE_MSG, "MatchDriver");
> - add_group_entry(&ptr->match_driver,
> - xstrtokenize(val.str, TOKEN_SEP));
> + xf86addMatchGroupEntry(&ptr->match_driver,
> + xstrtokenize(val.str, TOKEN_SEP));
> break;
> case MATCH_TAG:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> Error(QUOTE_MSG, "MatchTag");
> - add_group_entry(&ptr->match_tag,
> - xstrtokenize(val.str, TOKEN_SEP));
> + xf86addMatchGroupEntry(&ptr->match_tag,
> + xstrtokenize(val.str, TOKEN_SEP));
> break;
> case MATCH_IS_KEYBOARD:
> if (xf86getSubToken(&(ptr->comment)) != STRING)
> @@ -234,12 +248,22 @@ xf86parseInputClassSection(void)
> return ptr;
> }
>
> +static void
> +xf86printMatchGroup(FILE *cf, xf86MatchGroup *group, char *title)
> +{
> + char *const *cur;
> + for(; group; group = group->next) {
> + fprintf(cf, "\t%-17.17s\"", title);
> + for (cur = group->values; *cur; cur++)
> + fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> + *cur);
> + fprintf(cf, "\"\n");
> + }
> +}
> +
> void
> xf86printInputClassSection (FILE * cf, XF86ConfInputClassPtr ptr)
> {
> - const xf86MatchGroup *group;
> - char * const *cur;
> -
> while (ptr) {
> fprintf(cf, "Section \"InputClass\"\n");
> if (ptr->comment)
> @@ -249,62 +273,14 @@ xf86printInputClassSection (FILE * cf, XF86ConfInputClassPtr ptr)
> if (ptr->driver)
> fprintf(cf, "\tDriver \"%s\"\n", ptr->driver);
>
> - list_for_each_entry(group, &ptr->match_product, entry) {
> - fprintf(cf, "\tMatchProduct \"");
> - for (cur = group->values; *cur; cur++)
> - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> - *cur);
> - fprintf(cf, "\"\n");
> - }
> - list_for_each_entry(group, &ptr->match_vendor, entry) {
> - fprintf(cf, "\tMatchVendor \"");
> - for (cur = group->values; *cur; cur++)
> - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> - *cur);
> - fprintf(cf, "\"\n");
> - }
> - list_for_each_entry(group, &ptr->match_device, entry) {
> - fprintf(cf, "\tMatchDevicePath \"");
> - for (cur = group->values; *cur; cur++)
> - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> - *cur);
> - fprintf(cf, "\"\n");
> - }
> - list_for_each_entry(group, &ptr->match_os, entry) {
> - fprintf(cf, "\tMatchOS \"");
> - for (cur = group->values; *cur; cur++)
> - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> - *cur);
> - fprintf(cf, "\"\n");
> - }
> - list_for_each_entry(group, &ptr->match_pnpid, entry) {
> - fprintf(cf, "\tMatchPnPID \"");
> - for (cur = group->values; *cur; cur++)
> - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> - *cur);
> - fprintf(cf, "\"\n");
> - }
> - list_for_each_entry(group, &ptr->match_usbid, entry) {
> - fprintf(cf, "\tMatchUSBID \"");
> - for (cur = group->values; *cur; cur++)
> - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> - *cur);
> - fprintf(cf, "\"\n");
> - }
> - list_for_each_entry(group, &ptr->match_driver, entry) {
> - fprintf(cf, "\tMatchDriver \"");
> - for (cur = group->values; *cur; cur++)
> - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> - *cur);
> - fprintf(cf, "\"\n");
> - }
> - list_for_each_entry(group, &ptr->match_tag, entry) {
> - fprintf(cf, "\tMatchTag \"");
> - for (cur = group->values; *cur; cur++)
> - fprintf(cf, "%s%s", cur == group->values ? "" : TOKEN_SEP,
> - *cur);
> - fprintf(cf, "\"\n");
> - }
> + xf86printMatchGroup(cf, ptr->match_product, "MatchProduct");
> + xf86printMatchGroup(cf, ptr->match_vendor, "MatchVendor");
> + xf86printMatchGroup(cf, ptr->match_device, "MatchDevicePath");
> + xf86printMatchGroup(cf, ptr->match_os, "MatchOS");
> + xf86printMatchGroup(cf, ptr->match_pnpid, "MatchPnPID");
> + xf86printMatchGroup(cf, ptr->match_usbid, "MatchUSBID");
> + xf86printMatchGroup(cf, ptr->match_driver, "MatchDriver");
> + xf86printMatchGroup(cf, ptr->match_tag, "MatchTag");
>
> if (ptr->is_keyboard.set)
> fprintf(cf, "\tIsKeyboard \"%s\"\n",
> @@ -336,60 +312,17 @@ xf86freeInputClassList (XF86ConfInputClassPtr ptr)
> XF86ConfInputClassPtr prev;
>
> while (ptr) {
> - xf86MatchGroup *group, *next;
> - char **list;
> -
> TestFree(ptr->identifier);
> TestFree(ptr->driver);
>
> - list_for_each_entry_safe(group, next, &ptr->match_product, entry) {
> - list_del(&group->entry);
> - for (list = group->values; *list; list++)
> - free(*list);
> - free(group);
> - }
> - list_for_each_entry_safe(group, next, &ptr->match_vendor, entry) {
> - list_del(&group->entry);
> - for (list = group->values; *list; list++)
> - free(*list);
> - free(group);
> - }
> - list_for_each_entry_safe(group, next, &ptr->match_device, entry) {
> - list_del(&group->entry);
> - for (list = group->values; *list; list++)
> - free(*list);
> - free(group);
> - }
> - list_for_each_entry_safe(group, next, &ptr->match_os, entry) {
> - list_del(&group->entry);
> - for (list = group->values; *list; list++)
> - free(*list);
> - free(group);
> - }
> - list_for_each_entry_safe(group, next, &ptr->match_pnpid, entry) {
> - list_del(&group->entry);
> - for (list = group->values; *list; list++)
> - free(*list);
> - free(group);
> - }
> - list_for_each_entry_safe(group, next, &ptr->match_usbid, entry) {
> - list_del(&group->entry);
> - for (list = group->values; *list; list++)
> - free(*list);
> - free(group);
> - }
> - list_for_each_entry_safe(group, next, &ptr->match_driver, entry) {
> - list_del(&group->entry);
> - for (list = group->values; *list; list++)
> - free(*list);
> - free(group);
> - }
> - list_for_each_entry_safe(group, next, &ptr->match_tag, entry) {
> - list_del(&group->entry);
> - for (list = group->values; *list; list++)
> - free(*list);
> - free(group);
> - }
> + xf86freeMatchGroup(&ptr->match_product);
> + xf86freeMatchGroup(&ptr->match_vendor);
> + xf86freeMatchGroup(&ptr->match_device);
> + xf86freeMatchGroup(&ptr->match_os);
> + xf86freeMatchGroup(&ptr->match_pnpid);
> + xf86freeMatchGroup(&ptr->match_usbid);
> + xf86freeMatchGroup(&ptr->match_driver);
> + xf86freeMatchGroup(&ptr->match_tag);
>
> TestFree(ptr->comment);
> xf86optionListFree(ptr->option_lst);
> diff --git a/hw/xfree86/parser/xf86Parser.h b/hw/xfree86/parser/xf86Parser.h
> index 337ad07..6283167 100644
> --- a/hw/xfree86/parser/xf86Parser.h
> +++ b/hw/xfree86/parser/xf86Parser.h
> @@ -66,7 +66,6 @@
>
> #include <X11/Xdefs.h>
> #include "xf86Optrec.h"
> -#include "list.h"
>
> #define HAVE_PARSER_DECLS
>
> @@ -339,9 +338,9 @@ typedef struct
> }
> xf86TriState;
>
> -typedef struct
> +typedef struct _xf86MatchGroup
> {
> - struct list entry;
> + struct _xf86MatchGroup *next;
> char **values;
> }
> xf86MatchGroup;
> @@ -351,14 +350,14 @@ typedef struct
> GenericListRec list;
> char *identifier;
> char *driver;
> - struct list match_product;
> - struct list match_vendor;
> - struct list match_device;
> - struct list match_os;
> - struct list match_pnpid;
> - struct list match_usbid;
> - struct list match_driver;
> - struct list match_tag;
> + xf86MatchGroup *match_product;
> + xf86MatchGroup *match_vendor;
> + xf86MatchGroup *match_device;
> + xf86MatchGroup *match_os;
> + xf86MatchGroup *match_pnpid;
> + xf86MatchGroup *match_usbid;
> + xf86MatchGroup *match_driver;
> + xf86MatchGroup *match_tag;
> xf86TriState is_keyboard;
> xf86TriState is_pointer;
> xf86TriState is_joystick;
> --
> 1.7.1
>
>
>
> --
> keith.packard at intel.com
More information about the xorg-devel
mailing list