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

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. One thing I noticed when testing this on a Xen installation prior to that commit is log files are created that will never be written to. E.g. after starting libvirtd and one domain # ll /var/log/libvirt/libxl/ -rw-r--r-- 1 root root     0 Jan  5 15:48 Domain-0.log -rw-r--r-- 1 root root 17512 Jan  5 15:49 libxl-driver.log -rw-r--r-- 1 root root     0 Jan  5 15:49 sles12sp2-hvm.log IMO these empty, never-to-be-written-to log files might be a bit confusing to users. AFAIK there is no way to detect up front that the underlying Xen supports this capability right?

Regards, Jim > > Thanks to the default log file, this logger will also work with older > versions of Xen. > --- > >  * v2: addressed Jim's comments > >  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 | 223 +++++++++++++++++++++++++++++++++++++++++++++++ >  src/libxl/libxl_logger.h |  39 +++++++++ >  7 files changed, 274 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..ac83b51c7 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, (xentoollog_logger *)cfg->logger)) { >          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..d8ccdd852 > --- /dev/null > +++ b/src/libxl/libxl_logger.c > @@ -0,0 +1,223 @@ > +/* > + * 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(a)suse.com&gt; > + */ > +#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 = (xentoollog_logger_libvirt *)logger_in; > +    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 = (xentoollog_logger_libvirt*)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 = (xentoollog_logger*)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; > +    char ebuf[1024]; > + > +    if (virAsprintf(&path, "%s/%s.log", logger->logDir, name) < 0 || > +        virAsprintf(&domidstr, "%d", id) < 0) > +        goto cleanup; > + > +    if (!(logFile = fopen(path, "a"))) { > +        VIR_WARN("Failed to open log file %s: %s", > +                 path, virStrerror(errno, ebuf, sizeof(ebuf))); > +        goto cleanup; > +    } > +    ignore_value(virHashAddEntry(logger->files, domidstr, logFile)); > + > + cleanup: > +    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(a)suse.com&gt; > + */ > + > +#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 */