Changeset
src/util/virlog.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
Git apply log
Switched to a new branch '20180514135349.19406-1-berrange@redhat.com'
Applying: log: actually do substring matches with fnmatch
To https://github.com/patchew-project/libvirt
 + 2c2045d...5e8bd98 patchew/20180514135349.19406-1-berrange@redhat.com -> patchew/20180514135349.19406-1-berrange@redhat.com (forced update)
Test failed: syntax-check

loading

[libvirt] [PATCH] log: actually do substring matches with fnmatch
Posted by Daniel P. Berrangé, 1 week ago
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] log: actually do substring matches with fnmatch
Posted by Erik Skultety, 6 days ago
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>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] log: actually do substring matches with fnmatch
Posted by Daniel P. Berrangé, 6 days ago
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list