Re: [Qemu-devel] [PATCH 1/1] log: fix hanged connect from virt-manager to libvirt

[Paolo Bonzini]

On 03/03/2016 14:48, Denis V. Lunev wrote:
> libvirt in this case spawns
>   /usr/bin/qemu-system-x86_64 -S -no-user-config -nodefaults -nographic
>     -M none
>     -qmp unix:/var/lib/libvirt/qemu/capabilities.monitor.sock,server,nowait
>     -pidfile /var/lib/libvirt/qemu/capabilities.pidfile -daemonize
> and with CONFIG_TRACE_LOG this process hangs as stderr becomes redirected
> to terminal (qemu_logfile == stderr). We do not have redirection to
> /dev/null in this case which is necessary.
> 
> Broken by:
>     commit 96c33a4523ee1abe382ce4ff3e82b90ba78aa186
>     Author: Dimitris Aragiorgis <address@hidden>
>     Date:   Thu Feb 18 13:38:38 2016 +0200
> 
>     log: Redirect stderr to logfile if deamonized
> 
> We should also take into account log filename change in runtime through
> QMP/HMP, when the log could be even closed. In this case stderr should
> be tweaked accordingly.
> 
> Signed-off-by: Denis V. Lunev <address@hidden>
> CC: Dimitris Aragiorgis <address@hidden>
> CC: Paolo Bonzini <address@hidden>
> CC: Vladimir Sementsov-Ogievskiy <address@hidden>

A patch has been posted already, and I'll send a pull request tomorrow.

Paolo

> ---
>  include/qemu/log.h | 4 ++++
>  os-posix.c         | 2 +-
>  util/log.c         | 7 ++-----
>  3 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index dda65fd..8083f82 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -92,6 +92,10 @@ static inline void qemu_log_close(void)
>          }
>          qemu_logfile = NULL;
>      }
> +
> +    if (is_daemonized()) {
> +        dup2(STDOUT_FILENO, STDERR_FILENO); /* dup /dev/null to stderr */
> +    }
>  }
>  
>  /* define log items */
> diff --git a/os-posix.c b/os-posix.c
> index 92fa3ba..d4b2a91 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -277,7 +277,7 @@ void os_setup_post(void)
>          dup2(fd, 0);
>          dup2(fd, 1);
>          /* In case -D is given do not redirect stderr to /dev/null */
> -        if (!qemu_logfile) {
> +        if (!qemu_logfile || qemu_logfile == stderr) {
>              dup2(fd, 2);
>          }
>  
> diff --git a/util/log.c b/util/log.c
> index a7ddc7e..a06aa14 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -56,7 +56,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>  #ifdef CONFIG_TRACE_LOG
>      qemu_loglevel |= LOG_TRACE;
>  #endif
> -    if ((qemu_loglevel || is_daemonized()) && !qemu_logfile) {
> +    if (qemu_loglevel && !qemu_logfile) {
>          if (logfilename) {
>              qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
>              if (!qemu_logfile) {
> @@ -66,9 +66,6 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>              /* In case we are a daemon redirect stderr to logfile */
>              if (is_daemonized()) {
>                  dup2(fileno(qemu_logfile), STDERR_FILENO);
> -                fclose(qemu_logfile);
> -                /* This will skip closing logfile in qemu_log_close() */
> -                qemu_logfile = stderr;
>              }
>          } else {
>              /* Default to stderr if no log file specified */
> @@ -89,7 +86,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>              log_append = 1;
>          }
>      }
> -    if (!qemu_loglevel && !is_daemonized() && qemu_logfile) {
> +    if (!qemu_loglevel && qemu_logfile) {
>          qemu_log_close();
>      }
>  }
>