[libvirt] [PATCH 2/2] log: drain log of exiting qemu process carefully

[Daniel P. Berrange]

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 :|