* [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize @ 2015-12-16 16:56 Alex Pyrgiotis 2016-02-08 9:29 ` Paolo Bonzini 0 siblings, 1 reply; 17+ messages in thread From: Alex Pyrgiotis @ 2015-12-16 16:56 UTC (permalink / raw) To: qemu-devel From: Dimitris Aragiorgis <dimara@arrikto.com> In case of daemonize, use the logfile passed with the -D option in order to redirect stdout/stderr to a file instead of /dev/null. Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com> Signed-off-by: Alex Pyrgiotis <apyrgio@arrikto.com> diff --git a/include/qemu/log.h b/include/qemu/log.h index 362cbc4..b5de1df 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -175,6 +175,7 @@ static inline void qemu_set_log(int log_flags) } void qemu_set_log_filename(const char *filename); +char *qemu_get_log_filename(void); int qemu_str_to_log_mask(const char *str); /* Print a usage message listing all the valid logging categories diff --git a/os-posix.c b/os-posix.c index e4da406..18052fb 100644 --- a/os-posix.c +++ b/os-posix.c @@ -40,6 +40,7 @@ #include "net/slirp.h" #include "qemu-options.h" #include "qemu/rcu.h" +#include "qemu/log.h" #ifdef CONFIG_LINUX #include <sys/prctl.h> @@ -255,16 +256,25 @@ void os_daemonize(void) void os_setup_post(void) { int fd = 0; + char *log = NULL; if (daemonize) { if (chdir("/")) { perror("not able to chdir to /"); exit(1); } - TFR(fd = qemu_open("/dev/null", O_RDWR)); + + log = qemu_get_log_filename(); + if (log != NULL) { + TFR(fd = qemu_open(log, O_RDWR | O_APPEND | O_CREAT, 0640)); + } else { + TFR(fd = qemu_open("/dev/null", O_RDWR)); + } if (fd == -1) { + fprintf(stderr, "Cannot open \"%s\" for logging\n", log); exit(1); } + g_free(log); } change_root(); diff --git a/qemu-log.c b/qemu-log.c index 7cb01a8..44447bc 100644 --- a/qemu-log.c +++ b/qemu-log.c @@ -90,6 +90,11 @@ void qemu_set_log_filename(const char *filename) qemu_set_log(qemu_loglevel); } +char *qemu_get_log_filename(void) +{ + return g_strdup(logfilename); +} + const QEMULogItem qemu_log_items[] = { { CPU_LOG_TB_OUT_ASM, "out_asm", "show generated host assembly code for each compiled TB" }, -- 2.6.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize 2015-12-16 16:56 [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize Alex Pyrgiotis @ 2016-02-08 9:29 ` Paolo Bonzini 2016-02-11 12:12 ` Dimitris Aragiorgis 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2016-02-08 9:29 UTC (permalink / raw) To: Alex Pyrgiotis, qemu-devel On 16/12/2015 17:56, Alex Pyrgiotis wrote: > + > + log = qemu_get_log_filename(); > + if (log != NULL) { > + TFR(fd = qemu_open(log, O_RDWR | O_APPEND | O_CREAT, 0640)); Here you are opening the same file twice, but the FILE* that is opened in do_qemu_set_log does not necessarily have O_APPEND. I like the idea of moving stderr to the logfile, but I'm not sure how to do it. For now, can you prepare a simple patch that only does the "dup" if "isatty" returns true for the file descriptor? This lets you use redirection at the shell level to save the stdout and stderr. Paolo > + } else { > + TFR(fd = qemu_open("/dev/null", O_RDWR)); > + } > if (fd == -1) { > + fprintf(stderr, "Cannot open \"%s\" for logging\n", log); > exit(1); > } > + g_free(log); > } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize 2016-02-08 9:29 ` Paolo Bonzini @ 2016-02-11 12:12 ` Dimitris Aragiorgis 2016-02-11 12:31 ` Paolo Bonzini 0 siblings, 1 reply; 17+ messages in thread From: Dimitris Aragiorgis @ 2016-02-11 12:12 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Alex Pyrgiotis, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2798 bytes --] Hi, * Paolo Bonzini <pbonzini@redhat.com> [2016-02-08 10:29:55 +0100]: > > > On 16/12/2015 17:56, Alex Pyrgiotis wrote: > > + > > + log = qemu_get_log_filename(); > > + if (log != NULL) { > > + TFR(fd = qemu_open(log, O_RDWR | O_APPEND | O_CREAT, 0640)); > > Here you are opening the same file twice, but the FILE* that is opened > in do_qemu_set_log does not necessarily have O_APPEND. > This is partially true :) I am opening this file twice only if the -d option is passed along with -D. Still, point taken. > I like the idea of moving stderr to the logfile, but I'm not sure how to > do it. For now, can you prepare a simple patch that only does the "dup" > if "isatty" returns true for the file descriptor? This lets you use > redirection at the shell level to save the stdout and stderr. > This could be a workaround. Still, I think it is a bit unorthodox to change the core behavior of logging depending on the type of output (tty or not). I understand that checking the type of output is sometimes used to enable/disable colors automatically, for example. Besides that, when one executes a daemon, shell redirection is hardly, if ever, used. More so if the daemon already has a logfile option. So, we decided to give it a go and find the least painful way to log the stderr of a QEMU process to a logfile. To our understanding, the logfile (-D option) is used only for messages generated by qemu_log()/qemu_log_mask(). The current situation however is that fprintf(stderr, ...) is used in various places throughout the codebase for logging/debug purposes. A simple solution would be to redirect the stderr to the logfile when -D is used, but this may confuse people who expect the current behavior for their logfiles. Therefore, our suggestion is to introduce another option, "-log-stderr". If this is given then we can 'dup2' stderr to the already opened logfile inside do_qemu_set_log(). And we should not close it even if -d is not given. Afterwards, os_setup_post() in case of daemonize, would always dup2 0, 1 to /dev/null and only if qemu_logfile is not NULL would dup2 2 to /dev/null as well. To sum up if one wants to log stderr to the logfile, one should pass -D along with -log-stderr. Eventually qemu_log(), qemu_log_mask(), and fprintf(stderr, ...) will end up writing to our logfile. The -daemonize option should respect the other options. What do you think? dimara > Paolo > > > + } else { > > + TFR(fd = qemu_open("/dev/null", O_RDWR)); > > + } > > if (fd == -1) { > > + fprintf(stderr, "Cannot open \"%s\" for logging\n", log); > > exit(1); > > } > > + g_free(log); > > } > [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize 2016-02-11 12:12 ` Dimitris Aragiorgis @ 2016-02-11 12:31 ` Paolo Bonzini 2016-02-11 16:49 ` Dimitris Aragiorgis 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2016-02-11 12:31 UTC (permalink / raw) To: Dimitris Aragiorgis; +Cc: Alex Pyrgiotis, qemu-devel On 11/02/2016 13:12, Dimitris Aragiorgis wrote: > Besides that, when one executes a daemon, shell redirection is > hardly, if ever, used. More so if the daemon already has a logfile > option. > > So, we decided to give it a go and find the least painful way to > log the stderr of a QEMU process to a logfile. > > To our understanding, the logfile (-D option) is used only for > messages generated by qemu_log()/qemu_log_mask(). The current > situation however is that fprintf(stderr, ...) is used in various > places throughout the codebase for logging/debug purposes. Right, mostly through error_report. Actually I like your approach (log to -D if daemonize is used). I just was not sure of the best way to implement it. Perhaps when the logfile is opened you can replace the straight fopen with qemu_logfile = fopen(...); if (daemonized) { dup2(fileno(qemu_logfile), STDERR_FILENO); fclose(qemu_logfile); qemu_logfile = stderr; } Then the logfile will never be closed by qemu_log_close, and stderr will always be sent to it. Does this look sane? Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize 2016-02-11 12:31 ` Paolo Bonzini @ 2016-02-11 16:49 ` Dimitris Aragiorgis 2016-02-11 17:56 ` Paolo Bonzini 0 siblings, 1 reply; 17+ messages in thread From: Dimitris Aragiorgis @ 2016-02-11 16:49 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Alex Pyrgiotis, qemu-devel [-- Attachment #1: Type: text/plain, Size: 2106 bytes --] Hi, * Paolo Bonzini <pbonzini@redhat.com> [2016-02-11 13:31:17 +0100]: > > > On 11/02/2016 13:12, Dimitris Aragiorgis wrote: > > Besides that, when one executes a daemon, shell redirection is > > hardly, if ever, used. More so if the daemon already has a logfile > > option. > > > > So, we decided to give it a go and find the least painful way to > > log the stderr of a QEMU process to a logfile. > > > > To our understanding, the logfile (-D option) is used only for > > messages generated by qemu_log()/qemu_log_mask(). The current > > situation however is that fprintf(stderr, ...) is used in various > > places throughout the codebase for logging/debug purposes. > > Right, mostly through error_report. > > Actually I like your approach (log to -D if daemonize is used). I > just was not sure of the best way to implement it. > Well my apprach was based on the extra option so that the current behavior does not change and logfiles become too noisy out of the blue. I have a patch already that: - modifies do_qemu_set_log() so that if -log-stderr is given to: + open the logfile + dup stderr to it + skip qemu_log_close() - modifies os_setup_post() to dup stderr to /dev/null only if qemu_logfile is NULL. If we do not want this extra option, I guess we are talking about logging stderr to -D *only* if daemonize is used, right? > Perhaps when the logfile is opened you can replace the straight fopen with > > qemu_logfile = fopen(...); > if (daemonized) { > dup2(fileno(qemu_logfile), STDERR_FILENO); > fclose(qemu_logfile); > qemu_logfile = stderr; > } > > Then the logfile will never be closed by qemu_log_close, and stderr > will always be sent to it. Does this look sane? > I guess the above snippet will go in do_qemu_set_log(), right? If true, we have to open the logfile even if the -d option is not given. Besides that, log.c should become aware of daemonize. Depending on your answers I'll prepare the corresponding patch. Thanks, dimara > Paolo [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize 2016-02-11 16:49 ` Dimitris Aragiorgis @ 2016-02-11 17:56 ` Paolo Bonzini 2016-02-18 11:38 ` [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized Dimitris Aragiorgis 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2016-02-11 17:56 UTC (permalink / raw) To: Dimitris Aragiorgis; +Cc: Alex Pyrgiotis, qemu-devel -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 On 11/02/2016 17:49, Dimitris Aragiorgis wrote: >> Perhaps when the logfile is opened you can replace the straight >> fopen with >> >> qemu_logfile = fopen(...); if (daemonized) { >> dup2(fileno(qemu_logfile), STDERR_FILENO); fclose(qemu_logfile); >> qemu_logfile = stderr; } >> >> Then the logfile will never be closed by qemu_log_close, and >> stderr will always be sent to it. Does this look sane? >> > > I guess the above snippet will go in do_qemu_set_log(), right? > > If true, we have to open the logfile even if the -d option is not > given. Right, but only if daemonize. > Besides that, log.c should become aware of daemonize. Yes. Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-02-11 17:56 ` Paolo Bonzini @ 2016-02-18 11:38 ` Dimitris Aragiorgis 2016-02-18 15:22 ` Paolo Bonzini 2016-03-01 11:15 ` Gerd Hoffmann 0 siblings, 2 replies; 17+ messages in thread From: Dimitris Aragiorgis @ 2016-02-18 11:38 UTC (permalink / raw) To: qemu-devel; +Cc: pbonzini In case of daemonize, use the logfile passed with the -D option in order to redirect stderr to it instead of /dev/null. Also remove some unused code in log.h. Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com> --- include/qemu/log.h | 6 ------ os-posix.c | 6 +++++- util/log.c | 11 +++++++++-- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/include/qemu/log.h b/include/qemu/log.h index 30817f7..dda65fd 100644 --- a/include/qemu/log.h +++ b/include/qemu/log.h @@ -94,12 +94,6 @@ static inline void qemu_log_close(void) } } -/* Set up a new log file */ -static inline void qemu_log_set_file(FILE *f) -{ - qemu_logfile = f; -} - /* define log items */ typedef struct QEMULogItem { int mask; diff --git a/os-posix.c b/os-posix.c index cce62ed..92fa3ba 100644 --- a/os-posix.c +++ b/os-posix.c @@ -37,6 +37,7 @@ #include "qemu-options.h" #include "qemu/rcu.h" #include "qemu/error-report.h" +#include "qemu/log.h" #ifdef CONFIG_LINUX #include <sys/prctl.h> @@ -275,7 +276,10 @@ void os_setup_post(void) dup2(fd, 0); dup2(fd, 1); - dup2(fd, 2); + /* In case -D is given do not redirect stderr to /dev/null */ + if (!qemu_logfile) { + dup2(fd, 2); + } close(fd); diff --git a/util/log.c b/util/log.c index 2709e98..a7ddc7e 100644 --- a/util/log.c +++ b/util/log.c @@ -56,13 +56,20 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) #ifdef CONFIG_TRACE_LOG qemu_loglevel |= LOG_TRACE; #endif - if (qemu_loglevel && !qemu_logfile) { + if ((qemu_loglevel || is_daemonized()) && !qemu_logfile) { if (logfilename) { qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); if (!qemu_logfile) { perror(logfilename); _exit(1); } + /* 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 */ qemu_logfile = stderr; @@ -82,7 +89,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) log_append = 1; } } - if (!qemu_loglevel && qemu_logfile) { + if (!qemu_loglevel && !is_daemonized() && qemu_logfile) { qemu_log_close(); } } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-02-18 11:38 ` [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized Dimitris Aragiorgis @ 2016-02-18 15:22 ` Paolo Bonzini 2016-02-18 17:12 ` Dimitris Aragiorgis 2016-03-01 11:15 ` Gerd Hoffmann 1 sibling, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2016-02-18 15:22 UTC (permalink / raw) To: Dimitris Aragiorgis, qemu-devel On 18/02/2016 12:38, Dimitris Aragiorgis wrote: > In case of daemonize, use the logfile passed with the -D option in > order to redirect stderr to it instead of /dev/null. > > Also remove some unused code in log.h. > > Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com> > --- > include/qemu/log.h | 6 ------ > os-posix.c | 6 +++++- > util/log.c | 11 +++++++++-- > 3 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/include/qemu/log.h b/include/qemu/log.h > index 30817f7..dda65fd 100644 > --- a/include/qemu/log.h > +++ b/include/qemu/log.h > @@ -94,12 +94,6 @@ static inline void qemu_log_close(void) > } > } > > -/* Set up a new log file */ > -static inline void qemu_log_set_file(FILE *f) > -{ > - qemu_logfile = f; > -} > - > /* define log items */ > typedef struct QEMULogItem { > int mask; > diff --git a/os-posix.c b/os-posix.c > index cce62ed..92fa3ba 100644 > --- a/os-posix.c > +++ b/os-posix.c > @@ -37,6 +37,7 @@ > #include "qemu-options.h" > #include "qemu/rcu.h" > #include "qemu/error-report.h" > +#include "qemu/log.h" > > #ifdef CONFIG_LINUX > #include <sys/prctl.h> > @@ -275,7 +276,10 @@ void os_setup_post(void) > > dup2(fd, 0); > dup2(fd, 1); > - dup2(fd, 2); > + /* In case -D is given do not redirect stderr to /dev/null */ > + if (!qemu_logfile) { > + dup2(fd, 2); > + } > > close(fd); > > diff --git a/util/log.c b/util/log.c > index 2709e98..a7ddc7e 100644 > --- a/util/log.c > +++ b/util/log.c > @@ -56,13 +56,20 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) > #ifdef CONFIG_TRACE_LOG > qemu_loglevel |= LOG_TRACE; > #endif > - if (qemu_loglevel && !qemu_logfile) { > + if ((qemu_loglevel || is_daemonized()) && !qemu_logfile) { > if (logfilename) { > qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); > if (!qemu_logfile) { > perror(logfilename); > _exit(1); > } > + /* 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 */ > qemu_logfile = stderr; > @@ -82,7 +89,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) > log_append = 1; > } > } > - if (!qemu_loglevel && qemu_logfile) { > + if (!qemu_loglevel && !is_daemonized() && qemu_logfile) { > qemu_log_close(); > } Why is this necessary? Perhaps qemu_log_close should dup(1,2) if QEMU is daemonized. The rest looks great. Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-02-18 15:22 ` Paolo Bonzini @ 2016-02-18 17:12 ` Dimitris Aragiorgis 2016-02-19 17:25 ` Paolo Bonzini 0 siblings, 1 reply; 17+ messages in thread From: Dimitris Aragiorgis @ 2016-02-18 17:12 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel [-- Attachment #1: Type: text/plain, Size: 3645 bytes --] * Paolo Bonzini <pbonzini@redhat.com> [2016-02-18 16:22:13 +0100]: > > > On 18/02/2016 12:38, Dimitris Aragiorgis wrote: > > In case of daemonize, use the logfile passed with the -D option in > > order to redirect stderr to it instead of /dev/null. > > > > Also remove some unused code in log.h. > > > > Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com> > > --- > > include/qemu/log.h | 6 ------ > > os-posix.c | 6 +++++- > > util/log.c | 11 +++++++++-- > > 3 files changed, 14 insertions(+), 9 deletions(-) > > > > diff --git a/include/qemu/log.h b/include/qemu/log.h > > index 30817f7..dda65fd 100644 > > --- a/include/qemu/log.h > > +++ b/include/qemu/log.h > > @@ -94,12 +94,6 @@ static inline void qemu_log_close(void) > > } > > } > > > > -/* Set up a new log file */ > > -static inline void qemu_log_set_file(FILE *f) > > -{ > > - qemu_logfile = f; > > -} > > - > > /* define log items */ > > typedef struct QEMULogItem { > > int mask; > > diff --git a/os-posix.c b/os-posix.c > > index cce62ed..92fa3ba 100644 > > --- a/os-posix.c > > +++ b/os-posix.c > > @@ -37,6 +37,7 @@ > > #include "qemu-options.h" > > #include "qemu/rcu.h" > > #include "qemu/error-report.h" > > +#include "qemu/log.h" > > > > #ifdef CONFIG_LINUX > > #include <sys/prctl.h> > > @@ -275,7 +276,10 @@ void os_setup_post(void) > > > > dup2(fd, 0); > > dup2(fd, 1); > > - dup2(fd, 2); > > + /* In case -D is given do not redirect stderr to /dev/null */ > > + if (!qemu_logfile) { > > + dup2(fd, 2); > > + } > > > > close(fd); > > > > diff --git a/util/log.c b/util/log.c > > index 2709e98..a7ddc7e 100644 > > --- a/util/log.c > > +++ b/util/log.c > > @@ -56,13 +56,20 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) > > #ifdef CONFIG_TRACE_LOG > > qemu_loglevel |= LOG_TRACE; > > #endif > > - if (qemu_loglevel && !qemu_logfile) { > > + if ((qemu_loglevel || is_daemonized()) && !qemu_logfile) { > > if (logfilename) { > > qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); > > if (!qemu_logfile) { > > perror(logfilename); > > _exit(1); > > } > > + /* 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 */ > > qemu_logfile = stderr; > > @@ -82,7 +89,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) > > log_append = 1; > > } > > } > > - if (!qemu_loglevel && qemu_logfile) { > > + if (!qemu_loglevel && !is_daemonized() && qemu_logfile) { > > qemu_log_close(); > > } > > Why is this necessary? Perhaps qemu_log_close should dup(1,2) if QEMU > is daemonized. The rest looks great. > Without !is_daemonized(), if we use -daemon with -D without -d, qemu_log_close() will eventually set qemu_logfile to NULL. This will make os_setup_post() redirect stderr to /dev/null, which is unwanted. To be honest, I don't understand your suggestion. How would calling dup2(1, 2) help in our case? Isn't fd 1 the standard output? Thanks, dimara > Paolo [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-02-18 17:12 ` Dimitris Aragiorgis @ 2016-02-19 17:25 ` Paolo Bonzini 0 siblings, 0 replies; 17+ messages in thread From: Paolo Bonzini @ 2016-02-19 17:25 UTC (permalink / raw) To: Dimitris Aragiorgis; +Cc: qemu-devel On 18/02/2016 18:12, Dimitris Aragiorgis wrote: > * Paolo Bonzini <pbonzini@redhat.com> [2016-02-18 16:22:13 +0100]: > >> >> >> On 18/02/2016 12:38, Dimitris Aragiorgis wrote: >>> In case of daemonize, use the logfile passed with the -D option in >>> order to redirect stderr to it instead of /dev/null. >>> >>> Also remove some unused code in log.h. >>> >>> Signed-off-by: Dimitris Aragiorgis <dimara@arrikto.com> >>> --- >>> include/qemu/log.h | 6 ------ >>> os-posix.c | 6 +++++- >>> util/log.c | 11 +++++++++-- >>> 3 files changed, 14 insertions(+), 9 deletions(-) >>> >>> diff --git a/include/qemu/log.h b/include/qemu/log.h >>> index 30817f7..dda65fd 100644 >>> --- a/include/qemu/log.h >>> +++ b/include/qemu/log.h >>> @@ -94,12 +94,6 @@ static inline void qemu_log_close(void) >>> } >>> } >>> >>> -/* Set up a new log file */ >>> -static inline void qemu_log_set_file(FILE *f) >>> -{ >>> - qemu_logfile = f; >>> -} >>> - >>> /* define log items */ >>> typedef struct QEMULogItem { >>> int mask; >>> diff --git a/os-posix.c b/os-posix.c >>> index cce62ed..92fa3ba 100644 >>> --- a/os-posix.c >>> +++ b/os-posix.c >>> @@ -37,6 +37,7 @@ >>> #include "qemu-options.h" >>> #include "qemu/rcu.h" >>> #include "qemu/error-report.h" >>> +#include "qemu/log.h" >>> >>> #ifdef CONFIG_LINUX >>> #include <sys/prctl.h> >>> @@ -275,7 +276,10 @@ void os_setup_post(void) >>> >>> dup2(fd, 0); >>> dup2(fd, 1); >>> - dup2(fd, 2); >>> + /* In case -D is given do not redirect stderr to /dev/null */ >>> + if (!qemu_logfile) { >>> + dup2(fd, 2); >>> + } >>> >>> close(fd); >>> >>> diff --git a/util/log.c b/util/log.c >>> index 2709e98..a7ddc7e 100644 >>> --- a/util/log.c >>> +++ b/util/log.c >>> @@ -56,13 +56,20 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) >>> #ifdef CONFIG_TRACE_LOG >>> qemu_loglevel |= LOG_TRACE; >>> #endif >>> - if (qemu_loglevel && !qemu_logfile) { >>> + if ((qemu_loglevel || is_daemonized()) && !qemu_logfile) { >>> if (logfilename) { >>> qemu_logfile = fopen(logfilename, log_append ? "a" : "w"); >>> if (!qemu_logfile) { >>> perror(logfilename); >>> _exit(1); >>> } >>> + /* 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 */ >>> qemu_logfile = stderr; >>> @@ -82,7 +89,7 @@ void do_qemu_set_log(int log_flags, bool use_own_buffers) >>> log_append = 1; >>> } >>> } >>> - if (!qemu_loglevel && qemu_logfile) { >>> + if (!qemu_loglevel && !is_daemonized() && qemu_logfile) { >>> qemu_log_close(); >>> } >> >> Why is this necessary? Perhaps qemu_log_close should dup(1,2) if QEMU >> is daemonized. The rest looks great. >> > > Without !is_daemonized(), if we use -daemon with -D without -d, > qemu_log_close() will eventually set qemu_logfile to NULL. This > will make os_setup_post() redirect stderr to /dev/null, which > is unwanted. Sorry, I missed the context of this hunk. The patch is okay, thanks for putting up with me! Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-02-18 11:38 ` [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized Dimitris Aragiorgis 2016-02-18 15:22 ` Paolo Bonzini @ 2016-03-01 11:15 ` Gerd Hoffmann 2016-03-01 11:47 ` Daniel P. Berrange 2016-03-01 11:50 ` Paolo Bonzini 1 sibling, 2 replies; 17+ messages in thread From: Gerd Hoffmann @ 2016-03-01 11:15 UTC (permalink / raw) To: Dimitris Aragiorgis; +Cc: pbonzini, qemu-devel On Do, 2016-02-18 at 13:38 +0200, Dimitris Aragiorgis wrote: > In case of daemonize, use the logfile passed with the -D option in > order to redirect stderr to it instead of /dev/null. > > Also remove some unused code in log.h. Patch breaks interaction with libvirt. libvirt hangs on startup, while probing qemu capabilities. qemu runs in probing mode (command line is "/home/kraxel/projects/qemu/build-default/x86_64-softmmu/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" according to "systemctl status libvirtd -l"), apparently both qemu and libvirt wait for each other. cheers, Gerd ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-03-01 11:15 ` Gerd Hoffmann @ 2016-03-01 11:47 ` Daniel P. Berrange 2016-03-01 11:51 ` Paolo Bonzini 2016-03-01 11:50 ` Paolo Bonzini 1 sibling, 1 reply; 17+ messages in thread From: Daniel P. Berrange @ 2016-03-01 11:47 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: pbonzini, Dimitris Aragiorgis, qemu-devel On Tue, Mar 01, 2016 at 12:15:21PM +0100, Gerd Hoffmann wrote: > On Do, 2016-02-18 at 13:38 +0200, Dimitris Aragiorgis wrote: > > In case of daemonize, use the logfile passed with the -D option in > > order to redirect stderr to it instead of /dev/null. > > > > Also remove some unused code in log.h. > > Patch breaks interaction with libvirt. libvirt hangs on startup, while > probing qemu capabilities. qemu runs in probing mode (command line is > "/home/kraxel/projects/qemu/build-default/x86_64-softmmu/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" according to "systemctl status libvirtd -l"), apparently both qemu and libvirt wait for each other. When libvirt is probing capabilities it passes a pipe file descriptor for stderr. It reads from this pipe to detect any errors printed by QEMU before daemonizing. When QEMU daemonizes, it closes this FD and sets stderr to /dev/null. Libvirt knows qemu has successfully started up at this point. With this patch though, this code: @@ -275,7 +276,10 @@ void os_setup_post(void) dup2(fd, 0); dup2(fd, 1); - dup2(fd, 2); + /* In case -D is given do not redirect stderr to /dev/null */ + if (!qemu_logfile) { + dup2(fd, 2); + } close(fd); means that QEMU will never close stderr anymore, so libvirt things QEMU is still starting up....forever. Given current libvirt behaviour / expectations, I think the only option is to revert this change. IMHO if applications want qemu logs to go to stderr, they should explicitly ask for that to happen via a CLI arg. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-03-01 11:47 ` Daniel P. Berrange @ 2016-03-01 11:51 ` Paolo Bonzini 2016-03-01 11:58 ` Daniel P. Berrange 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2016-03-01 11:51 UTC (permalink / raw) To: Daniel P. Berrange, Gerd Hoffmann; +Cc: Dimitris Aragiorgis, qemu-devel On 01/03/2016 12:47, Daniel P. Berrange wrote: > means that QEMU will never close stderr anymore, so libvirt things > QEMU is still starting up....forever. > > Given current libvirt behaviour / expectations, I think the only > option is to revert this change. Why not fix it instead? :) Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-03-01 11:51 ` Paolo Bonzini @ 2016-03-01 11:58 ` Daniel P. Berrange 2016-03-01 12:03 ` Paolo Bonzini 0 siblings, 1 reply; 17+ messages in thread From: Daniel P. Berrange @ 2016-03-01 11:58 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, Gerd Hoffmann, Dimitris Aragiorgis On Tue, Mar 01, 2016 at 12:51:03PM +0100, Paolo Bonzini wrote: > > > On 01/03/2016 12:47, Daniel P. Berrange wrote: > > means that QEMU will never close stderr anymore, so libvirt things > > QEMU is still starting up....forever. > > > > Given current libvirt behaviour / expectations, I think the only > > option is to revert this change. > > Why not fix it instead? :) Because if the user upgrades QEMU it'll cause all existing deployed version of libvirt to hang. I think that's pretty undesirable behaviour. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-03-01 11:58 ` Daniel P. Berrange @ 2016-03-01 12:03 ` Paolo Bonzini 2016-03-01 13:54 ` Daniel P. Berrange 0 siblings, 1 reply; 17+ messages in thread From: Paolo Bonzini @ 2016-03-01 12:03 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: qemu-devel, Gerd Hoffmann, Dimitris Aragiorgis On 01/03/2016 12:58, Daniel P. Berrange wrote: > > > means that QEMU will never close stderr anymore, so libvirt things > > > QEMU is still starting up....forever. > > > > > > Given current libvirt behaviour / expectations, I think the only > > > option is to revert this change. > > > > Why not fix it instead? :) > > Because if the user upgrades QEMU it'll cause all existing deployed > version of libvirt to hang. I think that's pretty undesirable > behaviour. Let me rephrase, why not fix it in QEMU? Jan Tomko helped me debugging it, and I've sent a patch just before seeing your message. Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-03-01 12:03 ` Paolo Bonzini @ 2016-03-01 13:54 ` Daniel P. Berrange 0 siblings, 0 replies; 17+ messages in thread From: Daniel P. Berrange @ 2016-03-01 13:54 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, Gerd Hoffmann, Dimitris Aragiorgis On Tue, Mar 01, 2016 at 01:03:56PM +0100, Paolo Bonzini wrote: > > > On 01/03/2016 12:58, Daniel P. Berrange wrote: > > > > means that QEMU will never close stderr anymore, so libvirt things > > > > QEMU is still starting up....forever. > > > > > > > > Given current libvirt behaviour / expectations, I think the only > > > > option is to revert this change. > > > > > > Why not fix it instead? :) > > > > Because if the user upgrades QEMU it'll cause all existing deployed > > version of libvirt to hang. I think that's pretty undesirable > > behaviour. > > Let me rephrase, why not fix it in QEMU? Jan Tomko helped me debugging > it, and I've sent a patch just before seeing your message. Oh, we were talking with crossed wires. Yes, fixing the problem rather than fully reverting the patch is of course a reasonable solution. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized 2016-03-01 11:15 ` Gerd Hoffmann 2016-03-01 11:47 ` Daniel P. Berrange @ 2016-03-01 11:50 ` Paolo Bonzini 1 sibling, 0 replies; 17+ messages in thread From: Paolo Bonzini @ 2016-03-01 11:50 UTC (permalink / raw) To: Gerd Hoffmann, Dimitris Aragiorgis; +Cc: qemu-devel On 01/03/2016 12:15, Gerd Hoffmann wrote: > On Do, 2016-02-18 at 13:38 +0200, Dimitris Aragiorgis wrote: >> In case of daemonize, use the logfile passed with the -D option in >> order to redirect stderr to it instead of /dev/null. >> >> Also remove some unused code in log.h. > > Patch breaks interaction with libvirt. libvirt hangs on startup, while > probing qemu capabilities. qemu runs in probing mode (command line is > "/home/kraxel/projects/qemu/build-default/x86_64-softmmu/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" according to "systemctl status libvirtd -l"), apparently both qemu and libvirt wait for each other. Patch sent, as a workaround use "./configure --enable-trace-backend=nop". Paolo ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-03-01 13:54 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-12-16 16:56 [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize Alex Pyrgiotis 2016-02-08 9:29 ` Paolo Bonzini 2016-02-11 12:12 ` Dimitris Aragiorgis 2016-02-11 12:31 ` Paolo Bonzini 2016-02-11 16:49 ` Dimitris Aragiorgis 2016-02-11 17:56 ` Paolo Bonzini 2016-02-18 11:38 ` [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized Dimitris Aragiorgis 2016-02-18 15:22 ` Paolo Bonzini 2016-02-18 17:12 ` Dimitris Aragiorgis 2016-02-19 17:25 ` Paolo Bonzini 2016-03-01 11:15 ` Gerd Hoffmann 2016-03-01 11:47 ` Daniel P. Berrange 2016-03-01 11:51 ` Paolo Bonzini 2016-03-01 11:58 ` Daniel P. Berrange 2016-03-01 12:03 ` Paolo Bonzini 2016-03-01 13:54 ` Daniel P. Berrange 2016-03-01 11:50 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).