[libvirt] [PATCH] log: actually do substring matches with fnmatch
Historically we matched log filters with strstr(), and when switching to fnmatch in cbb0fd3cfdc287f6f4653ef1f04a7cfb2ea51b27, it was stated that we would continue to match substrings, with "foo" being equivalent to "*foo*". Unfortuntely I forget to provide the code to actually make that happen. This fixes it to prepend and append "*" if not already present in the user's pattern. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virlog.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/util/virlog.c b/src/util/virlog.c index be9fc0cf78..d548010b10 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1409,6 +1409,8 @@ virLogFilterNew(const char *match, { virLogFilterPtr ret = NULL; char *mdup = NULL; + size_t mlen = strlen(match); + size_t off = 0; virCheckFlags(VIR_LOG_STACK_TRACE, NULL); @@ -1418,9 +1420,19 @@ virLogFilterNew(const char *match, return NULL; } - if (VIR_STRDUP_QUIET(mdup, match) < 0) + if (VIR_ALLOC_N_QUIET(mdup, mlen + 3) < 0) return NULL; + if (match[0] != '*') { + mdup[off++] = '*'; + } + memcpy(mdup + off, match, mlen + 1); + off += mlen; + if (match[mlen - 1] != '*') { + mdup[off++] = '*'; + mdup[off++] = '\0'; + } + if (VIR_ALLOC_QUIET(ret) < 0) { VIR_FREE(mdup); return NULL; -- 2.17.0
On Mon, May 14, 2018 at 02:53:49PM +0100, Daniel P. Berrangé wrote:
Historically we matched log filters with strstr(), and when switching to fnmatch in cbb0fd3cfdc287f6f4653ef1f04a7cfb2ea51b27, it was stated that we would continue to match substrings, with "foo" being equivalent to "*foo*". Unfortuntely I forget to provide the code to actually make that happen. This fixes it to prepend and append "*" if not already present in the user's pattern.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virlog.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/util/virlog.c b/src/util/virlog.c index be9fc0cf78..d548010b10 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1409,6 +1409,8 @@ virLogFilterNew(const char *match, { virLogFilterPtr ret = NULL; char *mdup = NULL; + size_t mlen = strlen(match); + size_t off = 0;
virCheckFlags(VIR_LOG_STACK_TRACE, NULL);
@@ -1418,9 +1420,19 @@ virLogFilterNew(const char *match, return NULL; }
- if (VIR_STRDUP_QUIET(mdup, match) < 0) + if (VIR_ALLOC_N_QUIET(mdup, mlen + 3) < 0) return NULL;
At first glance it's not very clear why we need the hunk below, so I'd add a comment stating that we treat a match 'foo' to be equal to '*foo*' for the purposes of fnmatch() use. Also, now that I see commit 8ccee910f5, I know we say we treat the match always as substring, but I'd go even further and enhance the sentence in that config (could be a separate patch) that not just 'foo', but also '*foo' and 'foo*' are in fact equal to '*foo*' for that matter. But this config change is not a deal-breaker to me.
+ if (match[0] != '*') { + mdup[off++] = '*'; + }
^Braces around a single line body...
+ memcpy(mdup + off, match, mlen + 1);
Why do you copy the null byte too? 'mlen' should suffice.
+ off += mlen; + if (match[mlen - 1] != '*') { + mdup[off++] = '*'; + mdup[off++] = '\0';
You won't need ^this extra null byte assignment, because we always use calloc to allocate a block.
+ } + if (VIR_ALLOC_QUIET(ret) < 0) { VIR_FREE(mdup); return NULL;
With the nit-picks fixed and a commentary added right before the hunk this patch adds: Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Wed, May 16, 2018 at 11:59:31AM +0200, Erik Skultety wrote:
On Mon, May 14, 2018 at 02:53:49PM +0100, Daniel P. Berrangé wrote:
Historically we matched log filters with strstr(), and when switching to fnmatch in cbb0fd3cfdc287f6f4653ef1f04a7cfb2ea51b27, it was stated that we would continue to match substrings, with "foo" being equivalent to "*foo*". Unfortuntely I forget to provide the code to actually make that happen. This fixes it to prepend and append "*" if not already present in the user's pattern.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/util/virlog.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/util/virlog.c b/src/util/virlog.c index be9fc0cf78..d548010b10 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1409,6 +1409,8 @@ virLogFilterNew(const char *match, { virLogFilterPtr ret = NULL; char *mdup = NULL; + size_t mlen = strlen(match); + size_t off = 0;
virCheckFlags(VIR_LOG_STACK_TRACE, NULL);
@@ -1418,9 +1420,19 @@ virLogFilterNew(const char *match, return NULL; }
- if (VIR_STRDUP_QUIET(mdup, match) < 0) + if (VIR_ALLOC_N_QUIET(mdup, mlen + 3) < 0) return NULL;
At first glance it's not very clear why we need the hunk below, so I'd add a comment stating that we treat a match 'foo' to be equal to '*foo*' for the purposes of fnmatch() use.
Also, now that I see commit 8ccee910f5, I know we say we treat the match always as substring, but I'd go even further and enhance the sentence in that config (could be a separate patch) that not just 'foo', but also '*foo' and 'foo*' are in fact equal to '*foo*' for that matter. But this config change is not a deal-breaker to me.
+ if (match[0] != '*') { + mdup[off++] = '*'; + }
^Braces around a single line body...
+ memcpy(mdup + off, match, mlen + 1);
Why do you copy the null byte too? 'mlen' should suffice.
Oh true, I forgot that.
+ off += mlen; + if (match[mlen - 1] != '*') { + mdup[off++] = '*'; + mdup[off++] = '\0';
You won't need ^this extra null byte assignment, because we always use calloc to allocate a block.
Yep.
+ } + if (VIR_ALLOC_QUIET(ret) < 0) { VIR_FREE(mdup); return NULL;
With the nit-picks fixed and a commentary added right before the hunk this patch adds: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Thanks, will do. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrangé -
Erik Skultety