Re: [PATCH 1/2] Add lib/glob.c

[Tejun Heo]

Hello,

On Fri, May 09, 2014 at 11:13:56PM -0400, George Spelvin wrote:
> +config GLOB
> +	tristate
> +#	(Prompt disabled to reduce kbuild clutter until someone needs it.)
> +#	prompt "glob_match() function"
> +	help
> +	  This option provides a glob_match function for performing simple
> +	  text pattern matching.  It is primarily used by the ATA code
> +	  to blacklist particular drive models, but other device drivers
> +	  may need similar functionality.
> +
> +	  All in-kernel drivers that require this function automatically
> +	  select this option.  Say N unless you are compiling an out-of
> +	  tree driver which tells you it depend on it.

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

> +#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.

> +#else
> +
> +#include <linux/module.h>
> +#include <linux/glob.h>
> +
> +MODULE_DESCRIPTION("glob(7) matching");
> +MODULE_LICENSE("Dual MIT/GPL");

Do we make library routines separate modules usually?

...
> +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)

> +{
> +	/*
> +	 * 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 haven't delved into the actual implementation.  Looks sane on the
first glance.

> +#ifdef UNITTEST
> +
> +/* Test code */
> +#include <stdio.h>
> +#include <stdlib.h>
> +struct glob_test {
> +	char const *pat, *str;
> +	bool expected;
> +};
> +
> +static void
> +test(struct glob_test const *g)
> +{
> +	bool match = glob_match(g->pat, g->str);
> +
> +	printf("\"%s\" vs. \"%s\": %s %s\n", g->pat, g->str,
> +		match ? "match" : "mismatch",
> +		match == g->expected ? "OK" : "*** ERROR ***");
> +	if (match != g->expected)
> +		exit(1);
> +}
> +
> +static struct glob_test const tests[] = {
> +	{ "a", "a", true },
...
> +	{ "*ab*cd*", "abcabcabcabcefg", false }
> +};
> +
> +int
> +main(void)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < sizeof(tests)/sizeof(*tests); i++)
> +		test(tests + i);
> +
> +	return 0;
> +}
> +
> +#endif	/* UNITTEST */

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

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/