Re: [PATCH v2 04/13] logging: add client for virtlogd daemon

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

 




On 11/12/2015 12:19 PM, Daniel P. Berrange wrote:
> Add the virLogManager API which allows for communication with
> the virtlogd daemon to RPC program. This provides the client
> side API to open log files for guest domains.
> 
> The virtlogd daemon is setup to auto-spawn on first use when
> running unprivileged. For privileged usage, systemd socket
> activation is used instead.
> 
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
>  po/POTFILES.in                    |   2 +
>  src/Makefile.am                   |   4 +-
>  src/libvirt_private.syms          |   8 ++
>  src/logging/log_daemon_dispatch.c |   7 +
>  src/logging/log_handler.c         |   7 +-
>  src/logging/log_manager.c         | 283 ++++++++++++++++++++++++++++++++++++++
>  src/logging/log_manager.h         |  61 ++++++++
>  src/logging/log_protocol.x        |   1 +
>  8 files changed, 370 insertions(+), 3 deletions(-)
>  create mode 100644 src/logging/log_manager.c
>  create mode 100644 src/logging/log_manager.h
> 

[...]

> diff --git a/src/logging/log_daemon_dispatch.c b/src/logging/log_daemon_dispatch.c
> index f612be1..203fdc4 100644
> --- a/src/logging/log_daemon_dispatch.c
> +++ b/src/logging/log_daemon_dispatch.c
> @@ -116,6 +116,13 @@ virLogManagerProtocolDispatchDomainReadLogFile(virNetServerPtr server ATTRIBUTE_
>      int rv = -1;
>      char *data;
>  
> +    if (args->maxlen > VIR_LOG_MANAGER_PROTOCOL_STRING_MAX) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Requested data len %zu is larger than maximum %d"),
> +                       args->maxlen, VIR_LOG_MANAGER_PROTOCOL_STRING_MAX);
> +        goto cleanup;
> +    }
> +

This feels like it should have been in prior patch...

>      if ((data = virLogHandlerDomainReadLogFile(virLogDaemonGetHandler(logDaemon),
>                                                 args->driver,
>                                                 (unsigned char *)args->dom.uuid,
> diff --git a/src/logging/log_handler.c b/src/logging/log_handler.c
> index 8853fd0..1465c5a 100644
> --- a/src/logging/log_handler.c
> +++ b/src/logging/log_handler.c
> @@ -442,6 +442,7 @@ char *virLogHandlerDomainReadLogFile(virLogHandlerPtr handler,
>      char *path;
>      virRotatingFileReaderPtr file = NULL;
>      char *data = NULL;
> +    ssize_t got;
>  
>      if (!(path = virLogHandlerGetLogFilePathForDomain(handler,
>                                                        driver,
> @@ -455,11 +456,13 @@ char *virLogHandlerDomainReadLogFile(virLogHandlerPtr handler,
>      if (virRotatingFileReaderSeek(file, inode, offset) < 0)
>          goto error;
>  
> -    if (VIR_ALLOC_N(data, maxlen) < 0)
> +    if (VIR_ALLOC_N(data, maxlen + 1) < 0)
>          goto error;
>  
> -    if (virRotatingFileReaderConsume(file, data, maxlen) < 0)
> +    got = virRotatingFileReaderConsume(file, data, maxlen);
> +    if (got < 0)
>          goto error;
> +    data[got] = '\0';

Perhaps another prior patch..

>  
>      virRotatingFileReaderFree(file);
>      return data;

[...]  Nothing jumped out at me in log_manager.{ch}

> diff --git a/src/logging/log_protocol.x b/src/logging/log_protocol.x
> index 2702beb..de57c69 100644
> --- a/src/logging/log_protocol.x
> +++ b/src/logging/log_protocol.x
> @@ -57,6 +57,7 @@ struct virLogManagerProtocolDomainReadLogFileArgs {
>      virLogManagerProtocolDomain dom;
>      virLogManagerProtocolLogFilePosition pos;
>      unsigned hyper maxlen;
> +    unsigned int flags;

should this perhaps have been in the prior patch?

>  };
>  
>  struct virLogManagerProtocolDomainReadLogFileRet {
> 

ACK - nothing major here - your call on whether things should move.

John

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