| Message ID | 4C31EB69.5070903@teksavvy.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 07/05/2010 10:25 AM, Mark Lord wrote: > Enable use of hyphenated pattern ranges in glob_match(), similar to how > shell globbing works, and how developers might expect things to work. > > Signed-off-by: Mark Lord <mlord@pobox.com> > --- > > Patch also attached, to get around ongoing mailer issues here. > Eventually, glob_match() will move out of libata into lib/string.c or > similar. No issue with correctness... but where is the demonstrated need? Did someone ask for this? Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/07/10 02:33 PM, Jeff Garzik wrote: > On 07/05/2010 10:25 AM, Mark Lord wrote: >> Enable use of hyphenated pattern ranges in glob_match(), similar to how >> shell globbing works, and how developers might expect things to work. >> >> Signed-off-by: Mark Lord <mlord@pobox.com> >> --- >> >> Patch also attached, to get around ongoing mailer issues here. >> Eventually, glob_match() will move out of libata into lib/string.c or >> similar. > > No issue with correctness... but where is the demonstrated need? > > Did someone ask for this? .. Not specifically. But since this will eventually end up as a kernel-wide library function, somebody somewhere will eventually make the reasonable assumption that it does globbing "in the usual way". Adding the hyphenated ranges turned out to be almost trivial, so I added it on and posted the patch. I could also send you follow-on patches to use that feature in the ata blacklists, if you like, saving a few more bytes hither and dither. :) Speaking of this stuff.. how do we go about moving this function to /lib/strings.c ? Can that be done via your git tree? Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/05/2010 05:47 PM, Mark Lord wrote: > On 05/07/10 02:33 PM, Jeff Garzik wrote: >> On 07/05/2010 10:25 AM, Mark Lord wrote: >>> Enable use of hyphenated pattern ranges in glob_match(), similar to how >>> shell globbing works, and how developers might expect things to work. >>> >>> Signed-off-by: Mark Lord <mlord@pobox.com> >>> --- >>> >>> Patch also attached, to get around ongoing mailer issues here. >>> Eventually, glob_match() will move out of libata into lib/string.c or >>> similar. >> >> No issue with correctness... but where is the demonstrated need? >> >> Did someone ask for this? > .. > > Not specifically. But since this will eventually end up as a kernel-wide > library function, somebody somewhere will eventually make the reasonable > assumption that it does globbing "in the usual way". > > Adding the hyphenated ranges turned out to be almost trivial, > so I added it on and posted the patch. > > I could also send you follow-on patches to use that feature > in the ata blacklists, if you like, saving a few more bytes hither and > dither. :) I understand the logic, but typically lean towards rejecting this sort of patch due to "mission creep." It's always easy to make code do One More Thing, but without any actual users, there is little justification. In this case, if you send a follow-on patch to _use_ this, I would be happy to apply both glob_match() and blacklist patches at that point. > Speaking of this stuff.. how do we go about moving this function to > /lib/strings.c ? > Can that be done via your git tree? Technically, yes, git can easily do that. Two comments come to mind, though: 1) ditto on mission creep. lib/string.c is a burden carried by -all- Linux platforms unconditionally, whereas loading libata.ko depends on several conditions. I wouldn't advise moving it until additional usage justifies burdening all platforms -- including non-SATA tiny embedded platforms -- with glob_match() implementation. It's becoming a real problem with lib/ in general, unfortunately. If two minor sections of the kernel (or two minor, wholly unrelated drivers) need some common code, it gets dumped into lib/ with the associated costs forced upon all. 2) once other kernel users appear, submit the "move to lib/string" patch as a distinct patch in the series of patches to driver XYZ adding a glob_match() caller. I'll happily add an Acked-by on your patch, and it can be submitted either direct to Linus/Andrew, or via the specific subsystem tree that is adding a new glob_match() caller. I prefer the Acked-by route for patches that are only tangentially related to libata. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/07/10 06:20 PM, Jeff Garzik wrote: > > In this case, if you send a follow-on patch to _use_ this, I would be > happy to apply both glob_match() and blacklist patches at that point. .. Okay, I'll do that. Shortly. > 2) once other kernel users appear, submit the "move to lib/string" patch > as a distinct patch in the series of patches to driver XYZ adding a > glob_match() caller. I'll happily add an Acked-by on your patch, and it > can be submitted either direct to Linus/Andrew, or via the specific > subsystem tree that is adding a new glob_match() caller. .. James was nudging for me/somebody to do the same sort of thing for SCSI black/white lists. So that'll be the next user, I suppose. Except those'll have to go via his tree, which currently lacks the glob_match() function. So I suppose perhaps I'll just wait until after 2.6.36 opens for that. Cheers -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Enable use of hyphenated pattern ranges in glob_match(), similar to how shell globbing works, and how developers might expect things to work. Signed-off-by: Mark Lord <mlord@pobox.com> --- Patch also attached, to get around ongoing mailer issues here. Eventually, glob_match() will move out of libata into lib/string.c or similar. --- old/drivers/ata/libata-core.c 2010-07-01 18:12:52.471339000 -0400 +++ linux/drivers/ata/libata-core.c 2010-07-05 10:23:51.198204782 -0400 @@ -4283,11 +4283,13 @@ * ? matches any single character. * * matches any run of characters. * [xyz] matches a single character from the set: x, y, or z. + * [a-d] matches a single character from the range: a, b, c, or d. + * [a-d0-9] matches a single character from either range. * - * Note: hyphenated ranges [0-9] are _not_ supported here. - * The special characters ?, [, or *, can be matched using a set, eg. [*] + * The special characters ?, [, -, or *, can be matched using a set, eg. [*] + * Behaviour with malformed patterns is undefined, though generally reasonable. * - * Example patterns: "SD1?", "SD1[012345]", "*R0", SD*1?[012]*xx" + * Example patterns: "SD1?", "SD1[0-5]", "*R0", SD*1?[012]*xx" * * This function uses one level of recursion per '*' in pattern. * Since it calls _nothing_ else, and has _no_ explicit local variables, @@ -4307,7 +4309,13 @@ /* Match single char against a '[' bracketed ']' pattern set */ if (!*text || *pattern != '[') break; /* Not a pattern set */ - while (*++pattern && *pattern != ']' && *text != *pattern); + while (*++pattern && *pattern != ']' && *text != *pattern) { + if (*pattern == '-' && *(pattern - 1) != '[') + if (*text > *(pattern - 1) && *text < *(pattern + 1)) { + ++pattern; + break; + } + } if (!*pattern || *pattern == ']') return 1; /* No match */ while (*pattern && *pattern++ != ']');
Enable use of hyphenated pattern ranges in glob_match(), similar to how shell globbing works, and how developers might expect things to work. Signed-off-by: Mark Lord <mlord@pobox.com> --- Patch also attached, to get around ongoing mailer issues here. Eventually, glob_match() will move out of libata into lib/string.c or similar.