[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 1/3] Add method for checking if a string is (probably) a log message



On Thu, Mar 07, 2013 at 03:50:48PM +0100, Peter Krempa wrote:
> On 03/06/13 17:16, Daniel P. Berrange wrote:
> >From: "Daniel P. Berrange" <berrange redhat com>
> >
> >When reading log output from QEMU/LXC we need to skip over any
> >libvirt log messages. Currently the QEMU driver checks for a
> >fixed string, but this is better done with a regex. Add a method
> >virLogProbablyLogMessage to do a regex check
> >
> >Signed-off-by: Daniel P. Berrange <berrange redhat com>
> >---
> >  src/libvirt_private.syms |  1 +
> >  src/util/virlog.c        | 37 +++++++++++++++++++++++++++++++++++++
> >  src/util/virlog.h        |  3 +++
> >  3 files changed, 41 insertions(+)
> >
> >diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> >index ed46479..599b71e 100644
> >--- a/src/libvirt_private.syms
> >+++ b/src/libvirt_private.syms
> >@@ -1429,6 +1429,7 @@ virLogMessage;
> >  virLogParseDefaultPriority;
> >  virLogParseFilters;
> >  virLogParseOutputs;
> >+virLogProbablyLogMessage;
> >  virLogReset;
> >  virLogSetBufferSize;
> >  virLogSetDefaultPriority;
> >diff --git a/src/util/virlog.c b/src/util/virlog.c
> >index 24ec9d3..6a1adca 100644
> >--- a/src/util/virlog.c
> >+++ b/src/util/virlog.c
> >@@ -32,6 +32,7 @@
> >  #include <unistd.h>
> >  #include <signal.h>
> >  #include <execinfo.h>
> >+#include <regex.h>
> >  #if HAVE_SYSLOG_H
> >  # include <syslog.h>
> >  #endif
> >@@ -75,6 +76,17 @@ static char *virLogBuffer = NULL;
> >  static int virLogLen = 0;
> >  static int virLogStart = 0;
> >  static int virLogEnd = 0;
> >+static regex_t *virLogRegex = NULL;
> 
> This pointer will probably need to be whitelisted in
> coverity/valgrind for leaking it's contents.
> 
> >+
> >+
> >+#define VIR_LOG_DATE_REGEX "[0-9][0-9][0-9][0-9]-[0-9][0-9]-[0-9][0-9]"
> >+#define VIR_LOG_TIME_REGEX "[0-9][0-9]:[0-9][0-9]:[0-9][0-9].[0-9][0-9][0-9]+[0-9][0-9][0-9][0-9]"
> >+#define VIR_LOG_PID_REGEX "[0-9]+"
> >+#define VIR_LOG_LEVEL_REGEX "debug|info|warning|error"
> >+
> >+#define VIR_LOG_REGEX \
> >+    VIR_LOG_DATE_REGEX " " VIR_LOG_TIME_REGEX ": " \
> >+    VIR_LOG_PID_REGEX ": " VIR_LOG_LEVEL_REGEX " : "
> >
> >  /*
> >   * Filters are used to refine the rules on what to keep or drop
> >@@ -209,6 +221,12 @@ virLogOnceInit(void)
> >      virLogStart = 0;
> >      virLogEnd = 0;
> >      virLogDefaultPriority = VIR_LOG_DEFAULT;
> >+
> >+    if (VIR_ALLOC(virLogRegex) >= 0) {
> >+        if (regcomp(virLogRegex, VIR_LOG_REGEX, REG_EXTENDED) != 0)
> >+            VIR_FREE(virLogRegex);
> >+    }
> >+
> >      virLogUnlock();
> >      if (pbm)
> >          VIR_WARN("%s", pbm);
> >@@ -1587,3 +1605,22 @@ virLogSetFromEnv(void)
> >      if (debugEnv && *debugEnv)
> >          virLogParseOutputs(debugEnv);
> >  }
> >+
> >+
> >+/*
> >+ * Returns a true value if the first line in @str is
> >+ * probably a log message generated by the libvirt
> >+ * logging layer
> >+ */
> >+bool virLogProbablyLogMessage(const char *str)
> >+{
> >+    bool ret = false;
> >+    if (!virLogRegex)
> >+        return false;
> >+    virLogLock();
> >+    if (regexec(virLogRegex, str, 0, NULL, 0) == 0)
> 
> regexec() is touching the compiled regex while processing it? The
> manpage didn't enlighten me enough to say if we really need the
> lock.
> 
> Or is the lock here to protect the moment after the regex memory is
> allocated but the expression isn't compiled yet? Wouldn't it be
> possible then to rearrange the initializer to assign the value to
> the global pointer as the last step to avoid the lock here?

Linux manpages didn't say anything about thread safety, but the opengroup
docs clarify it

  "The interface is defined so that the matched substrings rm_sp and
   rm_ep are in a separate regmatch_t structure instead of in regex_t.
   This allows a single compiled RE to be used simultaneously in several
   contexts; in main() and a signal handler, perhaps, or in multiple
   threads of lightweight processes"

so we can remove the lock safely.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]