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

Hey Cedric, On 01/10/2017 09:03 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. > > Thanks to the default log file, this logger will also work with older > versions of Xen. This is great stuff! BTW, while applying the patch in my local repo, I noticed some issues (and confirmed make syntax-check is failing with them as well). But I assume those could be fixed on commit.

Joao > --- >  * v4: delay the log file opening to the first message write. > >  * v3: add JSON-formatted libxl_domain_config to the newly opened >        file when creating a guest. > >  * 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 |   7 ++ >  src/libxl/libxl_driver.c |   2 + >  src/libxl/libxl_logger.c | 257 +++++++++++++++++++++++++++++++++++++++++++++++ >  src/libxl/libxl_logger.h |  40 ++++++++ >  7 files changed, 312 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 0dd2faaf6..58e3108a6 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..fbe7ee5ff 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); >  } > @@ -1127,6 +1129,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, >      libxl_asyncprogress_how aop_console_how; >      libxl_domain_restore_params params; >      unsigned int hostdev_flags = VIR_HOSTDEV_SP_PCI; > +    char *config_json = NULL; >   >  #ifdef LIBXL_HAVE_PVUSB >      hostdev_flags |= VIR_HOSTDEV_SP_USB; > @@ -1290,6 +1293,9 @@ libxlDomainStart(libxlDriverPrivatePtr driver, >       * be cleaned up if there are any subsequent failures. >       */ >      vm->def->id = domid; > +    config_json = libxl_domain_config_to_json(cfg->ctx, &d_config); > + > +    libxlLoggerOpenFile(cfg->logger, domid, vm->def->name, config_json); >   >      /* Always enable domain death events */ >      if (libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW)) > @@ -1366,6 +1372,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, >   >   cleanup: >      libxl_domain_config_dispose(&d_config); > +    VIR_FREE(config_json); >      VIR_FREE(dom_xml); >      VIR_FREE(managed_save_path); >      virDomainDefFree(def); > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index 7e5d9b69e..325636c9a 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, NULL); > + >      /* 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..437dbb34b > --- /dev/null > +++ b/src/libxl/libxl_logger.c > @@ -0,0 +1,257 @@ > +/* > + * 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; > + > +typedef struct { > +    FILE *handle; > +    char *path; > +} libxlLoggerFile; > + > +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) > +{ > +    libxlLoggerFile *file = payload; > +    if (file->handle) { > +        VIR_FORCE_FCLOSE(file->handle); > +        file->handle = NULL; > +    } > +    VIR_FREE(file->path); > +} > + > +static int > +libxlLoggerFileOpen(libxlLoggerFile *file) {                                             ^^^^ newline here? > +    int ret = 0; > +    char ebuf[1024]; > + > +    if (!file->handle && !(file->handle = fopen(file->path, "a"))) { > +        VIR_WARN("Failed to open log file %s: %s", > +                 file->path, virStrerror(errno, ebuf, sizeof(ebuf))); > +        ret = -1; > +    } > + > +    return ret; > +} > + > +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, ":"))) { > +        libxlLoggerFile *file = NULL; > + > +        start = start + 9; > +        *end = '\0'; > + > +        file = virHashLookup(lg->files, start); > +        if (file) { > +            ignore_value(libxlLoggerFileOpen(file)); > +            logFile = file->handle; > +        } > + > +        *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 */ Wasn't migration code seeing progress reported using this xentoollog function? (see tools/libxl/libxl_save_callout.c:418) But then this is the save helper and I recall there was a pipe between libxl and the save helper child process which would send the output to the parent and libxl would wrap some other way. So I assume this is of no concern and the migration logging ends up in the same domain file (as one would expect) ? > +} > + > +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, > +                         const char *domain_config) > +{ > +    FILE *logFile = NULL; > +    char *domidstr = NULL; > +    libxlLoggerFile *file; > +        ^^^^ Nit: spurious whitespace here > +    if (VIR_ALLOC(file) < 0) > +        return; > + > +    if (virAsprintf(&file->path, "%s/%s.log", logger->logDir, name) < 0 || > +        virAsprintf(&domidstr, "%d", id) < 0) > +        goto error; > + > +    ignore_value(virHashAddEntry(logger->files, domidstr, file)); > + > +    /* domain_config is non NULL only when starting a new domain */ > +    if (domain_config) { > +        if (libxlLoggerFileOpen(file) < 0) > +            goto cleanup; > + > +        fprintf(logFile, "Domain start: %s\n", domain_config); > +        fflush(logFile); > +    } > +    goto cleanup; > + > + error: > +    VIR_FREE(file->path); > +    VIR_FREE(file); > + > + cleanup: > +    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..92b222578 > --- /dev/null > +++ b/src/libxl/libxl_logger.h > @@ -0,0 +1,40 @@ > +/* > + * 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, > +                         const char *domain_config); > +void libxlLoggerCloseFile(libxlLoggerPtr logger, int id); > + > +#endif /* __LIBXL_LOGGER_H */ >