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:	10 May 2014 10:03:04 -0400
From:	"George Spelvin" <linux@...izon.com>
To:	linux@...izon.com, tj@...nel.org
Cc:	akpm@...ux-foundation.org, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org, mingo@...hat.com,
	torvalds@...ux-foundation.org
Subject: Re: [PATCH 1/2] Add lib/glob.c

Thanks a lot for the feedback!

> On Fri, May 09, 2014 at 11:13:56PM -0400, George Spelvin wrote:
>> +/**
>> + * glob_match - Shell-style pattern matching, like !fnmatch(pat, str, 0)
>> + * @pat: Pattern to match.  Metacharacters are ?, *, [ and \.
>> + *       (And, inside character classes, !, - and ].)

> @ARG lines should be contained in a single line.  Just "Pattern to
> match." should do.  With detailed description in the body.

Huh, Documentation/kernel-doc-nano-HOWTO.txt (lines 57-59, to be precise)
implies otherwise pretty strongly.  But I can certainly change it.

> Just adding glob.o to lib-y should be enough.  It will be excluded
> from linking if unused.

Will that work right if the caller is a module?  What will it get linked
into, the main kernel binary or the module?

A significant and very helpful simplification; I just want to be sure
it works right.

>> +#ifdef UNITTEST
>> +/* To do a basic sanity test, "cc -DUNITTEST glob.c" and run a.out. */
>> +
>> +#include <stdbool.h>
>> +#define __pure __attribute__((pure))
>> +#define NOP(x)
>> +#define EXPORT_SYMBOL NOP	/* Two stages to avoid checkpatch complaints */

> These things tend to bitrot.  Let's please keep testing harness out of
> tree.

Damn, when separated it bitrots a lot faster.  That's *is* my testing
harness, and I wanted to keep it close so it has a chance on hell of
being used by someone who updates it.

Especially given that the function's interface is quite rigidly defined,
do you really think there will be a lot of rot?

> Do we make library routines separate modules usually?

A large number of files in lib/ are implemented that way (lib/crc-ccitt.c,
just for one example), and that's what I copied.  But if I just do the
obj-y thing, all that goes away

>> +bool __pure
>> +glob_match(char const *pat, char const *str)
>
> The whole thing fits in a single 80 column line, right?
> 
> bool __pure glob_match(char const *pat, char const *str)

Whoops, a residue of my personal code style.  (I like to left-align
function names in definitions so they're easy to search for with ^func.)
But it's not kernel style.  Will fix.

>> +{
>> +	/*
>> +	 * Backtrack to previous * on mismatch and retry starting one
>> +	 * character later in the string.  Because * matches all characters
>> +	 * (no exception for /), it can be easily proved that there's
>> +	 * never a need to backtrack multiple levels.
>> +	 */
>> +	char const *back_pat = 0, *back_str = back_str;

> Blank line here.

I had considered the "/*" start of the following block comment as visually
enough separation between variable declarations and statements, but sure,
I can add one.

> I haven't delved into the actual implementation.  Looks sane on the
> first glance.

That's the part I'm least worried about, actually.

> Again, I don't really think the userland testing code belongs here.
> If you want to keep them, please make it in-kernel selftesting.  We
> don't really want to keep code which can't get built and tested in
> kernel tree proper.

I'll see if I can figure out how to do that.  Simple as it is, I hate to
throw away regression tests.

Thank you very much.
--
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/

Powered by blists - more mailing lists