On Mon, Sep 12, 2016 at 02:12:10PM +0300, Nikolay Shirokovskiy wrote:
> Read API call of logger daemon is used to get extra
> information from terminated qemu process. For this purpose
> we need to wait until qemu process finishes its writing to log.
> We need to:
>
> 1. Read until EOF.
> 2. Don't stop reading on hangup.
>
> virtlogd will not receive EOF as both qemu and libvirtd
> keep logging pipe descriptor. Thus let's close it after
> qemu process startup is finished as it will not be used
> anymore. All following logging on behalf of qemu by libvirt
> is done either thru daemon API or by receiving new
> writing descriptor.
>
> Beware, qemuDomainLogContextFree is unref actually.
> ---
> src/logging/log_handler.c | 39 +++++++++++++++++++++++++++++++++++++--
> src/qemu/qemu_domain.c | 4 ++++
> src/qemu/qemu_domain.h | 1 +
> src/qemu/qemu_process.c | 2 ++
> 4 files changed, 44 insertions(+), 2 deletions(-)
Any virtlogd changes should really be in separate commits from
any QEMU changes. virtlogd is intended to be a reusable component
of libvirt for multiple use cases, so any changes to it need to
described & justifiable independently of any QEMU changes.
> +static virLogHandlerLogFilePtr
> +virLogHandlerGetLogFileFromPath(virLogHandlerPtr handler,
> + const char* path)
> +{
> + size_t i;
> +
> + for (i = 0; i < handler->nfiles; i++) {
> + if (STREQ(handler->files[i]->path, path))
> + return handler->files[i];
> + }
> +
> + return NULL;
> +}
> @@ -167,11 +185,14 @@ virLogHandlerDomainLogFileEvent(int watch,
> goto error;
> }
>
> + if (len == 0)
> + goto error;
> +
> if (virRotatingFileWriterAppend(logfile->file, buf, len) != len)
> goto error;
>
> if (events & VIR_EVENT_HANDLE_HANGUP)
> - goto error;
> + goto reread;
This patch hunk seems to be indendant of the other changes in this
file. It is a straightfoward bugfix to ensure we full read all data
before handling the hangup. So it should be a separate commit too.
>
> virObjectUnlock(handler);
> return;
> @@ -179,6 +200,7 @@ virLogHandlerDomainLogFileEvent(int watch,
> error:
> handler->inhibitor(false, handler->opaque);
> virLogHandlerLogFileClose(handler, logfile);
> + virCondBroadcast(&handler->condition);
> virObjectUnlock(handler);
> }
>
> @@ -198,6 +220,9 @@ virLogHandlerNew(bool privileged,
> if (!(handler = virObjectLockableNew(virLogHandlerClass)))
> goto error;
>
> + if (virCondInit(&handler->condition) < 0)
> + goto error;
> +
> handler->privileged = privileged;
> handler->max_size = max_size;
> handler->max_backups = max_backups;
> @@ -357,6 +382,7 @@ virLogHandlerDispose(void *obj)
> virLogHandlerLogFileFree(handler->files[i]);
> }
> VIR_FREE(handler->files);
> + virCondDestroy(&handler->condition);
> }
>
>
> @@ -401,7 +427,8 @@ virLogHandlerDomainOpenLogFile(virLogHandlerPtr handler,
> pipefd[0] = -1;
> memcpy(file->domuuid, domuuid, VIR_UUID_BUFLEN);
> if (VIR_STRDUP(file->driver, driver) < 0 ||
> - VIR_STRDUP(file->domname, domname) < 0)
> + VIR_STRDUP(file->domname, domname) < 0 ||
> + VIR_STRDUP(file->path, path) < 0)
> goto error;
>
> if ((file->file = virRotatingFileWriterNew(path,
> @@ -496,6 +523,14 @@ virLogHandlerDomainReadLogFile(virLogHandlerPtr handler,
>
> virObjectLock(handler);
>
> + while (virLogHandlerGetLogFileFromPath(handler, path)) {
> + if (virCondWait(&handler->condition, &handler->parent.lock) < 0) {
> + virReportSystemError(errno, "%s",
> + _("failed to wait for EOF"));
> + goto error;
> + }
> + }
Ewww no - this code is not only intended for use from codepath where
QEMU is shutdown. It absolutely should be able to read from a log
where QEMU is still running.
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