lkml.org 
[lkml]   [2013]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3] dynamic_debug: add wildcard support to filter files/functions/modules
From
2013/10/30 Joe Perches <joe@perches.com>:
> On Tue, 2013-10-29 at 21:33 +0800, Du, Changbin wrote:
>> This patch add wildcard '*'(matches zero or more characters) and '?'
>> (matches one character) support when qurying debug flags.
>
> Hi again. Some trivial notes and a possible logic error:
>

>> +/* check if the string matches given pattern which includes wildcards */
>> +static bool match_pattern(const char *pattern, const char *string)
>> +{
>> + const char *s = string,
>> + *p = pattern;
>
> This sort of alignment is pretty unusual.
> Most kernel uses just repeat the type like:
>
> const char *s = string;
> const char *p = pattern;
>
I think so.

>> + bool star = 0;
>
> bool star = false;
>
>> +
>> + while (*s) {
>> + switch (*p) {
>> + case '?':
>> + ++s, ++p;
>> + break;
>> + case '*':
>> + star = true;
>> + string = s;
>> + if (!*++p)
>> + return true;
>> + pattern = p;;
>
> repeated ;
> Running your patches through checkpatch should find
> this sort of defect.
>

Sorry, it's may fault. I forgot to check it using the tool.

>> + break;
>> + default:
>> + if (*s != *p) {
>> + if (!star)
>> + return false;
>> + string++;
>> + s = string;
>> + p = pattern;
>> + break;
>> + }
>> + ++s, ++p;
>
> Maybe nicer with an if/else, I think you're still
> missing a reset of "star = false;" and I also think
> it's better to use a break here too.
>
> if (*s == *p) {
> s++;
> p++;
> star = false;
> } else {
> if (!star)
> return false;
> string++;
> s = string;
> p = pattern;
> }
> break;

I have run loss of test before sending patch. all case passed. But I
will double check if need reset star flag. really thanks!


\
 
 \ /
  Last update: 2013-10-30 08:21    [W:0.095 / U:0.244 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site