linux-kernel - Re: [PATCH v3] dynamic_debug: add wildcard support to filter files/functions/modules

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 30 Oct 2013 14:58:07 +0800
From:	Changbin Du <changbin.du@...il.com>
To:	Joe Perches <joe@...ches.com>
Cc:	Jason Baron <jbaron@...mai.com>,
	"linux-kernel@...r.kernel.org list" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] dynamic_debug: add wildcard support to filter files/functions/modules

2013/10/30 Joe Perches <joe@...ches.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!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/