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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2017-01-10 at 12:21 +0000, Joao Martins wrote:
> 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.

Indeed, fixed it locally. Thanks for the heads up.
--
Cedric

> 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@xxxxxxxx>
> > + */
> > +#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@xxxxxxxx>
> > + */
> > +
> > +#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 */
> > 
> 
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux