[libvirt] [PATCH] Fix logging in libvirt_lxc controller
The lxc controller can't see libvirtd's log level setting so it needs to re-query it from the environment. The parsing code has a few users now, so I added a new function to the internal API, virLogParseDefaultPriority() along the lines of the other parse functions. Signed-off-by: Amy Griffis <amy.griffis@hp.com> diff --git a/qemud/qemud.c b/qemud/qemud.c index a58a767..bd17ccb 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -2465,16 +2465,8 @@ qemudSetLogging(virConfPtr conf, const char *filename) { */ GET_CONF_INT (conf, filename, log_level); debugEnv = getenv("LIBVIRT_DEBUG"); - if (debugEnv && *debugEnv && *debugEnv != '0') { - if (STREQ(debugEnv, "2") || STREQ(debugEnv, "info")) - log_level = VIR_LOG_INFO; - else if (STREQ(debugEnv, "3") || STREQ(debugEnv, "warning")) - log_level = VIR_LOG_WARN; - else if (STREQ(debugEnv, "4") || STREQ(debugEnv, "error")) - log_level = VIR_LOG_ERROR; - else - log_level = VIR_LOG_DEBUG; - } + if (debugEnv && *debugEnv && *debugEnv != '0') + log_level = virLogParseDefaultPriority(debugEnv); if ((verbose) && (log_level >= VIR_LOG_WARN)) log_level = VIR_LOG_INFO; virLogSetDefaultPriority(log_level); diff --git a/src/libvirt.c b/src/libvirt.c index bf49018..a3f36da 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -258,6 +258,7 @@ virInitialize(void) { #ifdef ENABLE_DEBUG char *debugEnv; + int logPrio; #endif if (initialized) return(0); @@ -272,14 +273,8 @@ virInitialize(void) #ifdef ENABLE_DEBUG debugEnv = getenv("LIBVIRT_DEBUG"); if (debugEnv && *debugEnv && *debugEnv != '0') { - if (STREQ(debugEnv, "2") || STREQ(debugEnv, "info")) - virLogSetDefaultPriority(VIR_LOG_INFO); - else if (STREQ(debugEnv, "3") || STREQ(debugEnv, "warning")) - virLogSetDefaultPriority(VIR_LOG_WARN); - else if (STREQ(debugEnv, "4") || STREQ(debugEnv, "error")) - virLogSetDefaultPriority(VIR_LOG_ERROR); - else - virLogSetDefaultPriority(VIR_LOG_DEBUG); + logPrio = virLogParseDefaultPriority(debugEnv); + virLogSetDefaultPriority(logPrio); } debugEnv = getenv("LIBVIRT_LOG_FILTERS"); if (debugEnv) diff --git a/src/libvirt_debug.syms b/src/libvirt_debug.syms index 1742a0b..539d879 100644 --- a/src/libvirt_debug.syms +++ b/src/libvirt_debug.syms @@ -12,6 +12,7 @@ virLogMessage; virLogSetDefaultPriority; virLogDefineFilter; virLogDefineOutput; +virLogParseDefaultPriority; virLogParseFilters; virLogParseOutputs; virLogStartup; diff --git a/src/logging.c b/src/logging.c index 83e07cc..c72d52e 100644 --- a/src/logging.c +++ b/src/logging.c @@ -793,5 +793,30 @@ int virLogParseFilters(const char *filters) { } return(ret); } + +/** + * virLogParseDefaultPriority: + * @priority: string defining the desired logging behavior + * + * It can take a string or number corresponding to the following log + * levels: + * 1: DEBUG + * 2: INFO + * 3: WARNING + * 4: ERROR + * + * DEBUG is returned if @priority doesn't match any of the other values. + */ +int virLogParseDefaultPriority(const char *priority) { + + if (STREQ(priority, "2") || STREQ(priority, "info")) + return VIR_LOG_INFO; + else if (STREQ(priority, "3") || STREQ(priority, "warning")) + return VIR_LOG_WARN; + else if (STREQ(priority, "4") || STREQ(priority, "error")) + return VIR_LOG_ERROR; + else + return VIR_LOG_DEBUG; +} #endif /* ENABLE_DEBUG */ diff --git a/src/logging.h b/src/logging.h index 7ea8935..72b3119 100644 --- a/src/logging.h +++ b/src/logging.h @@ -110,6 +110,7 @@ typedef void (*virLogCloseFunc) (void *data); #ifdef ENABLE_DEBUG +extern int virLogParseDefaultPriority(const char *priority); extern int virLogSetDefaultPriority(int priority); extern int virLogDefineFilter(const char *match, int priority, int flags); extern int virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, @@ -130,6 +131,7 @@ extern void virLogMessage(const char *category, int priority, #else /* ENABLE_DEBUG */ +#define virLogParseDefaultPriority(p) #define virLogSetDefaultPriority(p) #define virLogDefineFilter(m, p, f) #define virLogDefineOutput(func, c, d, p, f) diff --git a/src/lxc_controller.c b/src/lxc_controller.c index 356ecb8..9355d39 100644 --- a/src/lxc_controller.c +++ b/src/lxc_controller.c @@ -594,6 +594,8 @@ int main(int argc, char *argv[]) int monitor = -1; int appPty = -1; int bg = 0; + char *debugEnv; + int logPrio; virCapsPtr caps = NULL; virDomainDefPtr def = NULL; char *configFile = NULL; @@ -735,6 +737,13 @@ int main(int argc, char *argv[]) } } + /* New programs need to re-query and set the libvirt log level. */ + debugEnv = getenv("LIBVIRT_DEBUG"); + if (debugEnv && *debugEnv && *debugEnv != '0') { + logPrio = virLogParseDefaultPriority(debugEnv); + virLogSetDefaultPriority(logPrio); + } + /* Accept initial client which is the libvirtd daemon */ if ((client = accept(monitor, NULL, 0)) < 0) { virReportSystemError(NULL, errno, "%s",
On 06/16/2009 01:35 PM, Amy Griffis wrote:
The lxc controller can't see libvirtd's log level setting so it needs to re-query it from the environment. The parsing code has a few users now, so I added a new function to the internal API, virLogParseDefaultPriority() along the lines of the other parse functions.
I'd say go the extra step and add something like virLogSetFromEnv, which encapsulates the duplicate getenv calls as well. There is also another duplicate of this code in tests/eventtest.c. Thanks, Cole
Cole Robinson wrote: [Tue Jun 16 2009, 02:44:28PM EDT]
On 06/16/2009 01:35 PM, Amy Griffis wrote:
The lxc controller can't see libvirtd's log level setting so it needs to re-query it from the environment. The parsing code has a few users now, so I added a new function to the internal API, virLogParseDefaultPriority() along the lines of the other parse functions.
I'd say go the extra step and add something like virLogSetFromEnv, which encapsulates the duplicate getenv calls as well.
I thought about that, but wanted to keep consistent behavior with the other two parsing routines. I think we could go ahead and change all of them to include the getenv(). Only minor gotcha is qemud wants to call virLogParseOutputs() with it's own string in one case. So we'd need to make the getenv() conditional on not getting an input string. I think this would be cleaner. What do you think?
There is also another duplicate of this code in tests/eventtest.c.
Oops, missed that one. Thanks. Amy
On 06/16/2009 06:12 PM, Amy Griffis wrote:
Cole Robinson wrote: [Tue Jun 16 2009, 02:44:28PM EDT]
On 06/16/2009 01:35 PM, Amy Griffis wrote:
The lxc controller can't see libvirtd's log level setting so it needs to re-query it from the environment. The parsing code has a few users now, so I added a new function to the internal API, virLogParseDefaultPriority() along the lines of the other parse functions. I'd say go the extra step and add something like virLogSetFromEnv, which encapsulates the duplicate getenv calls as well.
I thought about that, but wanted to keep consistent behavior with the other two parsing routines. I think we could go ahead and change all of them to include the getenv(). Only minor gotcha is qemud wants to call virLogParseOutputs() with it's own string in one case. So we'd need to make the getenv() conditional on not getting an input string. I think this would be cleaner. What do you think?
My preference would be to make both virLogParseDefaultPriority and virLogSetFromEnv, rather than have one function with 2 different results. - Cole
On Tue, Jun 16, 2009 at 06:12:04PM -0400, Amy Griffis wrote:
Cole Robinson wrote: [Tue Jun 16 2009, 02:44:28PM EDT]
On 06/16/2009 01:35 PM, Amy Griffis wrote:
The lxc controller can't see libvirtd's log level setting so it needs to re-query it from the environment. The parsing code has a few users now, so I added a new function to the internal API, virLogParseDefaultPriority() along the lines of the other parse functions.
I'd say go the extra step and add something like virLogSetFromEnv, which encapsulates the duplicate getenv calls as well.
I thought about that, but wanted to keep consistent behavior with the other two parsing routines. I think we could go ahead and change all of them to include the getenv(). Only minor gotcha is qemud wants to call virLogParseOutputs() with it's own string in one case. So we'd need to make the getenv() conditional on not getting an input string. I think this would be cleaner. What do you think?
I think there's valid reasons for both suggested APIs. I'd suggest a slight variation on your original API, instead of: + logPrio = virLogParseDefaultPriority(debugEnv); + virLogSetDefaultPriority(logPrio); Just have a 'virLogSetDefaultPriorityFromString(char *str)', eg so it'd parse the string and then call virLogSetDefaultPriority for you. Also then having a virLogSetFromEnv() method which uses getenv to fetch the vars and then calls the virLogSet... methods Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange wrote: [Wed Jun 17 2009, 06:44:31AM EDT]
On Tue, Jun 16, 2009 at 06:12:04PM -0400, Amy Griffis wrote:
Cole Robinson wrote: [Tue Jun 16 2009, 02:44:28PM EDT]
On 06/16/2009 01:35 PM, Amy Griffis wrote:
The lxc controller can't see libvirtd's log level setting so it needs to re-query it from the environment. The parsing code has a few users now, so I added a new function to the internal API, virLogParseDefaultPriority() along the lines of the other parse functions.
I'd say go the extra step and add something like virLogSetFromEnv, which encapsulates the duplicate getenv calls as well.
I thought about that, but wanted to keep consistent behavior with the other two parsing routines. I think we could go ahead and change all of them to include the getenv(). Only minor gotcha is qemud wants to call virLogParseOutputs() with it's own string in one case. So we'd need to make the getenv() conditional on not getting an input string. I think this would be cleaner. What do you think?
I think there's valid reasons for both suggested APIs.
I'd suggest a slight variation on your original API, instead of:
+ logPrio = virLogParseDefaultPriority(debugEnv); + virLogSetDefaultPriority(logPrio);
Just have a 'virLogSetDefaultPriorityFromString(char *str)', eg so it'd parse the string and then call virLogSetDefaultPriority for you.
Since the config file parser in libvirtd is returning an integer, it actually needs one function to get the environment setting and convert it to integer, and then a second function to set the value in the log module from an integer.
Also then having a virLogSetFromEnv() method which uses getenv to fetch the vars and then calls the virLogSet... methods
But I see what you guys are saying. I'll make these changes and send a new patch. Thanks, Amy
Taking a second look at the log level code and documentation, there seems to be some confusion around the value of '0'. It's not one of the defined log priorities, but it is mentioned in the documentation on the website both as meaning "log everything" and "no logging at all". In the code, there are times when it's accepted (for outputs), rejected (for filters) and ignored (for the loglevel). For the outputs, specifying level 0 is effectively the same as specifying level 1. I'm wondering if there is any historical reason for the '0' value, or if we could just do away with it completely? Thanks, Amy
On Thu, Jun 18, 2009 at 01:25:46PM -0400, Amy Griffis wrote:
Taking a second look at the log level code and documentation, there seems to be some confusion around the value of '0'. It's not one of the defined log priorities, but it is mentioned in the documentation on the website both as meaning "log everything" and "no logging at all".
In the code, there are times when it's accepted (for outputs), rejected (for filters) and ignored (for the loglevel). For the outputs, specifying level 0 is effectively the same as specifying level 1.
I'm wondering if there is any historical reason for the '0' value, or if we could just do away with it completely?
Well I think we should clean this up, yes it's a bit of a mess and I think that's the kind of things were we can clean things without really breaking compatibility. I would map it uniformly as same as 1, and clean up the documentation to remove any reference. I guess that's the best considering the current state. If you feel you can work out a patch, that would be great :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (4)
-
Amy Griffis -
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard