1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] log: actually do substring matches with fnmatch

Daniel P. Berrangé posted 1 patch 5 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180514135349.19406-1-berrange@redhat.com
Test syntax-check failed
There is a newer version of this series
src/util/virlog.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
[libvirt] [PATCH] log: actually do substring matches with fnmatch
Posted by Daniel P. Berrangé 5 years, 11 months 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 5 years, 11 months 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é 5 years, 11 months 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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.