[Qemu-devel] [PATCH] log: do not log if QEMU is daemonized but without -D

[Qemu-devel] [PATCH] log: do not log if QEMU is daemonized but without -D

[Paolo Bonzini]

Commit 96c33a4 ("log: Redirect stderr to logfile if deamonized",
2016-02-22) wanted to move stderr of a daemonized QEMU to the file
specified with -D.

However, if -D was not passed, the patch had the side effect of not
redirecting stderr to /dev/null.  This happened because qemu_logfile
was set to stderr rather than the expected value of NULL.  The fix
is simply in the "if" condition of do_qemu_set_log; the "if" for
closing the file is also changed to match.

Reported-by: Jan Tomko <jtomko@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 util/log.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/util/log.c b/util/log.c
index a7ddc7e..8b921de 100644
--- a/util/log.c
+++ b/util/log.c
@@ -56,7 +56,8 @@ 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_logfile &&
+        (is_daemonized() ? logfilename != NULL : qemu_loglevel)) {
         if (logfilename) {
             qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
             if (!qemu_logfile) {
@@ -72,6 +73,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
             }
         } else {
             /* Default to stderr if no log file specified */
+            assert(!is_daemonized());
             qemu_logfile = stderr;
         }
         /* must avoid mmap() usage of glibc by setting a buffer "by hand" */
@@ -89,7 +91,8 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
             log_append = 1;
         }
     }
-    if (!qemu_loglevel && !is_daemonized() && qemu_logfile) {
+    if (qemu_logfile &&
+        (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) {
         qemu_log_close();
     }
 }
-- 
2.5.0

Re: [Qemu-devel] [PATCH] log: do not log if QEMU is daemonized but without -D

[Cornelia Huck]

On Tue,  1 Mar 2016 12:50:53 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Commit 96c33a4 ("log: Redirect stderr to logfile if deamonized",
> 2016-02-22) wanted to move stderr of a daemonized QEMU to the file
> specified with -D.
> 
> However, if -D was not passed, the patch had the side effect of not
> redirecting stderr to /dev/null.  This happened because qemu_logfile
> was set to stderr rather than the expected value of NULL.  The fix
> is simply in the "if" condition of do_qemu_set_log; the "if" for
> closing the file is also changed to match.
> 
> Reported-by: Jan Tomko <jtomko@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/log.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

This fixes running qemu under libvirt for me.

Tested-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH] log: do not log if QEMU is daemonized but without -D

[Gerd Hoffmann]

On Di, 2016-03-01 at 12:50 +0100, Paolo Bonzini wrote:
> Commit 96c33a4 ("log: Redirect stderr to logfile if deamonized",
> 2016-02-22) wanted to move stderr of a daemonized QEMU to the file
> specified with -D.
> 
> However, if -D was not passed, the patch had the side effect of not
> redirecting stderr to /dev/null.  This happened because qemu_logfile
> was set to stderr rather than the expected value of NULL.  The fix
> is simply in the "if" condition of do_qemu_set_log; the "if" for
> closing the file is also changed to match.

Tested-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] log: do not log if QEMU is daemonized but without -D

[Dimitris Aragiorgis]

[-- Attachment #1: Type: text/plain, Size: 3346 bytes --]

Hi all,

* Paolo Bonzini <pbonzini@redhat.com> [2016-03-01 12:50:53 +0100]:

> Commit 96c33a4 ("log: Redirect stderr to logfile if deamonized",
> 2016-02-22) wanted to move stderr of a daemonized QEMU to the file
> specified with -D.
> 
> However, if -D was not passed, the patch had the side effect of not
> redirecting stderr to /dev/null.  This happened because qemu_logfile
> was set to stderr rather than the expected value of NULL.  The fix
> is simply in the "if" condition of do_qemu_set_log; the "if" for
> closing the file is also changed to match.
> 

Sorry, that was my bad.

I tested the proposed fix as well and it works. I would suggest to
add some sort of comments since the whole do_qemu_set_log() is hard
to parse in one pass. For example:

diff --git a/util/log.c b/util/log.c
index 8b921de..1d73be0 100644
--- a/util/log.c
+++ b/util/log.c
@@ -56,6 +56,10 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
 #ifdef CONFIG_TRACE_LOG
     qemu_loglevel |= LOG_TRACE;
 #endif
+    /* Setup the logfile when:
+     *  - The loglevel is set via -d option or CONFIG_TRACE_LOG, or
+     *  - Both -daemonize and -D options are given
+     */
     if (!qemu_logfile &&
         (is_daemonized() ? logfilename != NULL : qemu_loglevel)) {
         if (logfilename) {
@@ -91,6 +95,10 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
             log_append = 1;
         }
     }
+    /* Close the already setup logfile when loglevel gets reset (e.g.
+     * via the log HMP command), unless both -daemonize and -D options
+     * are given.
+     */
     if (qemu_logfile &&
         (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) {
         qemu_log_close();


Thanks for this quick fix and sorry again for any trouble caused in the first
place.

dimara

> Reported-by: Jan Tomko <jtomko@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/log.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/util/log.c b/util/log.c
> index a7ddc7e..8b921de 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -56,7 +56,8 @@ 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_logfile &&
> +        (is_daemonized() ? logfilename != NULL : qemu_loglevel)) {
>          if (logfilename) {
>              qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
>              if (!qemu_logfile) {
> @@ -72,6 +73,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>              }
>          } else {
>              /* Default to stderr if no log file specified */
> +            assert(!is_daemonized());
>              qemu_logfile = stderr;
>          }
>          /* must avoid mmap() usage of glibc by setting a buffer "by hand" */
> @@ -89,7 +91,8 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers)
>              log_append = 1;
>          }
>      }
> -    if (!qemu_loglevel && !is_daemonized() && qemu_logfile) {
> +    if (qemu_logfile &&
> +        (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) {
>          qemu_log_close();
>      }
>  }
> -- 
> 2.5.0
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]