Daniel P. Berrange wrote: [Mon Oct 05 2009, 08:51:00AM EDT]

On Sun, Oct 04, 2009 at 03:28:46PM -0400, Amy Griffis wrote:

...

When configuring logging settings, keep more information about the output destination. Add accessors to retrieve the filter and output settings in the original string form; this to be used to set up environment for a child process that also logs. Open output files O_APPEND so child can also write -- was there a reason to truncate them?

I don't think it really matters - APPEND is fine

...

Note this patch changes the API for virLogDefineOutput(), which is part of the internal libvirt API, but is currently only used within logging.c.

...

@@ -107,12 +114,15 @@ typedef void (*virLogCloseFunc) (void *data);

extern int virLogGetNbFilters(void); extern int virLogGetNbOutputs(void); +extern int virLogGetFilters(virBufferPtr); +extern int virLogGetOutputs(virBufferPtr);

Having looked at the wway the next patch uses these, I think it'd be nicer to change the contract to just be

extern char *virLogGetFilters(void); extern char *virLogGetOutputs(void);

Heh, that's how I wrote it the first time. Then I changed it to make use of the virBuffer API, and tried to follow precedent with the rest of libvirt. The code is not really doing much with the string, maybe virBuffer is overkill? Amy

Daniel P. Berrange wrote: [Tue Oct 06 2009, 05:00:55AM EDT]

...

...

Having looked at the wway the next patch uses these, I think it'd be nicer to change the contract to just be

extern char *virLogGetFilters(void); extern char *virLogGetOutputs(void);

Heh, that's how I wrote it the first time. Then I changed it to make use of the virBuffer API, and tried to follow precedent with the rest of libvirt. The code is not really doing much with the string, maybe virBuffer is overkill?

The general rule to try & follow is that if you just have a single printf style call to make, then use virAsprintf. If you have several printf/strcat calls to make, then use virBuffer.

In this particular case, its fine to use virBuffer for your internal impl - I just thing its better to not include it in the public API, convert the virBuffer into a char * for the return value

