Re: [Mesa-dev] [PATCH] util: require debug options to be separated by commas
On Wed, Jan 26, 2011 at 3:17 AM, José Fonseca jfons...@vmware.com wrote:
On Mon, 2011-01-24 at 20:52 -0800, Marek Olšák wrote:
Let's assume there are two options with names such that one is a substring
of another. Previously, if we only specified the longer one as a debug
option,
the shorter one would be considered specified as well (because of strstr).
This commit fixes it by checking that each option is surrounded by commas.
(a regexp would be nicer, but this is not a performance critical code)
---
src/gallium/auxiliary/util/u_debug.c | 39
+-
1 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/src/gallium/auxiliary/util/u_debug.c
b/src/gallium/auxiliary/util/u_debug.c
index 59b7613..8cf7660 100644
--- a/src/gallium/auxiliary/util/u_debug.c
+++ b/src/gallium/auxiliary/util/u_debug.c
@@ -180,6 +180,43 @@ debug_get_num_option(const char *name, long dfault)
return result;
}
+static boolean str_has_option(const char *str, const char *name)
+{
+ const char *substr;
+
+ /* OPTION=all */
+ if (!util_strcmp(str, all)) {
+ return TRUE;
+ }
+
+ /* OPTION=name */
+ if (!util_strcmp(str, name)) {
+ return TRUE;
+ }
+
+ substr = util_strstr(str, name);
+
+ if (substr) {
+ unsigned name_len = strlen(name);
+
+ /* OPTION=name,... */
+ if (substr == str substr[name_len] == ',') {
+ return TRUE;
+ }
+
+ /* OPTION=...,name */
+ if (substr+name_len == str+strlen(str) substr[-1] == ',') {
+ return TRUE;
+ }
+
+ /* OPTION=...,name,... */
+ if (substr[-1] == ',' substr[name_len] == ',') {
+ return TRUE;
+ }
+ }
+
+ return FALSE;
+}
Marek,
The intention is good -- it would be nice to get options obeyed properly
--, but this will fail to find foo in OPTION=prefixfoosuffix,foo, so
it's replacing a bug with another.
I'd prefer we stop using strstr completely, and instead do:
1 - find the first ',' or '\0' in the string
2 - compare the previous characters with the option being searched, and
return TRUE if matches
3 - if it was ',' go back to 1, but starting from character after ','.
4 - otherwise return FALSE
It should be robust and almost the same amount of code.
You could also strtok pretty easily, but it can be a little
heavyweight if you're doing this repeatedly.
char *tmp, *tok;
tmp = strdup(str);
for (tok = strtok(tmp, ,); tok; tok = strtok(NULL, ,)) {
if (strcmp(tok, opt) == 0) {
free(tmp);
return TRUE;
}
}
free(tmp);
return FALSE;
--
Dan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util: require debug options to be separated by commas
On Mon, 2011-01-24 at 20:52 -0800, Marek Olšák wrote:
Let's assume there are two options with names such that one is a substring
of another. Previously, if we only specified the longer one as a debug option,
the shorter one would be considered specified as well (because of strstr).
This commit fixes it by checking that each option is surrounded by commas.
(a regexp would be nicer, but this is not a performance critical code)
---
src/gallium/auxiliary/util/u_debug.c | 39
+-
1 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/src/gallium/auxiliary/util/u_debug.c
b/src/gallium/auxiliary/util/u_debug.c
index 59b7613..8cf7660 100644
--- a/src/gallium/auxiliary/util/u_debug.c
+++ b/src/gallium/auxiliary/util/u_debug.c
@@ -180,6 +180,43 @@ debug_get_num_option(const char *name, long dfault)
return result;
}
+static boolean str_has_option(const char *str, const char *name)
+{
+ const char *substr;
+
+ /* OPTION=all */
+ if (!util_strcmp(str, all)) {
+ return TRUE;
+ }
+
+ /* OPTION=name */
+ if (!util_strcmp(str, name)) {
+ return TRUE;
+ }
+
+ substr = util_strstr(str, name);
+
+ if (substr) {
+ unsigned name_len = strlen(name);
+
+ /* OPTION=name,... */
+ if (substr == str substr[name_len] == ',') {
+ return TRUE;
+ }
+
+ /* OPTION=...,name */
+ if (substr+name_len == str+strlen(str) substr[-1] == ',') {
+ return TRUE;
+ }
+
+ /* OPTION=...,name,... */
+ if (substr[-1] == ',' substr[name_len] == ',') {
+ return TRUE;
+ }
+ }
+
+ return FALSE;
+}
Marek,
The intention is good -- it would be nice to get options obeyed properly
--, but this will fail to find foo in OPTION=prefixfoosuffix,foo, so
it's replacing a bug with another.
I'd prefer we stop using strstr completely, and instead do:
1 - find the first ',' or '\0' in the string
2 - compare the previous characters with the option being searched, and
return TRUE if matches
3 - if it was ',' go back to 1, but starting from character after ','.
4 - otherwise return FALSE
It should be robust and almost the same amount of code.
Jose
unsigned long
debug_get_flags_option(const char *name,
@@ -207,7 +244,7 @@ debug_get_flags_option(const char *name,
else {
result = 0;
while( flags-name ) {
- if (!util_strcmp(str, all) || util_strstr(str, flags-name ))
+ if (str_has_option(str, flags-name))
result |= flags-value;
++flags;
}
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util: require debug options to be separated by commas
On Mon, Jan 24, 2011 at 9:52 PM, Marek Olšák mar...@gmail.com wrote: Let's assume there are two options with names such that one is a substring of another. Previously, if we only specified the longer one as a debug option, the shorter one would be considered specified as well (because of strstr). This commit fixes it by checking that each option is surrounded by commas. (a regexp would be nicer, but this is not a performance critical code) Looks good to me. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] util: require debug options to be separated by commas
Let's assume there are two options with names such that one is a substring
of another. Previously, if we only specified the longer one as a debug option,
the shorter one would be considered specified as well (because of strstr).
This commit fixes it by checking that each option is surrounded by commas.
(a regexp would be nicer, but this is not a performance critical code)
---
src/gallium/auxiliary/util/u_debug.c | 39 +-
1 files changed, 38 insertions(+), 1 deletions(-)
diff --git a/src/gallium/auxiliary/util/u_debug.c
b/src/gallium/auxiliary/util/u_debug.c
index 59b7613..8cf7660 100644
--- a/src/gallium/auxiliary/util/u_debug.c
+++ b/src/gallium/auxiliary/util/u_debug.c
@@ -180,6 +180,43 @@ debug_get_num_option(const char *name, long dfault)
return result;
}
+static boolean str_has_option(const char *str, const char *name)
+{
+ const char *substr;
+
+ /* OPTION=all */
+ if (!util_strcmp(str, all)) {
+ return TRUE;
+ }
+
+ /* OPTION=name */
+ if (!util_strcmp(str, name)) {
+ return TRUE;
+ }
+
+ substr = util_strstr(str, name);
+
+ if (substr) {
+ unsigned name_len = strlen(name);
+
+ /* OPTION=name,... */
+ if (substr == str substr[name_len] == ',') {
+ return TRUE;
+ }
+
+ /* OPTION=...,name */
+ if (substr+name_len == str+strlen(str) substr[-1] == ',') {
+ return TRUE;
+ }
+
+ /* OPTION=...,name,... */
+ if (substr[-1] == ',' substr[name_len] == ',') {
+ return TRUE;
+ }
+ }
+
+ return FALSE;
+}
unsigned long
debug_get_flags_option(const char *name,
@@ -207,7 +244,7 @@ debug_get_flags_option(const char *name,
else {
result = 0;
while( flags-name ) {
-if (!util_strcmp(str, all) || util_strstr(str, flags-name ))
+if (str_has_option(str, flags-name))
result |= flags-value;
++flags;
}
--
1.7.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
