[libvirt] [PATCH] libxl: define a per-domain logger.

Jim Fehlig jfehlig at suse.com
Wed Jan 4 04:28:40 UTC 2017


On 12/20/2016 09:40 AM, Cédric Bosdonnat wrote:
> libxl doesn't provide a way to write one log for each domain. Thus
> we need to demux the messages. If our logger doesn't know to which
> domain to attribute a message, then it will write it to the default
> log file.
>
> Starting with Xen 4.9 (commit f9858025 and following), libxl will
> write the domain ID in an easy to grab manner. The logger introduced
> by this commit will use it to demux the libxl log messages.

Cool, thanks for the work! With new enough Xen this allows for the 
reintroduction of per-domain log files.

>
> Thanks to the default log file, this logger will also work with older
> versions of Xen.
> ---
>  src/Makefile.am          |   1 +
>  src/libxl/libxl_conf.c   |  38 +--------
>  src/libxl/libxl_conf.h   |   4 +-
>  src/libxl/libxl_domain.c |   4 +
>  src/libxl/libxl_driver.c |   2 +
>  src/libxl/libxl_logger.c | 217 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_logger.h |  39 +++++++++
>  7 files changed, 268 insertions(+), 37 deletions(-)
>  create mode 100644 src/libxl/libxl_logger.c
>  create mode 100644 src/libxl/libxl_logger.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index b71378728..e34d52345 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -851,6 +851,7 @@ LIBXL_DRIVER_SOURCES =							\
>  		libxl/libxl_capabilities.c libxl/libxl_capabilities.h	\
>  		libxl/libxl_domain.c libxl/libxl_domain.h       	\
>  		libxl/libxl_driver.c libxl/libxl_driver.h       	\
> +		libxl/libxl_logger.c libxl/libxl_logger.h           \
>  		libxl/libxl_migration.c libxl/libxl_migration.h
>
>  UML_DRIVER_SOURCES =						\
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b569ddad8..929179903 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -76,9 +76,7 @@ libxlDriverConfigDispose(void *obj)
>
>      virObjectUnref(cfg->caps);
>      libxl_ctx_free(cfg->ctx);
> -    xtl_logger_destroy(cfg->logger);
> -    if (cfg->logger_file)
> -        VIR_FORCE_FCLOSE(cfg->logger_file);
> +    libxlLoggerFree(cfg->logger);
>
>      VIR_FREE(cfg->configDir);
>      VIR_FREE(cfg->autostartDir);
> @@ -1356,8 +1354,6 @@ libxlDriverConfigPtr
>  libxlDriverConfigNew(void)
>  {
>      libxlDriverConfigPtr cfg;
> -    char *log_file = NULL;
> -    xentoollog_level log_level = XTL_DEBUG;
>      char ebuf[1024];
>      unsigned int free_mem;
>
> @@ -1386,9 +1382,6 @@ libxlDriverConfigNew(void)
>      if (VIR_STRDUP(cfg->channelDir, LIBXL_CHANNEL_DIR) < 0)
>          goto error;
>
> -    if (virAsprintf(&log_file, "%s/libxl-driver.log", cfg->logDir) < 0)
> -        goto error;
> -
>      if (virFileMakePath(cfg->logDir) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("failed to create log dir '%s': %s"),
> @@ -1397,37 +1390,13 @@ libxlDriverConfigNew(void)
>          goto error;
>      }
>
> -    if ((cfg->logger_file = fopen(log_file, "a")) == NULL)  {
> -        VIR_ERROR(_("Failed to create log file '%s': %s"),
> -                  log_file, virStrerror(errno, ebuf, sizeof(ebuf)));
> -        goto error;
> -    }
> -    VIR_FREE(log_file);
> -
> -    switch (virLogGetDefaultPriority()) {
> -    case VIR_LOG_DEBUG:
> -        log_level = XTL_DEBUG;
> -        break;
> -    case VIR_LOG_INFO:
> -        log_level = XTL_INFO;
> -        break;
> -    case VIR_LOG_WARN:
> -        log_level = XTL_WARN;
> -        break;
> -    case VIR_LOG_ERROR:
> -        log_level = XTL_ERROR;
> -        break;
> -    }
> -
> -    cfg->logger =
> -        (xentoollog_logger *)xtl_createlogger_stdiostream(cfg->logger_file,
> -                                      log_level, XTL_STDIOSTREAM_SHOW_DATE);
> +    cfg->logger = libxlLoggerNew(cfg->logDir, virLogGetDefaultPriority());
>      if (!cfg->logger) {
>          VIR_ERROR(_("cannot create logger for libxenlight, disabling driver"));
>          goto error;
>      }
>
> -    if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, cfg->logger)) {
> +    if (libxl_ctx_alloc(&cfg->ctx, LIBXL_VERSION, 0, (void*)cfg->logger)) {

Should this be casted to 'xentoollog_logger *' instead of 'void *'?

>          VIR_ERROR(_("cannot initialize libxenlight context, probably not "
>                      "running in a Xen Dom0, disabling driver"));
>          goto error;
> @@ -1478,7 +1447,6 @@ libxlDriverConfigNew(void)
>      return cfg;
>
>   error:
> -    VIR_FREE(log_file);
>      virObjectUnref(cfg);
>      return NULL;
>  }
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 851f3afb4..69d78851a 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -41,6 +41,7 @@
>  # include "locking/lock_manager.h"
>  # include "virfirmware.h"
>  # include "libxl_capabilities.h"
> +# include "libxl_logger.h"
>
>  # define LIBXL_DRIVER_NAME "xenlight"
>  # define LIBXL_VNC_PORT_MIN  5900
> @@ -74,8 +75,7 @@ struct _libxlDriverConfig {
>      unsigned int version;
>
>      /* log stream for driver-wide libxl ctx */
> -    FILE *logger_file;
> -    xentoollog_logger *logger;
> +    libxlLoggerPtr logger;
>      /* libxl ctx for driver wide ops; getVersion, getNodeInfo, ... */
>      libxl_ctx *ctx;
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index 5cde576ef..3bc468f61 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -809,6 +809,8 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>          VIR_FREE(xml);
>      }
>
> +    libxlLoggerCloseFile(cfg->logger, vm->def->id);
> +
>      virDomainObjRemoveTransientDef(vm);
>      virObjectUnref(cfg);
>  }
> @@ -1291,6 +1293,8 @@ libxlDomainStart(libxlDriverPrivatePtr driver,
>       */
>      vm->def->id = domid;
>
> +    libxlLoggerOpenFile(cfg->logger, domid, vm->def->name);
> +
>      /* Always enable domain death events */
>      if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW))
>          goto destroy_dom;
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 7e5d9b69e..6a4ecddef 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -406,6 +406,8 @@ libxlReconnectDomain(virDomainObjPtr vm,
>      /* Update domid in case it changed (e.g. reboot) while we were gone? */
>      vm->def->id = d_info.domid;
>
> +    libxlLoggerOpenFile(cfg->logger, vm->def->id, vm->def->name);
> +
>      /* Update hostdev state */
>      if (virHostdevUpdateActiveDomainDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
>                                              vm->def, hostdev_flags) < 0)
> diff --git a/src/libxl/libxl_logger.c b/src/libxl/libxl_logger.c
> new file mode 100644
> index 000000000..31110d0fd
> --- /dev/null
> +++ b/src/libxl/libxl_logger.c
> @@ -0,0 +1,217 @@
> +/*
> + * libxl_logger.c: libxl logger implementation
> + *
> + * Copyright (c) 2016 SUSE LINUX Products GmbH, Nuernberg, Germany.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Authors:
> + *     Cédric Bosdonnat <cbosdonnat at suse.com>
> + */
> +#include <config.h>
> +
> +#include <string.h>
> +#include <libxl.h>
> +
> +#include "internal.h"
> +#include "libxl_logger.h"
> +#include "util/viralloc.h"
> +#include "util/virerror.h"
> +#include "util/virfile.h"
> +#include "util/virhash.h"
> +#include "util/virstring.h"
> +#include "util/virtime.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_LIBXL
> +
> +VIR_LOG_INIT("libxl.libxl_logger");
> +
> +typedef struct xentoollog_logger_libvirt xentoollog_logger_libvirt;
> +
> +struct xentoollog_logger_libvirt {
> +    xentoollog_logger vtable;
> +    xentoollog_level minLevel;
> +    const char *logDir;
> +
> +    /* map storing the opened fds: "domid" -> FILE* */
> +    virHashTablePtr files;
> +    FILE *defaultLogFile;
> +};
> +
> +static void
> +libxlLoggerFileFree(void *payload, const void *key ATTRIBUTE_UNUSED)
> +{
> +    FILE *file = payload;
> +    VIR_FORCE_FCLOSE(file);
> +    file = NULL;
> +}
> +
> +ATTRIBUTE_FMT_PRINTF(5, 0) static void
> +libvirt_vmessage(xentoollog_logger *logger_in,
> +                 xentoollog_level level,
> +                 int errnoval,
> +                 const char *context,
> +                 const char *format,
> +                 va_list args)
> +{
> +    xentoollog_logger_libvirt *lg = (void*)logger_in;

Same comment about the cast here (and a few additional spots below where 'void 
*' is used). Should it simply be 'xentoollog_logger_libvirt *' ?

> +    FILE *logFile = lg->defaultLogFile;
> +    char timestamp[VIR_TIME_STRING_BUFLEN];
> +    char *message = NULL;
> +    char *start, *end;
> +    char ebuf[1024];
> +
> +    VIR_DEBUG("libvirt_vmessage: context='%s' format='%s'", context, format);
> +
> +    if (level < lg->minLevel)
> +        return;
> +
> +    if (virVasprintf(&message, format, args) < 0)
> +        return;
> +
> +    /* Should we print to a domain-specific log file? */
> +    if ((start = strstr(message, ": Domain ")) &&
> +        (end = strstr(start + 9, ":"))) {
> +        FILE *domainLogFile;
> +
> +        VIR_DEBUG("Found domain log message");
> +
> +        start = start + 9;
> +        *end = '\0';
> +
> +        domainLogFile = virHashLookup(lg->files, start);
> +        if (domainLogFile)
> +            logFile = domainLogFile;
> +
> +        *end = ':';
> +    }
> +
> +    /* Do the actual print to the log file */
> +    if (virTimeStringNowRaw(timestamp) < 0)
> +        timestamp[0] = '\0';
> +
> +    fprintf(logFile, "%s: ", timestamp);
> +    if (context)
> +        fprintf(logFile, "%s: ", context);
> +
> +    fprintf(logFile, "%s", message);
> +
> +    if (errnoval >= 0)
> +        fprintf(logFile, ": %s", virStrerror(errnoval, ebuf, sizeof(ebuf)));
> +
> +    fputc('\n', logFile);
> +    fflush(logFile);
> +
> +    VIR_FREE(message);
> +}
> +
> +static void
> +libvirt_progress(xentoollog_logger *logger_in ATTRIBUTE_UNUSED,
> +                 const char *context ATTRIBUTE_UNUSED,
> +                 const char *doingwhat ATTRIBUTE_UNUSED,
> +                 int percent ATTRIBUTE_UNUSED,
> +                 unsigned long done ATTRIBUTE_UNUSED,
> +                 unsigned long total ATTRIBUTE_UNUSED)
> +{
> +    /* This function purposedly does nothing: it's no logging info */
> +}
> +
> +static void
> +libvirt_destroy(xentoollog_logger *logger_in)
> +{
> +    xentoollog_logger_libvirt *lg = (void*)logger_in;
> +    VIR_FREE(lg);
> +}
> +
> +
> +libxlLoggerPtr libxlLoggerNew(const char *logDir,
> +                              virLogPriority minLevel)
> +{
> +    xentoollog_logger_libvirt logger;
> +    libxlLoggerPtr logger_out = NULL;
> +    char *path = NULL;
> +
> +    switch (minLevel) {
> +    case VIR_LOG_DEBUG:
> +        logger.minLevel = XTL_DEBUG;
> +        break;
> +    case VIR_LOG_INFO:
> +        logger.minLevel = XTL_INFO;
> +        break;
> +    case VIR_LOG_WARN:
> +        logger.minLevel = XTL_WARN;
> +        break;
> +    case VIR_LOG_ERROR:
> +        logger.minLevel = XTL_ERROR;
> +        break;
> +    }
> +    logger.logDir = logDir;
> +
> +    if ((logger.files = virHashCreate(3, libxlLoggerFileFree)) == NULL)
> +        return NULL;
> +
> +    if (virAsprintf(&path, "%s/libxl-driver.log", logDir) < 0)
> +        goto error;
> +
> +    if ((logger.defaultLogFile = fopen(path, "a")) == NULL)
> +        goto error;
> +
> +    logger_out = XTL_NEW_LOGGER(libvirt, logger);
> +
> + cleanup:
> +    VIR_FREE(path);
> +    return logger_out;
> +
> + error:
> +    virHashFree(logger.files);
> +    goto cleanup;
> +}
> +
> +void libxlLoggerFree(libxlLoggerPtr logger)
> +{
> +    xentoollog_logger *xtl_logger = (void*)logger;
> +    if (logger->defaultLogFile)
> +        VIR_FORCE_FCLOSE(logger->defaultLogFile);
> +    virHashFree(logger->files);
> +    xtl_logger_destroy(xtl_logger);
> +}
> +
> +void libxlLoggerOpenFile(libxlLoggerPtr logger, int id, const char *name)
> +{
> +    char *path = NULL;
> +    FILE *logFile = NULL;
> +    char *domidstr = NULL;
> +
> +    if (virAsprintf(&path, "%s/%s.log", logger->logDir, name) < 0 ||
> +        virAsprintf(&domidstr, "%d", id) < 0)
> +        return;

'path' will leak here if the second virAsprintf() fails.

> +
> +    logFile = fopen(path, "a");

Should we check for non-NULL logFile before adding it to the hashtable?

I've only compile tested the patch, but it is looking good.

Regards,
Jim

> +    ignore_value(virHashAddEntry(logger->files, domidstr, logFile));
> +
> +    VIR_FREE(path);
> +    VIR_FREE(domidstr);
> +}
> +
> +void libxlLoggerCloseFile(libxlLoggerPtr logger, int id)
> +{
> +    char *domidstr = NULL;
> +    if (virAsprintf(&domidstr, "%d", id) < 0)
> +        return;
> +
> +    ignore_value(virHashRemoveEntry(logger->files, domidstr));
> +
> +    VIR_FREE(domidstr);
> +}
> diff --git a/src/libxl/libxl_logger.h b/src/libxl/libxl_logger.h
> new file mode 100644
> index 000000000..88c2868eb
> --- /dev/null
> +++ b/src/libxl/libxl_logger.h
> @@ -0,0 +1,39 @@
> +/*
> + * libxl_logger.h: libxl logger implementation
> + *
> + * Copyright (c) 2016 SUSE LINUX Products GmbH, Nuernberg, Germany.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library.  If not, see
> + * <http://www.gnu.org/licenses/>.
> + *
> + * Authors:
> + *     Cédric Bosdonnat <cbosdonnat at suse.com>
> + */
> +
> +#ifndef __LIBXL_LOGGER_H
> +# define __LIBXL_LOGGER_H
> +
> +# include "util/virlog.h"
> +
> +typedef struct xentoollog_logger_libvirt libxlLogger;
> +typedef libxlLogger *libxlLoggerPtr;
> +
> +libxlLoggerPtr libxlLoggerNew(const char *logDir,
> +                               virLogPriority minLevel);
> +void libxlLoggerFree(libxlLoggerPtr logger);
> +
> +void libxlLoggerOpenFile(libxlLoggerPtr logger, int id, const char *name);
> +void libxlLoggerCloseFile(libxlLoggerPtr logger, int id);
> +
> +#endif /* __LIBXL_LOGGER_H */
>




More information about the libvir-list mailing list