Re: [PATCH 1/2] Add lib/glob.c
On Sat, 10 May 2014 08:21:38 -0400 Tejun Heo wrote:
> > +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.
If the tests don't measurably increase boot times we could run them
unconditionally at boot. Mark everything __init/__initdata and
the costs are very low. lib/plist.c does this.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On Sat, 10 May 2014 08:21:38 -0400 Tejun Heo t...@kernel.org wrote:
+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.
If the tests don't measurably increase boot times we could run them
unconditionally at boot. Mark everything __init/__initdata and
the costs are very low. lib/plist.c does this.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On Sat, May 10, 2014 at 10:29:18AM -0700, Randy Dunlap wrote: > > + select this option. Say N unless you are compiling an out-of > > + tree driver which tells you it depend on it. > > To support out-of-tree drivers, I'm pretty sure that you will need > to use obj- instead of lib-. lib- drops unused code, like Tejun said. I don't think we need to worry about out-of-tree drivers from the beginning. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On Sat, 10 May 2014 08:21:38 -0400, Tejun Heo wrote:
> 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.
But, I just confirmed, it will also be excluded from linking if
CONFIG_ATA=m. See for example commit b4d3ba3346f0 where some helpers
had to be moved from lib-y to obj-y to fix module load errors.
(Thanks to Randy Dunlap for this example.)
>> +#else
>> +
>> +#include
>> +#include
>> +
>> +MODULE_DESCRIPTION("glob(7) matching");
>> +MODULE_LICENSE("Dual MIT/GPL");
> Do we make library routines separate modules usually?
There are basically three options I can see:
1) Make it non-configurable and always include the code in the kernel
using oby-y, and just accept the wasted space if CONFIG_ATA=n
2) Make it a boolean option, so CONFIG_ATA=m will force CONFIG_GLOB=y and
it will be included in the static kernel but unused until the module
is loaded.
3) Make it a tristate option, which compiles into the kernel if
CONFIG_ATA=y (the common case), and is its own module if CONFIG_ATA=m.
An awful lot of the files in lib/ take this approach.
Do you have a preference? Option 3 is the current status.
(Revised patch will come when I have the self-test converted.)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On Sat, 10 May 2014 08:21:38 -0400, Tejun Heo wrote: 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. But, I just confirmed, it will also be excluded from linking if CONFIG_ATA=m. See for example commit b4d3ba3346f0 where some helpers had to be moved from lib-y to obj-y to fix module load errors. (Thanks to Randy Dunlap for this example.) +#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? There are basically three options I can see: 1) Make it non-configurable and always include the code in the kernel using oby-y, and just accept the wasted space if CONFIG_ATA=n 2) Make it a boolean option, so CONFIG_ATA=m will force CONFIG_GLOB=y and it will be included in the static kernel but unused until the module is loaded. 3) Make it a tristate option, which compiles into the kernel if CONFIG_ATA=y (the common case), and is its own module if CONFIG_ATA=m. An awful lot of the files in lib/ take this approach. Do you have a preference? Option 3 is the current status. (Revised patch will come when I have the self-test converted.) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On Sat, May 10, 2014 at 10:29:18AM -0700, Randy Dunlap wrote: + select this option. Say N unless you are compiling an out-of + tree driver which tells you it depend on it. To support out-of-tree drivers, I'm pretty sure that you will need to use obj- instead of lib-. lib- drops unused code, like Tejun said. I don't think we need to worry about out-of-tree drivers from the beginning. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On 05/09/2014 08:13 PM, George Spelvin wrote: > This is a helper function from drivers/ata/libata_core.c, where it is used > to blacklist particular device models. It's being moved to lib/ so other > drivers may use it for the same purpose. > > This implementation in non-recursive, so is safe for the kernel stack. > > Signed-off-by: George Spelvin > --- > Finally rescued this from the back burner. The code size will go back > down in the second patch which removes the old implementation, although > the number of source line reflects more comments and a test driver. > > The infrastructure parts of this, such as the module name and Kconfig > declarations, are something I haven't done before, and comments are > appreciated. > > Tejun Heo wrote: >> On Wed, Mar 12, 2014 at 06:52:44PM -0400, George Spelvin wrote: >>> Sure; I'll prepare some patches. May I feed it through you, or >>> is there a lib/ maintainer I need to go through? >> >> Please keep me cc'd but you'd probably also want to cc Linus, Andrew >> Morton and Ingo. > > include/linux/glob.h | 10 +++ > lib/Kconfig | 14 > lib/Makefile | 2 + > lib/glob.c | 225 > +++ > 4 files changed, 251 insertions(+) > create mode 100644 include/linux/glob.h > create mode 100644 lib/glob.c > diff --git a/lib/Kconfig b/lib/Kconfig > index 991c98b..5333d10 100644 > --- a/lib/Kconfig > +++ b/lib/Kconfig > @@ -373,6 +373,20 @@ config CPU_RMAP > config DQL > bool > > +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 I would drop "automatically". It has to be coded. > + select this option. Say N unless you are compiling an out-of > + tree driver which tells you it depend on it. To support out-of-tree drivers, I'm pretty sure that you will need to use obj- instead of lib-. lib- drops unused code, like Tejun said. > + > # > # Netlink attribute parsing support is select'ed if needed > # -- ~Randy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On 05/10/2014 07:03 AM, George Spelvin wrote:
> 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.
That's old/historical, not current.
> Huh, Documentation/kernel-doc-nano-HOWTO.txt (lines 57-59, to be precise)
> implies otherwise pretty strongly. But I can certainly change it.
Either way should be OK.
>> 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?
and sometimes we have to use obj-y instead of lib-y.
> 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
>>> +#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.
--
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
>> +#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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
Ooh, one more nitpick. 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. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
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
> +#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
> +#include
> +
> +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
> +#include
> +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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
Ooh, one more nitpick. 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. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On 05/10/2014 07:03 AM, George Spelvin wrote:
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.
That's old/historical, not current.
Huh, Documentation/kernel-doc-nano-HOWTO.txt (lines 57-59, to be precise)
implies otherwise pretty strongly. But I can certainly change it.
Either way should be OK.
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?
and sometimes we have to use obj-y instead of lib-y.
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.
--
~Randy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] Add lib/glob.c
On 05/09/2014 08:13 PM, George Spelvin wrote: This is a helper function from drivers/ata/libata_core.c, where it is used to blacklist particular device models. It's being moved to lib/ so other drivers may use it for the same purpose. This implementation in non-recursive, so is safe for the kernel stack. Signed-off-by: George Spelvin li...@horizon.com --- Finally rescued this from the back burner. The code size will go back down in the second patch which removes the old implementation, although the number of source line reflects more comments and a test driver. The infrastructure parts of this, such as the module name and Kconfig declarations, are something I haven't done before, and comments are appreciated. Tejun Heo hte...@gmail.com wrote: On Wed, Mar 12, 2014 at 06:52:44PM -0400, George Spelvin wrote: Sure; I'll prepare some patches. May I feed it through you, or is there a lib/ maintainer I need to go through? Please keep me cc'd but you'd probably also want to cc Linus, Andrew Morton and Ingo. include/linux/glob.h | 10 +++ lib/Kconfig | 14 lib/Makefile | 2 + lib/glob.c | 225 +++ 4 files changed, 251 insertions(+) create mode 100644 include/linux/glob.h create mode 100644 lib/glob.c diff --git a/lib/Kconfig b/lib/Kconfig index 991c98b..5333d10 100644 --- a/lib/Kconfig +++ b/lib/Kconfig @@ -373,6 +373,20 @@ config CPU_RMAP config DQL bool +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 I would drop automatically. It has to be coded. + select this option. Say N unless you are compiling an out-of + tree driver which tells you it depend on it. To support out-of-tree drivers, I'm pretty sure that you will need to use obj- instead of lib-. lib- drops unused code, like Tejun said. + # # Netlink attribute parsing support is select'ed if needed # -- ~Randy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
