Re: [libvirt] [PATCH 4/5] qemu: use virtlogd for character device log files

On 02/23/2016 11:41 AM, Daniel P. Berrange wrote: > If use of virtlogd is enabled, then use it for backing the > character device log files too. > --- > src/logging/log_daemon_dispatch.c | 3 +- > src/logging/log_handler.c | 6 +- > src/logging/log_handler.h | 2 +- > src/logging/log_manager.h | 2 + > src/logging/log_protocol.x | 4 + > src/qemu/qemu_command.c | 234 ++++++++++++++++++++++++-------------- <sigh> One of us will have conflicts depending upon whether my final pile of qemu_command.c cleanup changes gets reviewed... > src/qemu/qemu_command.h | 7 +- > src/qemu/qemu_domain.c | 6 + > src/qemu/qemu_domain.h | 3 + > src/qemu/qemu_driver.c | 2 +- > src/qemu/qemu_process.c | 4 +- > tests/qemuxml2argvtest.c | 2 +- > 12 files changed, 179 insertions(+), 96 deletions(-) > > diff --git a/src/logging/log_daemon_dispatch.c b/src/logging/log_daemon_dispatch.c > index a5fa7f0..b00cee2 100644 > --- a/src/logging/log_daemon_dispatch.c > +++ b/src/logging/log_daemon_dispatch.c > @@ -50,13 +50,14 @@ virLogManagerProtocolDispatchDomainOpenLogFile(virNetServerPtr server ATTRIBUTE_ > int rv = -1; > off_t offset; > ino_t inode; > + bool trunc = args->flags & VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE; > So by changing this means you don't envision other flags in the future for virLogHandlerDomainOpenLogFile? IOW: Why isn't this done in the next layer down? Seems we'd be changing what we're doing (not that anything else is using this yet).

Should perhaps this part be a separate patch? > if ((fd = virLogHandlerDomainOpenLogFile(virLogDaemonGetHandler(logDaemon), > args->driver, > (unsigned char *)args->dom.uuid, > args->dom.name, > args->path, > - args->flags, > + trunc, > &inode, &offset)) < 0) > goto cleanup; > > diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c > index 92cff50..4c08223 100644 > --- a/src/logging/log_handler.c > +++ b/src/logging/log_handler.c > @@ -357,7 +357,7 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler, > const unsigned char *domuuid, > const char *domname, > const char *path, > - unsigned int flags, > + bool trunc, > ino_t *inode, > off_t *offset) > { > @@ -365,8 +365,6 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler, > virLogHandlerLogFilePtr file = NULL; > int pipefd[2] = { -1, -1 }; > > - virCheckFlags(0, -1); > - > virObjectLock(handler); > > handler->inhibitor(true, handler->opaque); > @@ -400,7 +398,7 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler, > if ((file->file = virRotatingFileWriterNew(path, > DEFAULT_FILE_SIZE, > DEFAULT_MAX_BACKUP, > - false, > + trunc, > DEFAULT_MODE)) == NULL) > goto error; > > diff --git a/src/logging/log_handler.h b/src/logging/log_handler.h > index e61f32d..54a9cd9 100644 > --- a/src/logging/log_handler.h > +++ b/src/logging/log_handler.h > @@ -48,7 +48,7 @@ int virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler, > const unsigned char *domuuid, > const char *domname, > const char *path, > - unsigned int flags, > + bool trunc, > ino_t *inode, > off_t *offset); > > diff --git a/src/logging/log_manager.h b/src/logging/log_manager.h > index d3b9d29..7deaba7 100644 > --- a/src/logging/log_manager.h > +++ b/src/logging/log_manager.h > @@ -26,6 +26,8 @@ > > # include "internal.h" > > +# include "logging/log_protocol.h" > + > typedef struct _virLogManager virLogManager; > typedef virLogManager *virLogManagerPtr; > > diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x > index b0ac31b..0363c75 100644 > --- a/src/logging/log_protocol.x > +++ b/src/logging/log_protocol.x > @@ -30,6 +30,10 @@ struct virLogManagerProtocolLogFilePosition { > }; > typedef struct virLogManagerProtocolLogFilePosition virLogManagerProtocolLogFilePosition; > > +enum virLogManagerProtocolDomainOpenLogFileFlags { > + VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE = 1 > +}; > + DomainOpenLogFile doesn't have flags now - it just has trunc - so nothing could be added here (at least w/r/t how I understand it).

