[3/9] log: improve code in do_qemu_set_log
diff mbox

Message ID 1457954501-26528-4-git-send-email-den@openvz.org
State New
Headers show

Commit Message

Denis V. Lunev March 14, 2016, 11:21 a.m. UTC
The following commit
    commit 96c33a4523ee1abe382ce4ff3e82b90ba78aa186
    Author: Dimitris Aragiorgis <dimara@arrikto.com>
    Date:   Thu Feb 18 13:38:38 2016 +0200

    log: Redirect stderr to logfile if deamonized
was created with unnecessary side effect - connect from libvirt starts
to hang. This, in turn, was fixed by
    commit c586eac33670c198c6c9ceb1419aa99dafcce907
    Author: Paolo Bonzini <pbonzini@redhat.com>
    Date:   Mon Feb 29 12:18:40 2016 +0100

    log: do not log if QEMU is daemonized but without -D
but the fix seems a bit awkward to me.

This patch fixes the problem with a bit more readable approach and makes the
code closer to the original state.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/log.h |  4 ++++
 os-posix.c         |  2 +-
 util/log.c         | 10 ++--------
 3 files changed, 7 insertions(+), 9 deletions(-)

Comments

Paolo Bonzini March 14, 2016, 2:28 p.m. UTC | #1
On 14/03/2016 12:21, Denis V. Lunev wrote:
>          /* In case -D is given do not redirect stderr to /dev/null */
> -        if (!qemu_logfile) {
> +        if (!qemu_logfile || qemu_logfile == stderr) {
>              dup2(fd, 2);

This relies on knowledge that fileno(qemu_logfile) is dup-ed to stderr.
 I'm not sure what's the problem in commit c586eac33; the idea is that,
if -daemonize is given, a named logfile should always be open (so that
stderr is redirected) but stderr should not be used as log destination
(because that's just /dev/null).  That's clear from the condition:

   is_daemonized() ? logfilename != NULL : qemu_loglevel

Paolo
Denis V. Lunev March 16, 2016, 11:20 a.m. UTC | #2
On 03/14/2016 05:28 PM, Paolo Bonzini wrote:
>
> On 14/03/2016 12:21, Denis V. Lunev wrote:
>>           /* In case -D is given do not redirect stderr to /dev/null */
>> -        if (!qemu_logfile) {
>> +        if (!qemu_logfile || qemu_logfile == stderr) {
>>               dup2(fd, 2);
> This relies on knowledge that fileno(qemu_logfile) is dup-ed to stderr.
actually the comment 2 lines above is exactly about this :))))))

>   I'm not sure what's the problem in commit c586eac33; the idea is that,
> if -daemonize is given, a named logfile should always be open (so that
> stderr is redirected) but stderr should not be used as log destination
> (because that's just /dev/null).  That's clear from the condition:
>
>     is_daemonized() ? logfilename != NULL : qemu_loglevel
>
> Paolo
The question here is the following actually.

Should we continue to keep writing 'stderr' output to the log file
when we have cleared all log levels. If the answer is 'yes',
original == your code is correct while my is wrong. In the
other case - the situation becomes mirrored.

Den
Paolo Bonzini March 16, 2016, 11:25 a.m. UTC | #3
On 16/03/2016 12:20, Denis V. Lunev wrote:
> The question here is the following actually.
> 
> Should we continue to keep writing 'stderr' output to the log file
> when we have cleared all log levels. If the answer is 'yes',
> original == your code is correct while my is wrong. In the
> other case - the situation becomes mirrored.

You're right about the actual question to answer.

I think the answer is yes, given the original purpose of Dimitris's
patch.  The behavior of "-daemonize -D foo" should be the same if QEMU
is configured with or without the "log" trace backend.

Paolo

Patch
diff mbox

diff --git a/include/qemu/log.h b/include/qemu/log.h
index 40c24fd..be20811 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -88,6 +88,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 8b921de..a06aa14 100644
--- a/util/log.c
+++ b/util/log.c
@@ -56,8 +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_logfile &&
-        (is_daemonized() ? logfilename != NULL : qemu_loglevel)) {
+    if (qemu_loglevel && !qemu_logfile) {
         if (logfilename) {
             qemu_logfile = fopen(logfilename, log_append ? "a" : "w");
             if (!qemu_logfile) {
@@ -67,13 +66,9 @@  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 */
-            assert(!is_daemonized());
             qemu_logfile = stderr;
         }
         /* must avoid mmap() usage of glibc by setting a buffer "by hand" */
@@ -91,8 +86,7 @@  void do_qemu_set_log(int log_flags, bool use_own_buffers)
             log_append = 1;
         }
     }
-    if (qemu_logfile &&
-        (is_daemonized() ? logfilename == NULL : !qemu_loglevel)) {
+    if (!qemu_loglevel && qemu_logfile) {
         qemu_log_close();
     }
 }