On Fri, Feb 26, 2016 at 10:33:30AM -0500, John Ferlan wrote:
>
>
> 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).
This flag is defined by the remote protocol. I want to keep the
virLogHandler class isolated from the remote protocol, by not
exposing the flags to it. Hence I've gone for named arguments
in the internal API and convert from the flags to the named
arguemnts.
> 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).
The wire protocol *does* have flags defined currently:
struct virLogManagerProtocolDomainOpenLogFileArgs {
virLogManagerProtocolNonNullString driver;
virLogManagerProtocolDomain dom;
virLogManagerProtocolNonNullString path;
unsigned int flags;
};
this code is just defining the first flag we'll use on the wire.
> > +/**
> > + * 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?
Yeah, that would be best practice.
> > @@ -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?
Yep, because we pass VIR_COMMAND_PASS_FD_CLOSE_PARENT, we're guaranteed
that virCommandFree will always close the FD for us, no matter what
happens.
>
> > + 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?
The append vs truncate decision is taken by virtlogd - we pass that info
in the virLogManagerDomainOpenLogFile call flags parameter above.
At the QEMU level, it just gets an anonymous pipe file descriptor, which
can only ever be used in append mode - you can't call ftruncate() on a
pipe.
> > @@ -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...
Yeah, could be cleaner to separate the chardev move
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list