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

[Paolo Bonzini <pbonzini@redhat.com>]

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