[lxc-devel] [PATCH] use a default per-container logfile (v2)
Stéphane Graber
stgraber at ubuntu.com
Mon Jan 28 22:53:23 UTC 2013
On 01/24/2013 01:04 PM, Serge Hallyn wrote:
> Until now, if a lxc-* (i.e. lxc-start) command did not specify a logfile
> (with -o logfile), the default was effectively 'none'. With this patch,
> the default becomes a per-container log file.
>
> If a container config file specifies 'lxc.logfile', that will override
> the default. If a '-o logfile' argument is specifed at lxc-start,
> then that will override both the default and the configuration file
> entry. Finally, '-o none' can be used to avoid having a logfile at
> all (in other words, the previous default), and that will override
> a lxc.logfile entry in the container configuration file.
>
> If the user does not have rights to open the default, then 'none' will
> be used. However, in that case an error will show up on console. (We
> can work on removing that if it annoys people, but I think it is
> helpful, at least while we're still ironing this set out) If the user
> or container configuration file specified a logfile, and the user does
> not have rights to open the default, then the action will fail.
>
> One slight "mis-behavior" which I have not fixed (and may not fix) is
> that if a lxc.logfile is specified, the default logfile will still
> get created before we read the configuration file to find out there
> is a lxc.logfile entry.
>
> changelog: Jan 24:
>
> add --enable-configpath-log configure option
>
> When we log to /var/lib/lxc/$container/$container.log, several things
> need to be done differently than when we log into /var/log/lxc (for
> instance). So give it a configure option so we know what to do
>
> When the user specifies a logfile, we bail if we can't open it. But
> when opening the default logfile, the user may not have rights to
> open it, so in that case ignore it and continue as if using 'none'.
>
> When using /var/lib/lxc/$c/$c.log, we use $LOGPATH/$name/$name.log.
> Otherwise, we use $LOGPATH/$name.log.
>
> When using /var/lib/lxc/$c/$c.log, don't try to create the log path
> /var/lib/lxc/$c. It can only not exist if the container doesn't
> exist. We don't want to create the directory in that case. When
> using /var/log/lxc, then we do want to create the path if it does
> not exist.
>
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
Acked-by: Stéphane Graber <stgraber at ubuntu.com>
I think this addresses all of the issues I've seen with v1. Test build
on both android and ubuntu was successful. Pushed to staging. Thanks
> ---
> configure.ac | 21 +++++++
> src/lxc/Makefile.am | 7 ++-
> src/lxc/confile.c | 5 --
> src/lxc/log.c | 145 ++++++++++++++++++++++++++++++++++++++++++-----
> src/lxc/log.h | 4 +-
> src/lxc/lxc_attach.c | 2 +-
> src/lxc/lxc_cgroup.c | 2 +-
> src/lxc/lxc_checkpoint.c | 2 +-
> src/lxc/lxc_console.c | 2 +-
> src/lxc/lxc_execute.c | 2 +-
> src/lxc/lxc_freeze.c | 2 +-
> src/lxc/lxc_info.c | 2 +-
> src/lxc/lxc_init.c | 2 +-
> src/lxc/lxc_kill.c | 2 +-
> src/lxc/lxc_monitor.c | 2 +-
> src/lxc/lxc_restart.c | 2 +-
> src/lxc/lxc_start.c | 2 +-
> src/lxc/lxc_stop.c | 2 +-
> src/lxc/lxc_unfreeze.c | 2 +-
> src/lxc/lxc_wait.c | 2 +-
> src/lxc/lxccontainer.c | 2 +-
> 21 files changed, 175 insertions(+), 39 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index d1f5ad9..e567245 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -157,6 +157,26 @@ AC_ARG_WITH([rootfs-path],
> [lxc rootfs mount point]
> )], [], [with_rootfs_path=['${libdir}/lxc/rootfs']])
>
> +# Container log path. By default, use $lxcpath.
> +AC_MSG_CHECKING([Whether to place logfiles in container config path])
> +AC_ARG_ENABLE([configpath-log],
> + [AC_HELP_STRING([--enable-configpath-log], [use logfiles in config path])],
> + [use_configpath_logs=yes], [use_configpath_logs=no])
> +AC_MSG_RESULT([$use_configpath_logs])
> +AM_CONDITIONAL([USE_CONFIGPATH_LOGS], [test "$use_configpath_logs" = "yes"])
> +
> +if test "$use_configpath_logs" = "yes"; then
> + default_log_path="${with_config_path}"
> +else
> + default_log_path="/var/log/lxc"
> +fi
> +
> +AC_ARG_WITH([log-path],
> + [AC_HELP_STRING(
> + [--with-log-path=dir],
> + [per container log path]
> + )], [], [with_log_path=['${default_log_path}']])
> +
> # Expand some useful variables
> AS_AC_EXPAND(PREFIX, "$prefix")
> AS_AC_EXPAND(LIBDIR, "$libdir")
> @@ -173,6 +193,7 @@ AS_AC_EXPAND(LXCPATH, "$with_config_path")
> AS_AC_EXPAND(LXCROOTFSMOUNT, "$with_rootfs_path")
> AS_AC_EXPAND(LXCTEMPLATEDIR, "$datadir/lxc/templates")
> AS_AC_EXPAND(LXCINITDIR, "$libexecdir")
> +AS_AC_EXPAND(LOGPATH, "$with_log_path")
>
> # Check for some standard kernel headers
> AC_CHECK_HEADERS([linux/unistd.h linux/netlink.h linux/genetlink.h],
> diff --git a/src/lxc/Makefile.am b/src/lxc/Makefile.am
> index b55a20c..2fb8291 100644
> --- a/src/lxc/Makefile.am
> +++ b/src/lxc/Makefile.am
> @@ -89,12 +89,17 @@ AM_CFLAGS=-I$(top_srcdir)/src \
> -DLXCROOTFSMOUNT=\"$(LXCROOTFSMOUNT)\" \
> -DLXCPATH=\"$(LXCPATH)\" \
> -DLXCINITDIR=\"$(LXCINITDIR)\" \
> - -DLXCTEMPLATEDIR=\"$(LXCTEMPLATEDIR)\"
> + -DLXCTEMPLATEDIR=\"$(LXCTEMPLATEDIR)\" \
> + -DLOGPATH=\"$(LOGPATH)\"
>
> if ENABLE_APPARMOR
> AM_CFLAGS += -DHAVE_APPARMOR
> endif
>
> +if USE_CONFIGPATH_LOGS
> +AM_CFLAGS += -DUSE_CONFIGPATH_LOGS
> +endif
> +
> if ENABLE_SECCOMP
> AM_CFLAGS += -DHAVE_SECCOMP
> liblxc_so_SOURCES += seccomp.c
> diff --git a/src/lxc/confile.c b/src/lxc/confile.c
> index 67a989e..d350f01 100644
> --- a/src/lxc/confile.c
> +++ b/src/lxc/confile.c
> @@ -927,11 +927,6 @@ static int config_aa_profile(const char *key, const char *value,
> static int config_logfile(const char *key, const char *value,
> struct lxc_conf *lxc_conf)
> {
> - if (lxc_log_get_file()) {
> - DEBUG("Log file already specified - ignoring new value");
> - return 0;
> - }
> -
> return lxc_log_set_file(value);
> }
>
> diff --git a/src/lxc/log.c b/src/lxc/log.c
> index d2a90de..ff512e3 100644
> --- a/src/lxc/log.c
> +++ b/src/lxc/log.c
> @@ -42,6 +42,8 @@
> int lxc_log_fd = -1;
> static char log_prefix[LXC_LOG_PREFIX_SIZE] = "lxc";
> static int lxc_loglevel_specified = 0;
> +// if logfile was specifed on command line, it won't be overridden by lxc.logfile
> +static int lxc_log_specified = 0;
>
> lxc_log_define(lxc_log, lxc);
>
> @@ -123,6 +125,36 @@ extern void lxc_log_setprefix(const char *prefix)
> log_prefix[sizeof(log_prefix) - 1] = 0;
> }
>
> +static int build_dir(const char *name)
> +{
> + char *n = strdup(name); // because we'll be modifying it
> + char *p, *e;
> + int ret;
> +
> + if (!n) {
> + ERROR("Out of memory while creating directory '%s'.", name);
> + return -1;
> + }
> +
> + e = &n[strlen(n)];
> + for (p = n+1; p < e; p++) {
> + if (*p != '/')
> + continue;
> + *p = '\0';
> + if (access(n, F_OK)) {
> + ret = lxc_unpriv(mkdir(n, 0755));
> + if (ret && errno != -EEXIST) {
> + SYSERROR("failed to create directory '%s'.", n);
> + free(n);
> + return -1;
> + }
> + }
> + *p = '/';
> + }
> + free(n);
> + return 0;
> +}
> +
> /*---------------------------------------------------------------------------*/
> static int log_open(const char *name)
> {
> @@ -148,11 +180,46 @@ static int log_open(const char *name)
> return newfd;
> }
>
> +static char *build_log_path(const char *name)
> +{
> + char *p;
> + int len, ret;
> +
> + /*
> + * '$logpath' + '/' + '$name' + '.log' + '\0'
> + * or
> + * '$logpath' + '/' + '$name' + '/' + '$name' + '.log' + '\0'
> + * sizeof(LOGPATH) includes its \0
> + */
> + len = sizeof(LOGPATH) + strlen(name) + 6;
> +#if USE_CONFIGPATH_LOGS
> + len += strlen(name) + 1; /* add "/$container_name/" */
> +#endif
> + p = malloc(len);
> + if (!p)
> + return p;
> +#if USE_CONFIGPATH_LOGS
> + ret = snprintf(p, len, "%s/%s/%s.log", LOGPATH, name, name);
> +#else
> + ret = snprintf(p, len, "%s/%s.log", LOGPATH, name);
> +#endif
> + if (ret < 0 || ret >= len) {
> + free(p);
> + return NULL;
> + }
> + return p;
> +}
> +
> +int do_lxc_log_set_file(const char *fname, int from_default);
> +
> /*---------------------------------------------------------------------------*/
> -extern int lxc_log_init(const char *file, const char *priority,
> - const char *prefix, int quiet)
> +extern int lxc_log_init(const char *name, const char *file,
> + const char *priority, const char *prefix, int quiet)
> {
> int lxc_priority = LXC_LOG_PRIORITY_ERROR;
> + int ret;
> + char *tmpfile = NULL;
> + int want_lxc_log_specified = 0;
>
> if (lxc_log_fd != -1)
> return 0;
> @@ -176,10 +243,38 @@ extern int lxc_log_init(const char *file, const char *priority,
> if (prefix)
> lxc_log_setprefix(prefix);
>
> - if (file)
> - return lxc_log_set_file(file);
> + if (file && strcmp(file, "none") == 0) {
> + want_lxc_log_specified = 1;
> + return 0;
> + }
>
> - return 0;
> + if (!file) {
> + tmpfile = build_log_path(name);
> + if (!tmpfile) {
> + ERROR("could not build log path");
> + return -1;
> + }
> + } else {
> + want_lxc_log_specified = 1;
> + }
> +
> + ret = do_lxc_log_set_file(tmpfile ? tmpfile : file, !want_lxc_log_specified);
> +
> + if (want_lxc_log_specified)
> + lxc_log_specified = 1;
> + /*
> + * If !want_lxc_log_specified, that is, if the user did not request
> + * this logpath, then ignore failures and continue logging to console
> + */
> + if (!want_lxc_log_specified && ret != 0) {
> + INFO("Ignoring failure to open default logfile.");
> + ret = 0;
> + }
> +
> + if (tmpfile)
> + free(tmpfile);
> +
> + return ret;
> }
>
> /*
> @@ -200,30 +295,50 @@ extern int lxc_log_set_level(int level)
> }
>
> char *log_fname; // default to NULL, set in lxc_log_set_file.
> -
> /*
> - * This is called when we read a lxc.logfile entry in a lxc.conf file. This
> - * happens after processing command line arguments, which override the .conf
> - * settings. So only set the logfile if previously unset.
> + * This can be called:
> + * 1. when a program calls lxc_log_init with no logfile parameter (in which
> + * case the default is used). In this case lxc.logfile can override this.
> + * 2. when a program calls lxc_log_init with a logfile parameter. In this
> + * case we don't want lxc.logfile to override this.
> + * 3. When a lxc.logfile entry is found in config file.
> */
> -extern int lxc_log_set_file(const char *fname)
> +int do_lxc_log_set_file(const char *fname, int from_default)
> {
> + if (lxc_log_specified) {
> + INFO("lxc.logfile overriden by command line");
> + return 0;
> + }
> if (lxc_log_fd != -1) {
> - // this should've been caught at config_logfile.
> - ERROR("Race in setting logfile?");
> + // we are overriding the default.
> + close(lxc_log_fd);
> + free(log_fname);
> + }
> +
> +#if USE_CONFIGPATH_LOGS
> + // we don't build_dir for the default if the default is
> + // i.e. /var/lib/lxc/$container/$container.log
> + if (!from_default)
> +#endif
> + if (build_dir(fname)) {
> + ERROR("failed to create dir for log file \"%s\" : %s", fname,
> + strerror(errno));
> return -1;
> }
>
> lxc_log_fd = log_open(fname);
> - if (lxc_log_fd == -1) {
> - ERROR("failed to open log file %s\n", fname);
> + if (lxc_log_fd == -1)
> return -1;
> - }
>
> log_fname = strdup(fname);
> return 0;
> }
>
> +extern int lxc_log_set_file(const char *fname)
> +{
> + return do_lxc_log_set_file(fname, 0);
> +}
> +
> extern int lxc_log_get_level(void)
> {
> if (!lxc_loglevel_specified)
> diff --git a/src/lxc/log.h b/src/lxc/log.h
> index a9260f2..7719958 100644
> --- a/src/lxc/log.h
> +++ b/src/lxc/log.h
> @@ -287,8 +287,8 @@ extern struct lxc_log_category lxc_log_category_lxc;
>
> extern int lxc_log_fd;
>
> -extern int lxc_log_init(const char *file, const char *priority,
> - const char *prefix, int quiet);
> +extern int lxc_log_init(const char *name, const char *file,
> + const char *priority, const char *prefix, int quiet);
>
> extern void lxc_log_setprefix(const char *a_prefix);
> extern int lxc_log_set_level(int level);
> diff --git a/src/lxc/lxc_attach.c b/src/lxc/lxc_attach.c
> index 851a37a..437a400 100644
> --- a/src/lxc/lxc_attach.c
> +++ b/src/lxc/lxc_attach.c
> @@ -139,7 +139,7 @@ int main(int argc, char *argv[])
> if (ret)
> return ret;
>
> - ret = lxc_log_init(my_args.log_file, my_args.log_priority,
> + ret = lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet);
> if (ret)
> return ret;
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> index 97769a5..970391b 100644
> --- a/src/lxc/lxc_cgroup.c
> +++ b/src/lxc/lxc_cgroup.c
> @@ -68,7 +68,7 @@ int main(int argc, char *argv[])
> if (lxc_arguments_parse(&my_args, argc, argv))
> return -1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return -1;
>
> diff --git a/src/lxc/lxc_checkpoint.c b/src/lxc/lxc_checkpoint.c
> index 76f6709..a151750 100644
> --- a/src/lxc/lxc_checkpoint.c
> +++ b/src/lxc/lxc_checkpoint.c
> @@ -115,7 +115,7 @@ int main(int argc, char *argv[])
> if (ret)
> return ret;
>
> - ret = lxc_log_init(my_args.log_file, my_args.log_priority,
> + ret = lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet);
> if (ret)
> return ret;
> diff --git a/src/lxc/lxc_console.c b/src/lxc/lxc_console.c
> index d09c688..c263d0f 100644
> --- a/src/lxc/lxc_console.c
> +++ b/src/lxc/lxc_console.c
> @@ -187,7 +187,7 @@ int main(int argc, char *argv[])
> if (err)
> return -1;
>
> - err = lxc_log_init(my_args.log_file, my_args.log_priority,
> + err = lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet);
> if (err)
> return -1;
> diff --git a/src/lxc/lxc_execute.c b/src/lxc/lxc_execute.c
> index 1eb25a7..9377f4f 100644
> --- a/src/lxc/lxc_execute.c
> +++ b/src/lxc/lxc_execute.c
> @@ -99,7 +99,7 @@ int main(int argc, char *argv[])
> if (lxc_arguments_parse(&my_args, argc, argv))
> return -1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return -1;
>
> diff --git a/src/lxc/lxc_freeze.c b/src/lxc/lxc_freeze.c
> index 770d923..4da5654 100644
> --- a/src/lxc/lxc_freeze.c
> +++ b/src/lxc/lxc_freeze.c
> @@ -54,7 +54,7 @@ int main(int argc, char *argv[])
> if (lxc_arguments_parse(&my_args, argc, argv))
> return -1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return -1;
>
> diff --git a/src/lxc/lxc_info.c b/src/lxc/lxc_info.c
> index 1a1cc22..48c1370 100644
> --- a/src/lxc/lxc_info.c
> +++ b/src/lxc/lxc_info.c
> @@ -79,7 +79,7 @@ int main(int argc, char *argv[])
> if (ret)
> return 1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return 1;
>
> diff --git a/src/lxc/lxc_init.c b/src/lxc/lxc_init.c
> index 2263cd1..90e1ad6 100644
> --- a/src/lxc/lxc_init.c
> +++ b/src/lxc/lxc_init.c
> @@ -79,7 +79,7 @@ int main(int argc, char *argv[])
> if (lxc_caps_init())
> exit(err);
>
> - if (lxc_log_init(NULL, 0, basename(argv[0]), quiet))
> + if (lxc_log_init(NULL, "none", 0, basename(argv[0]), quiet))
> exit(err);
>
> if (!argv[optind]) {
> diff --git a/src/lxc/lxc_kill.c b/src/lxc/lxc_kill.c
> index 3d996a5..f9bfe34 100644
> --- a/src/lxc/lxc_kill.c
> +++ b/src/lxc/lxc_kill.c
> @@ -61,7 +61,7 @@ int main(int argc, char *argv[], char *envp[])
> if (ret)
> return ret;
>
> - ret = lxc_log_init(my_args.log_file, my_args.log_priority,
> + ret = lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet);
> if (ret)
> return ret;
> diff --git a/src/lxc/lxc_monitor.c b/src/lxc/lxc_monitor.c
> index 4bd6ab0..2b8b0a0 100644
> --- a/src/lxc/lxc_monitor.c
> +++ b/src/lxc/lxc_monitor.c
> @@ -65,7 +65,7 @@ int main(int argc, char *argv[])
> if (lxc_arguments_parse(&my_args, argc, argv))
> return -1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return -1;
>
> diff --git a/src/lxc/lxc_restart.c b/src/lxc/lxc_restart.c
> index 7548682..1cf9462 100644
> --- a/src/lxc/lxc_restart.c
> +++ b/src/lxc/lxc_restart.c
> @@ -122,7 +122,7 @@ int main(int argc, char *argv[])
> if (lxc_arguments_parse(&my_args, argc, argv))
> return -1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return -1;
>
> diff --git a/src/lxc/lxc_start.c b/src/lxc/lxc_start.c
> index a97dcca..b64acff 100644
> --- a/src/lxc/lxc_start.c
> +++ b/src/lxc/lxc_start.c
> @@ -164,7 +164,7 @@ int main(int argc, char *argv[])
> else
> args = my_args.argv;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return err;
>
> diff --git a/src/lxc/lxc_stop.c b/src/lxc/lxc_stop.c
> index 639b750..749d78a 100644
> --- a/src/lxc/lxc_stop.c
> +++ b/src/lxc/lxc_stop.c
> @@ -53,7 +53,7 @@ int main(int argc, char *argv[])
> if (lxc_arguments_parse(&my_args, argc, argv))
> return -1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return -1;
>
> diff --git a/src/lxc/lxc_unfreeze.c b/src/lxc/lxc_unfreeze.c
> index 22be839..02e9a47 100644
> --- a/src/lxc/lxc_unfreeze.c
> +++ b/src/lxc/lxc_unfreeze.c
> @@ -53,7 +53,7 @@ int main(int argc, char *argv[])
> if (lxc_arguments_parse(&my_args, argc, argv))
> return -1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return -1;
>
> diff --git a/src/lxc/lxc_wait.c b/src/lxc/lxc_wait.c
> index 799225d..b0643c7 100644
> --- a/src/lxc/lxc_wait.c
> +++ b/src/lxc/lxc_wait.c
> @@ -83,7 +83,7 @@ int main(int argc, char *argv[])
> if (lxc_arguments_parse(&my_args, argc, argv))
> return -1;
>
> - if (lxc_log_init(my_args.log_file, my_args.log_priority,
> + if (lxc_log_init(my_args.name, my_args.log_file, my_args.log_priority,
> my_args.progname, my_args.quiet))
> return -1;
>
> diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> index 5919d2c..502a7a7 100644
> --- a/src/lxc/lxccontainer.c
> +++ b/src/lxc/lxccontainer.c
> @@ -976,7 +976,7 @@ struct lxc_container *lxc_container_new(const char *name)
> c->set_cgroup_item = lxcapi_set_cgroup_item;
>
> /* we'll allow the caller to update these later */
> - if (lxc_log_init(NULL, NULL, "lxc_container", 0)) {
> + if (lxc_log_init(NULL, "none", NULL, "lxc_container", 0)) {
> fprintf(stderr, "failed to open log\n");
> goto err;
> }
>
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20130128/437c71a4/attachment.pgp>
More information about the lxc-devel
mailing list