> +/** > + * qemuVirCommandGetFDSet: > + * @cmd: the command to modify > + * @fd: fd to reassign to the child > + * > + * Get the parameters for the QEMU -add-fd command line option > + * for the given file descriptor. The file descriptor must previously > + * have been 'transferred' in a virCommandPassFD() call. > + * This function for example returns "set=10,fd=20". > + */ > +static char * > +qemuVirCommandGetFDSet(virCommandPtr cmd, int fd) > +{ > + char *result = NULL; > + int idx = virCommandPassFDGetFDIndex(cmd, fd); > + > + if (idx >= 0) { > + ignore_value(virAsprintf(&result, "set=%d,fd=%d", idx, fd)); > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("file descriptor %d has not been transferred"), fd); > + } > + > + return result; > +} > + > + > +/** > + * qemuVirCommandGetDevSet: > + * @cmd: the command to modify > + * @fd: fd to reassign to the child > + * > + * Get the parameters for the QEMU path= parameter where a file > + * descriptor is accessed via a file descriptor set, for example > + * /dev/fdset/10. The file descriptor must previously have been > + * 'transferred' in a virCommandPassFD() call. > + */ > +static char * > +qemuVirCommandGetDevSet(virCommandPtr cmd, int fd) > +{ > + char *result = NULL; > + int idx = virCommandPassFDGetFDIndex(cmd, fd); > + > + if (idx >= 0) { > + ignore_value(virAsprintf(&result, "/dev/fdset/%d", idx)); > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("file descriptor %d has not been transferred"), fd); > + } > + return result; > +} > + > + [1] Code motion... Should it be separate?

> @@ -3973,10 +4030,42 @@ qemuBuildChrChardevStr(const virDomainChrSourceDef *dev, > _("logfile not supported in this QEMU binary")); > goto error; > } > - virBufferAsprintf(&buf, ",logfile=%s", dev->logfile); > - if (dev->logappend != VIR_TRISTATE_SWITCH_ABSENT) { > - virBufferAsprintf(&buf, ",logappend=%s", > - virTristateSwitchTypeToString(dev->logappend)); > + if (logManager) { > + char *fdset, *fdpath; > + int flags = 0; > + int logfd; > + > + if (dev->logappend == VIR_TRISTATE_SWITCH_OFF) > + flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE; > + > + if ((logfd = virLogManagerDomainOpenLogFile(logManager, > + "qemu", > + def->uuid, > + def->name, > + dev->logfile, > + flags, > + NULL, NULL)) < 0) > + goto error; > + > + virCommandPassFD(cmd, logfd, VIR_COMMAND_PASS_FD_CLOSE_PARENT); > + if (!(fdset = qemuVirCommandGetFDSet(cmd, logfd))) Would this error path need to close logfd or is that done because of the above PassFD path?

> + goto error; > + > + virCommandAddArg(cmd, "-add-fd"); > + virCommandAddArg(cmd, fdset); > + VIR_FREE(fdset); > + > + if (!(fdpath = qemuVirCommandGetDevSet(cmd, logfd))) Similar question about logfd... > + goto error; > + > + virBufferAsprintf(&buf, ",logfile=%s,logappend=on", fdpath); Always on? Doesn't matter what's been provided in logappend in the XML?

> @@ -4106,10 +4197,11 @@ qemuBuildMonitorCommandLine(virCommandPtr cmd, > /* Use -chardev if it's available */ > if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV)) { > > - virCommandAddArg(cmd, "-chardev"); > - if (!(chrdev = qemuBuildChrChardevStr(monitor_chr, "monitor", > + if (!(chrdev = qemuBuildChrChardevStr(logManager, cmd, def, > + monitor_chr, "monitor", > qemuCaps))) > return -1; > + virCommandAddArg(cmd, "-chardev"); doh! Although I suppose one could make the argument that all the additions of "-chardev" could be their own patch...