| Message ID | 1457954501-26528-4-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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(); } }
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(-)