[Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize

[Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize

[Alex Pyrgiotis]

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

Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize

[Paolo Bonzini]

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);
>      }

Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize

[Dimitris Aragiorgis]

[-- 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 --]

Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize

[Dimitris Aragiorgis]

[-- 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 --]

Re: [Qemu-devel] [PATCH] os-posix: Log to logfile in case of daemonize

[Paolo Bonzini]

-----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

[Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Dimitris Aragiorgis]

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

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Dimitris Aragiorgis]

[-- 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 --]

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Gerd Hoffmann]

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

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Daniel P. Berrange]

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 :|

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Daniel P. Berrange]

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 :|

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] log: Redirect stderr to logfile if deamonized

[Daniel P. Berrange]

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 :|