[Mesa-dev] [PATCH] util: require debug options to be separated by commas

[José Fonseca]

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;
>        }