That makes sense. Does this look good now? --- When configuring logging settings, keep more information about the output destination. Add accessors to retrieve the filter and output settings in the original string form; this to be used to set up environment for a child process that also logs. Open output files O_APPEND so child can also write -- was there a reason to truncate them? Note this patch changes the API for virLogDefineOutput(), which is part of the internal libvirt API, but is currently only used within logging.c. --- src/libvirt_private.syms | 2 + src/util/logging.c | 112 +++++++++++++++++++++++++++++++++++++++++++--- src/util/logging.h | 14 +++++- 3 files changed, 119 insertions(+), 9 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fb9b9ae..732fd08 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -253,6 +253,8 @@ virRegisterSecretDriver; virLogMessage; virLogGetNbFilters; virLogGetNbOutputs; +virLogGetFilters; +virLogGetOutputs; virLogGetDefaultPriority; virLogSetDefaultPriority; virLogSetFromEnv; diff --git a/src/util/logging.c b/src/util/logging.c index 07c2b0e..4899de6 100644 --- a/src/util/logging.c +++ b/src/util/logging.c @@ -37,6 +37,7 @@ #include "logging.h" #include "memory.h" #include "util.h" +#include "buf.h" #include "threads.h" /* @@ -109,6 +110,8 @@ struct _virLogOutput { virLogOutputFunc f; virLogCloseFunc c; int priority; + virLogDestination dest; + const char *name; }; typedef struct _virLogOutput virLogOutput; typedef virLogOutput *virLogOutputPtr; @@ -138,6 +141,17 @@ static void virLogUnlock(void) virMutexUnlock(&virLogMutex); } +static const char *virLogOutputString(virLogDestination ldest) { + switch (ldest) { + case VIR_LOG_TO_STDERR: + return("stderr"); + case VIR_LOG_TO_SYSLOG: + return("syslog"); + case VIR_LOG_TO_FILE: + return("file"); + } + return("unknown"); +} static const char *virLogPriorityString(virLogPriority lvl) { switch (lvl) { @@ -428,6 +442,7 @@ static int virLogResetOutputs(void) { for (i = 0;i < virLogNbOutputs;i++) { if (virLogOutputs[i].c != NULL) virLogOutputs[i].c(virLogOutputs[i].data); + VIR_FREE(virLogOutputs[i].name); } VIR_FREE(virLogOutputs); i = virLogNbOutputs; @@ -438,9 +453,11 @@ static int virLogResetOutputs(void) { /** * virLogDefineOutput: * @f: the function to call to output a message - * @f: the function to call to close the output (or NULL) + * @c: the function to call to close the output (or NULL) * @data: extra data passed as first arg to the function * @priority: minimal priority for this filter, use 0 for none + * @dest: where to send output of this priority + * @name: optional name data associated with an output * @flags: extra flag, currently unused * * Defines an output function for log messages. Each message once @@ -449,12 +466,22 @@ static int virLogResetOutputs(void) { * Returns -1 in case of failure or the output number if successful */ int virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, void *data, - int priority, int flags ATTRIBUTE_UNUSED) { + int priority, int dest, const char *name, + int flags ATTRIBUTE_UNUSED) { int ret = -1; + char *ndup = NULL; if (f == NULL) return(-1); + if (dest == VIR_LOG_TO_SYSLOG || dest == VIR_LOG_TO_FILE) { + if (name == NULL) + return(-1); + ndup = strdup(name); + if (ndup == NULL) + return(-1); + } + virLogLock(); if (VIR_REALLOC_N(virLogOutputs, virLogNbOutputs + 1)) { goto cleanup; @@ -464,6 +491,8 @@ int virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, void *data, virLogOutputs[ret].c = c; virLogOutputs[ret].data = data; virLogOutputs[ret].priority = priority; + virLogOutputs[ret].dest = dest; + virLogOutputs[ret].name = ndup; cleanup: virLogUnlock(); return(ret); @@ -577,7 +606,8 @@ static void virLogCloseFd(void *data) { } static int virLogAddOutputToStderr(int priority) { - if (virLogDefineOutput(virLogOutputToFd, NULL, (void *)2L, priority, 0) < 0) + if (virLogDefineOutput(virLogOutputToFd, NULL, (void *)2L, priority, + VIR_LOG_TO_STDERR, NULL, 0) < 0) return(-1); return(0); } @@ -585,11 +615,11 @@ static int virLogAddOutputToStderr(int priority) { static int virLogAddOutputToFile(int priority, const char *file) { int fd; - fd = open(file, O_CREAT | O_TRUNC | O_WRONLY, S_IRUSR | S_IWUSR); + fd = open(file, O_CREAT | O_APPEND | O_WRONLY, S_IRUSR | S_IWUSR); if (fd < 0) return(-1); if (virLogDefineOutput(virLogOutputToFd, virLogCloseFd, (void *)(long)fd, - priority, 0) < 0) { + priority, VIR_LOG_TO_FILE, file, 0) < 0) { close(fd); return(-1); } @@ -643,7 +673,7 @@ static int virLogAddOutputToSyslog(int priority, const char *ident) { openlog(current_ident, 0, 0); if (virLogDefineOutput(virLogOutputToSyslog, virLogCloseSyslog, NULL, - priority, 0) < 0) { + priority, VIR_LOG_TO_SYSLOG, ident, 0) < 0) { closelog(); VIR_FREE(current_ident); return(-1); @@ -682,6 +712,7 @@ static int virLogAddOutputToSyslog(int priority, const char *ident) { int virLogParseOutputs(const char *outputs) { const char *cur = outputs, *str; char *name; + char *abspath; int prio; int ret = -1; int count = 0; @@ -732,9 +763,14 @@ int virLogParseOutputs(const char *outputs) { name = strndup(str, cur - str); if (name == NULL) goto cleanup; - if (virLogAddOutputToFile(prio, name) == 0) + if (virFileAbsPath(name, &abspath) < 0) { + VIR_FREE(name); + return -1; /* skip warning here because setting was fine */ + } + if (virLogAddOutputToFile(prio, abspath) == 0) count++; VIR_FREE(name); + VIR_FREE(abspath); } else { goto cleanup; } @@ -813,6 +849,68 @@ int virLogGetDefaultPriority(void) { } /** + * virLogGetFilters: + * + * Returns a string listing the current filters, in the format originally + * specified in the config file or environment. Caller must free the + * result. + */ +char *virLogGetFilters(void) { + int i; + virBuffer filterbuf = VIR_BUFFER_INITIALIZER; + + virLogLock(); + for (i = 0; i < virLogNbFilters; i++) { + virBufferVSprintf(&filterbuf, "%d:%s ", virLogFilters[i].priority, + virLogFilters[i].match); + } + virLogUnlock(); + + if (virBufferError(&filterbuf)) + return NULL; + + return virBufferContentAndReset(&filterbuf); +} + +/** + * virLogGetOutputs: + * + * Returns a string listing the current outputs, in the format originally + * specified in the config file or environment. Caller must free the + * result. + */ +char *virLogGetOutputs(void) { + int i; + virBuffer outputbuf = VIR_BUFFER_INITIALIZER; + + virLogLock(); + for (i = 0; i < virLogNbOutputs; i++) { + int dest = virLogOutputs[i].dest; + if (i) + virBufferVSprintf(&outputbuf, " "); + switch (dest) { + case VIR_LOG_TO_SYSLOG: + case VIR_LOG_TO_FILE: + virBufferVSprintf(&outputbuf, "%d:%s:%s", + virLogOutputs[i].priority, + virLogOutputString(dest), + virLogOutputs[i].name); + break; + default: + virBufferVSprintf(&outputbuf, "%d:%s", + virLogOutputs[i].priority, + virLogOutputString(dest)); + } + } + virLogUnlock(); + + if (virBufferError(&outputbuf)) + return NULL; + + return virBufferContentAndReset(&outputbuf); +} + +/** * virLogGetNbFilters: * * Returns the current number of defined log filters. diff --git a/src/util/logging.h b/src/util/logging.h index 8b2b84c..49e2f6d 100644 --- a/src/util/logging.h +++ b/src/util/logging.h @@ -23,6 +23,7 @@ #define __VIRTLOG_H_ #include "internal.h" +#include "buf.h" /* * If configured with --enable-debug=yes then library calls @@ -79,6 +80,12 @@ typedef enum { #define VIR_LOG_DEFAULT VIR_LOG_WARN +typedef enum { + VIR_LOG_TO_STDERR = 1, + VIR_LOG_TO_SYSLOG, + VIR_LOG_TO_FILE, +} virLogDestination; + /** * virLogOutputFunc: * @category: the category for the message @@ -107,12 +114,15 @@ typedef void (*virLogCloseFunc) (void *data); extern int virLogGetNbFilters(void); extern int virLogGetNbOutputs(void); +extern char *virLogGetFilters(void); +extern char *virLogGetOutputs(void); extern int virLogGetDefaultPriority(void); extern int virLogSetDefaultPriority(int priority); extern void virLogSetFromEnv(void); extern int virLogDefineFilter(const char *match, int priority, int flags); -extern int virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, - void *data, int priority, int flags); +extern int virLogDefineOutput(virLogOutputFunc f, virLogCloseFunc c, void *data, + int priority, int dest, const char *name, + int flags); /* * Internal logging API

On Sun, Oct 04, 2009 at 03:28:54PM -0400, Amy Griffis wrote:

Some callers may have set envp[]. ---

src/util/util.c | 13 ++++++++++++- 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/src/util/util.c b/src/util/util.c index 81b743c..e5135fc 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -601,12 +601,23 @@ virExecWithHook(virConnectPtr conn, char *pidfile) { char *argv_str; + char *envp_str;

if ((argv_str = virArgvToString(argv)) == NULL) { virReportOOMError(conn); return -1; } - DEBUG0(argv_str); + + if (envp) { + if ((envp_str = virArgvToString(envp)) == NULL) { + virReportOOMError(conn); + return -1; + } + VIR_DEBUG("%s %s", envp_str, argv_str); + VIR_FREE(envp_str); + } else { + VIR_DEBUG0(argv_str); + } VIR_FREE(argv_str);

return __virExec(conn, argv, envp, keepfd, retpid, infd, outfd, errfd,

ACK 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: [Mon Oct 05 2009, 08:52:20AM EDT]

ACK, although it'd need a small change for my suggested API contract change in your previous patch in the series

Updated for API change... Before launching the lxc controller, have the lxc driver query the log settings and setup envp[]. This provides the advantage of honoring the actual log configuration instead of only what had been set in the environment. The lxc controller now simply has to call virLogSetFromEnv(). --- src/lxc/lxc_controller.c | 3 ++ src/lxc/lxc_driver.c | 77 ++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 70 insertions(+), 10 deletions(-) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index bca721b..545f718 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -793,6 +793,9 @@ int main(int argc, char *argv[]) } } + /* Initialize logging */ + virLogSetFromEnv(); + /* Accept initial client which is the libvirtd daemon */ if ((client = accept(monitor, NULL, 0)) < 0) { virReportSystemError(NULL, errno, "%s", diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 5fb4105..f79d109 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -830,9 +830,13 @@ static int lxcControllerStart(virConnectPtr conn, { int i; int rc; - int ret = -1; int largc = 0, larga = 0; const char **largv = NULL; + int lenvc = 0, lenva = 0; + const char **lenv = NULL; + char *filterstr; + char *outputstr; + char *tmp; pid_t child; int status; fd_set keepfd; @@ -863,6 +867,52 @@ static int lxcControllerStart(virConnectPtr conn, goto no_memory; \ } while (0) +#define ADD_ENV_SPACE \ + do { \ + if (lenvc == lenva) { \ + lenva += 10; \ + if (VIR_REALLOC_N(lenv, lenva) < 0) \ + goto no_memory; \ + } \ + } while (0) + +#define ADD_ENV(thisarg) \ + do { \ + ADD_ENV_SPACE; \ + lenv[lenvc++] = thisarg; \ + } while (0) + +#define ADD_ENV_PAIR(envname, val) \ + do { \ + char *envval; \ + ADD_ENV_SPACE; \ + if (virAsprintf(&envval, "%s=%s", envname, val) < 0) \ + goto no_memory; \ + lenv[lenvc++] = envval; \ + } while (0) + + if (virAsprintf(&tmp, "LIBVIRT_DEBUG=%d", virLogGetDefaultPriority()) < 0) + goto no_memory; + ADD_ENV(tmp); + + if (virLogGetNbFilters() > 0) { + filterstr = virLogGetFilters(); + if (!filterstr) + goto no_memory; + ADD_ENV_PAIR("LIBVIRT_LOG_FILTERS", filterstr); + VIR_FREE(filterstr); + } + + if (virLogGetNbOutputs() > 0) { + outputstr = virLogGetOutputs(); + if (!outputstr) + goto no_memory; + ADD_ENV_PAIR("LIBVIRT_LOG_OUTPUTS", outputstr); + VIR_FREE(outputstr); + } + + ADD_ENV(NULL); + snprintf(appPtyStr, sizeof(appPtyStr), "%d", appPty); emulator = vm->def->emulator; @@ -883,7 +933,7 @@ static int lxcControllerStart(virConnectPtr conn, FD_SET(appPty, &keepfd); - if (virExec(conn, largv, NULL, &keepfd, &child, + if (virExec(conn, largv, lenv, &keepfd, &child, -1, &logfd, &logfd, VIR_EXEC_NONE) < 0) goto cleanup; @@ -910,18 +960,25 @@ static int lxcControllerStart(virConnectPtr conn, #undef ADD_ARG #undef ADD_ARG_LIT #undef ADD_ARG_SPACE +#undef ADD_ENV_SPACE +#undef ADD_ENV_PAIR - ret = 0; - -cleanup: - for (i = 0 ; i < largc ; i++) - VIR_FREE(largv[i]); - - return ret; + return 0; no_memory: virReportOOMError(conn); - goto cleanup; +cleanup: + if (largv) { + for (i = 0 ; i < largc ; i++) + VIR_FREE(largv[i]); + VIR_FREE(largv); + } + if (lenv) { + for (i=0 ; i < lenvc ; i++) + VIR_FREE(lenv[i]); + VIR_FREE(lenv); + } + return -1; }

On Sun, Oct 04, 2009 at 03:29:10PM -0400, Amy Griffis wrote:

Do we really want to overwrite the container log file every time we restart? ---

src/lxc/lxc_driver.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index c5a49f2..73cfecd 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1037,7 +1037,7 @@ static int lxcVmStart(virConnectPtr conn, goto cleanup; }

- if ((logfd = open(logfile, O_WRONLY | O_TRUNC | O_CREAT, + if ((logfd = open(logfile, O_WRONLY | O_APPEND | O_CREAT, S_IRUSR|S_IWUSR)) < 0) { virReportSystemError(conn, errno, _("failed to open '%s'"),

ACK Truncation can be dealt with my logrotate in a much more flexible manner 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 :|

Updated for API change. Add a config file for the lxc driver, based on existing qemu.conf. There is currently one tunable "log_with_libvirtd" that controls whether an lxc controller will log only to the container log file, or whether it will honor libvirtd's log output configuration. This provides a way to have libvirtd and its children log to a single file. The default is to log to the container log file. --- libvirt.spec.in | 6 ++++++ src/Makefile.am | 9 +++++++-- src/lxc/lxc.conf | 13 +++++++++++++ src/lxc/lxc_conf.c | 24 ++++++++++++++++++++++++ src/lxc/lxc_conf.h | 1 + src/lxc/lxc_driver.c | 22 ++++++++++++++++------ 6 files changed, 67 insertions(+), 8 deletions(-) create mode 100644 src/lxc/lxc.conf diff --git a/libvirt.spec.in b/libvirt.spec.in index da3b2a7..725ef60 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -550,6 +550,9 @@ rm -rf $RPM_BUILD_ROOT%{_datadir}/doc/libvirt-%{version} %if ! %{with_qemu} rm -rf $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/qemu.conf %endif +%if ! %{with_lxc} +rm -rf $RPM_BUILD_ROOT%{_sysconfdir}/libvirt/lxc.conf +%endif %if %{with_libvirtd} chmod 0644 $RPM_BUILD_ROOT%{_sysconfdir}/sysconfig/libvirtd @@ -627,6 +630,9 @@ fi %if %{with_qemu} %config(noreplace) %{_sysconfdir}/libvirt/qemu.conf %endif +%if %{with_lxc} +%config(noreplace) %{_sysconfdir}/libvirt/lxc.conf +%endif %dir %{_datadir}/libvirt/ diff --git a/src/Makefile.am b/src/Makefile.am index f3d4559..73bbb70 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -32,6 +32,8 @@ lib_LTLIBRARIES = libvirt.la moddir = $(libdir)/libvirt/drivers mod_LTLIBRARIES = +confdir = $(sysconfdir)/libvirt +conf_DATA = # These files are not related to driver APIs. Simply generic # helper APIs for various purposes @@ -425,8 +427,7 @@ libvirt_driver_qemu_la_LDFLAGS += -module -avoid-version endif libvirt_driver_qemu_la_SOURCES = $(QEMU_DRIVER_SOURCES) -confdir = $(sysconfdir)/libvirt/ -conf_DATA = qemu/qemu.conf +conf_DATA += qemu/qemu.conf augeasdir = $(datadir)/augeas/lenses augeas_DATA = qemu/libvirtd_qemu.aug @@ -456,7 +457,11 @@ if WITH_DRIVER_MODULES libvirt_driver_lxc_la_LDFLAGS = -module -avoid-version endif libvirt_driver_lxc_la_SOURCES = $(LXC_DRIVER_SOURCES) + +conf_DATA += lxc/lxc.conf + endif +EXTRA_DIST += lxc/lxc.conf if WITH_UML if WITH_DRIVER_MODULES diff --git a/src/lxc/lxc.conf b/src/lxc/lxc.conf new file mode 100644 index 0000000..7a5066f --- /dev/null +++ b/src/lxc/lxc.conf @@ -0,0 +1,13 @@ +# Master configuration file for the LXC driver. +# All settings described here are optional - if omitted, sensible +# defaults are used. + +# By default, log messages generated by the lxc controller go to the +# container logfile. It is also possible to accumulate log messages +# from all lxc controllers along with libvirtd's log outputs. In this +# case, the lxc controller will honor either LIBVIRT_LOG_OUTPUTS or +# log_outputs from libvirtd.conf. +# +# This is disabled by default, uncomment below to enable it. +# +# log_with_libvirtd = 1 diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index fef60ba..de059bd 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -30,6 +30,7 @@ #include "lxc_conf.h" #include "nodeinfo.h" #include "virterror_internal.h" +#include "conf.h" #include "logging.h" @@ -90,6 +91,10 @@ no_memory: int lxcLoadDriverConfig(lxc_driver_t *driver) { + char *filename; + virConfPtr conf; + virConfValuePtr p; + /* Set the container configuration directory */ if ((driver->configDir = strdup(LXC_CONFIG_DIR)) == NULL) goto no_memory; @@ -98,6 +103,25 @@ int lxcLoadDriverConfig(lxc_driver_t *driver) if ((driver->logDir = strdup(LXC_LOG_DIR)) == NULL) goto no_memory; + if ((filename = strdup(SYSCONF_DIR "/libvirt/lxc.conf")) == NULL) + goto no_memory; + + /* Avoid error from non-existant or unreadable file. */ + if (access (filename, R_OK) == -1) + return 0; + conf = virConfReadFile(filename, 0); + if (!conf) + return 0; + + p = virConfGetValue(conf, "log_with_libvirtd"); + if (p) { + if (p->type != VIR_CONF_LONG) + VIR_WARN0("lxcLoadDriverConfig: invalid setting: log_with_libvirtd"); + else + driver->log_libvirtd = p->l; + } + + virConfFree(conf); return 0; no_memory: diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 15402d8..6e4c855 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -49,6 +49,7 @@ struct __lxc_driver { char *autostartDir; char *stateDir; char *logDir; + int log_libvirtd; int have_netns; /* An array of callbacks */ diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 0780666..91dad7c 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -837,11 +837,13 @@ static int lxcControllerStart(virConnectPtr conn, char *filterstr; char *outputstr; char *tmp; + int log_level; pid_t child; int status; fd_set keepfd; char appPtyStr[30]; const char *emulator; + lxc_driver_t *driver = conn->privateData; FD_ZERO(&keepfd); @@ -891,7 +893,8 @@ static int lxcControllerStart(virConnectPtr conn, lenv[lenvc++] = envval; \ } while (0) - if (virAsprintf(&tmp, "LIBVIRT_DEBUG=%d", virLogGetDefaultPriority()) < 0) + log_level = virLogGetDefaultPriority(); + if (virAsprintf(&tmp, "LIBVIRT_DEBUG=%d", log_level) < 0) goto no_memory; ADD_ENV(tmp); @@ -903,12 +906,18 @@ static int lxcControllerStart(virConnectPtr conn, VIR_FREE(filterstr); } - if (virLogGetNbOutputs() > 0) { - outputstr = virLogGetOutputs(); - if (!outputstr) + if (driver->log_libvirtd) { + if (virLogGetNbOutputs() > 0) { + outputstr = virLogGetOutputs(); + if (!outputstr) + goto no_memory; + ADD_ENV_PAIR("LIBVIRT_LOG_OUTPUTS", outputstr); + VIR_FREE(outputstr); + } + } else { + if (virAsprintf(&tmp, "LIBVIRT_LOG_OUTPUTS=%d:stderr", log_level) < 0) goto no_memory; - ADD_ENV_PAIR("LIBVIRT_LOG_OUTPUTS", outputstr); - VIR_FREE(outputstr); + ADD_ENV(tmp); } ADD_ENV(NULL); @@ -1514,6 +1523,7 @@ static int lxcStartup(int privileged) virEventAddTimeout(-1, lxcDomainEventFlush, lxc_driver, NULL)) < 0) goto cleanup; + lxc_driver->log_libvirtd = 0; /* by default log to container logfile */ lxc_driver->have_netns = lxcCheckNetNsSupport(); rc = virCgroupForDriver("lxc", &lxc_driver->cgroup, privileged, 1);

On Sun, Oct 04, 2009 at 03:29:26PM -0400, Amy Griffis wrote:

I've attempted to add augeas support for lxc.conf based on qemu.conf. diff --git a/src/Makefile.am b/src/Makefile.am index 73bbb70..7e3abab 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -35,6 +35,12 @@ mod_LTLIBRARIES = confdir = $(sysconfdir)/libvirt conf_DATA =

+augeasdir = $(datadir)/augeas/lenses +augeas_DATA = + +augeastestdir = $(datadir)/augeas/lenses/tests +augeastest_DATA = + # These files are not related to driver APIs. Simply generic # helper APIs for various purposes UTIL_SOURCES = \ @@ -429,11 +435,8 @@ libvirt_driver_qemu_la_SOURCES = $(QEMU_DRIVER_SOURCES)

conf_DATA += qemu/qemu.conf

-augeasdir = $(datadir)/augeas/lenses -augeas_DATA = qemu/libvirtd_qemu.aug - -augeastestdir = $(datadir)/augeas/lenses/tests -augeastest_DATA = qemu/test_libvirtd_qemu.aug +augeas_DATA += qemu/libvirtd_qemu.aug +augeastest_DATA += qemu/test_libvirtd_qemu.aug

check-local: test -x '$(AUGPARSE)' \

It'd be good to add to the check-local: test here, so the config file parser is tested as part of 'make check' 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 :|

Ryota Ozaki wrote: [Tue Oct 06 2009, 12:01:30AM EDT]

On Mon, Oct 5, 2009 at 9:55 PM, Daniel P. Berrange <berrange@redhat.com> wrote:

...

It'd be good to add to the check-local:  test here, so the config file parser is tested as part of 'make check'

Oh, the check-local is corrupted.

check-local: test -x '$(AUGPARSE)' \ && '$(AUGPARSE)' -I $(srcdir) $(srcdir)/test_libvirtd_qemu.aug || :

We ought to append qemu/ to $(srcdir).

ozaki-r

I just caught that too. I think this updated patch should fix it. I changed the rule slightly so it won't fail when there's no $(AUGPARSE) but it will fail if it doesn't parse correctly. I think that's what you would want. Amy --- libvirt.spec.in | 10 ++++++++++ src/Makefile.am | 36 ++++++++++++++++++++++++++---------- src/lxc/libvirtd_lxc.aug | 30 ++++++++++++++++++++++++++++++ src/lxc/test_libvirtd_lxc.aug | 31 +++++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 10 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 725ef60..4af1f00 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -539,6 +539,11 @@ rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug %endif %find_lang %{name} +%if ! %{with_lxc} +rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/libvirtd_lxc.aug +rm -f $RPM_BUILD_ROOT%{_datadir}/augeas/lenses/tests/test_libvirtd_lxc.aug +endif + %if ! %{with_python} rm -rf $RPM_BUILD_ROOT%{_datadir}/doc/libvirt-python-%{version} %endif @@ -674,6 +679,11 @@ fi %{_datadir}/augeas/lenses/tests/test_libvirtd_qemu.aug %endif +%if %{with_lxc} +%{_datadir}/augeas/lenses/libvirtd_lxc.aug +%{_datadir}/augeas/lenses/tests/test_libvirtd_lxc.aug +%endif + %{_datadir}/augeas/lenses/libvirtd.aug %{_datadir}/augeas/lenses/tests/test_libvirtd.aug diff --git a/src/Makefile.am b/src/Makefile.am index 73bbb70..689b947 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -35,6 +35,12 @@ mod_LTLIBRARIES = confdir = $(sysconfdir)/libvirt conf_DATA = +augeasdir = $(datadir)/augeas/lenses +augeas_DATA = + +augeastestdir = $(datadir)/augeas/lenses/tests +augeastest_DATA = + # These files are not related to driver APIs. Simply generic # helper APIs for various purposes UTIL_SOURCES = \ @@ -429,15 +435,8 @@ libvirt_driver_qemu_la_SOURCES = $(QEMU_DRIVER_SOURCES) conf_DATA += qemu/qemu.conf -augeasdir = $(datadir)/augeas/lenses -augeas_DATA = qemu/libvirtd_qemu.aug - -augeastestdir = $(datadir)/augeas/lenses/tests -augeastest_DATA = qemu/test_libvirtd_qemu.aug - -check-local: - test -x '$(AUGPARSE)' \ - && '$(AUGPARSE)' -I $(srcdir) $(srcdir)/test_libvirtd_qemu.aug || : +augeas_DATA += qemu/libvirtd_qemu.aug +augeastest_DATA += qemu/test_libvirtd_qemu.aug endif EXTRA_DIST += qemu/qemu.conf qemu/libvirtd_qemu.aug qemu/test_libvirtd_qemu.aug @@ -460,8 +459,11 @@ libvirt_driver_lxc_la_SOURCES = $(LXC_DRIVER_SOURCES) conf_DATA += lxc/lxc.conf +augeas_DATA += lxc/libvirtd_lxc.aug +augeastest_DATA += lxc/test_libvirtd_lxc.aug + endif -EXTRA_DIST += lxc/lxc.conf +EXTRA_DIST += lxc/lxc.conf lxc/libvirtd_lxc.aug lxc/test_libvirtd_lxc.aug if WITH_UML if WITH_DRIVER_MODULES @@ -679,6 +681,20 @@ EXTRA_DIST += \ $(SECRET_DRIVER_SOURCES) \ $(VBOX_DRIVER_EXTRA_DIST) +check-local: +if WITH_QEMU + if test -x '$(AUGPARSE)'; then \ + '$(AUGPARSE)' -I $(srcdir)/qemu \ + $(srcdir)/qemu/test_libvirtd_qemu.aug; \ + fi +endif +if WITH_LXC + if test -x '$(AUGPARSE)'; then \ + '$(AUGPARSE)' -I $(srcdir)/lxc \ + $(srcdir)/lxc/test_libvirtd_lxc.aug; \ + fi +endif + # # Build our version script. This is composed of three parts: # diff --git a/src/lxc/libvirtd_lxc.aug b/src/lxc/libvirtd_lxc.aug new file mode 100644 index 0000000..10f25e4 --- /dev/null +++ b/src/lxc/libvirtd_lxc.aug @@ -0,0 +1,30 @@ +(* /etc/libvirt/lxc.conf *) + +module Libvirtd_lxc = + autoload xfm + + let eol = del /[ \t]*\n/ "\n" + let value_sep = del /[ \t]*=[ \t]*/ " = " + let indent = del /[ \t]*/ "" + + let bool_val = store /0|1/ + + let bool_entry (kw:string) = [ key kw . value_sep . bool_val ] + + + (* Config entry grouped by function - same order as example config *) + let log_entry = bool_entry "log_with_libvirtd" + + (* Each enty in the config is one of the following three ... *) + let entry = log_entry + let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] + let empty = [ label "#empty" . eol ] + + let record = indent . entry . eol + + let lns = ( record | comment | empty ) * + + let filter = incl "/etc/libvirt/lxc.conf" + . Util.stdexcl + + let xfm = transform lns filter diff --git a/src/lxc/test_libvirtd_lxc.aug b/src/lxc/test_libvirtd_lxc.aug new file mode 100644 index 0000000..e757b82 --- /dev/null +++ b/src/lxc/test_libvirtd_lxc.aug @@ -0,0 +1,31 @@ +module Test_libvirtd_lxc = + + let conf = "# Master configuration file for the LXC driver. +# All settings described here are optional - if omitted, sensible +# defaults are used. + +# By default, log messages generated by the lxc controller go to the +# container logfile. It is also possible to accumulate log messages +# from all lxc controllers along with libvirtd's log outputs. In this +# case, the lxc controller will honor either LIBVIRT_LOG_OUTPUTS or +# log_outputs from libvirtd.conf. +# +# This is disabled by default, uncomment below to enable it. +# +log_with_libvirtd = 1 +" + + test Libvirtd_lxc.lns get conf = +{ "#comment" = "Master configuration file for the LXC driver." } +{ "#comment" = "All settings described here are optional - if omitted, sensible" } +{ "#comment" = "defaults are used." } +{ "#empty" } +{ "#comment" = "By default, log messages generated by the lxc controller go to the" } +{ "#comment" = "container logfile. It is also possible to accumulate log messages" } +{ "#comment" = "from all lxc controllers along with libvirtd's log outputs. In this" } +{ "#comment" = "case, the lxc controller will honor either LIBVIRT_LOG_OUTPUTS or" } +{ "#comment" = "log_outputs from libvirtd.conf." } +{ "#comment" = "" } +{ "#comment" = "This is disabled by default, uncomment below to enable it." } +{ "#comment" = "" } +{ "log_with_libvirtd" = "1" }