Re: [PATCH v2 27/39] util/log: Introduce qemu_set_log_filename_flags

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> Provide a function to set both filename and flags at
> the same time.  This is the common case at startup.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> v2: Return bool, per recommendations in qapi/error.h (phil).
> ---
>  include/qemu/log.h |   1 +
>  util/log.c         | 122 ++++++++++++++++++++++++++++-----------------
>  2 files changed, 77 insertions(+), 46 deletions(-)
>
> diff --git a/include/qemu/log.h b/include/qemu/log.h
> index 42d545f77a..b6c73376b5 100644
> --- a/include/qemu/log.h
> +++ b/include/qemu/log.h
> @@ -82,6 +82,7 @@ extern const QEMULogItem qemu_log_items[];
>  
>  bool qemu_set_log(int log_flags, Error **errp);
>  bool qemu_set_log_filename(const char *filename, Error **errp);
> +bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp);
>  void qemu_set_dfilter_ranges(const char *ranges, Error **errp);
>  bool qemu_log_in_addr_range(uint64_t addr);
>  int qemu_str_to_log_mask(const char *str);
> diff --git a/util/log.c b/util/log.c
> index 8b8b6a5d83..2152d5591e 100644
> --- a/util/log.c
> +++ b/util/log.c
> @@ -117,15 +117,58 @@ static void qemu_logfile_free(QemuLogFile *logfile)
>  }
>  
>  /* enable or disable low levels log */
> -bool qemu_set_log(int log_flags, Error **errp)
> +static bool qemu_set_log_internal(const char *filename, bool changed_name,
> +                                  int log_flags, Error **errp)
>  {
> -    bool need_to_open_file = false;
> +    bool need_to_open_file;
>      QemuLogFile *logfile;
>  
> -    qemu_loglevel = log_flags;
> +    QEMU_LOCK_GUARD(&qemu_logfile_mutex);
> +    logfile = qemu_logfile;
> +
> +    if (changed_name) {
> +        char *newname = NULL;
> +
> +        /*
> +         * Allow the user to include %d in their logfile which will be
> +         * substituted with the current PID. This is useful for debugging many
> +         * nested linux-user tasks but will result in lots of logs.
> +         *
> +         * filename may be NULL. In that case, log output is sent to stderr
> +         */
> +        if (filename) {
> +            char *pidstr = strstr(filename, "%");
> +
> +            if (pidstr) {
> +                /* We only accept one %d, no other format strings */
> +                if (pidstr[1] != 'd' || strchr(pidstr + 2, '%')) {
> +                    error_setg(errp, "Bad logfile format: %s", filename);
> +                    return false;
> +                }
> +                newname = g_strdup_printf(filename, getpid());
> +            } else {
> +                newname = g_strdup(filename);
> +            }
> +        }
> +
> +        g_free(logfilename);
> +        logfilename = newname;
> +        filename = newname;
> +
> +        if (logfile) {
> +            qatomic_rcu_set(&qemu_logfile, NULL);
> +            call_rcu(logfile, qemu_logfile_free, rcu);
> +            logfile = NULL;
> +        }
> +    } else {
> +        filename = logfilename;
> +    }
> +
>  #ifdef CONFIG_TRACE_LOG
> -    qemu_loglevel |= LOG_TRACE;
> +    log_flags |= LOG_TRACE;
>  #endif
> +    qemu_loglevel = log_flags;
> +

This looked weird - so should we consider a qatomic_set here to avoid an
inconsistent set of flags being read non-atomically elsewhere?

>      /*
>       * In all cases we only log if qemu_loglevel is set.
>       * Also:
> @@ -134,71 +177,58 @@ bool qemu_set_log(int log_flags, Error **errp)
>       *   If we are daemonized,
>       *     we will only log if there is a logfilename.
>       */
> -    if (qemu_loglevel && (!is_daemonized() || logfilename)) {
> -        need_to_open_file = true;
> -    }
> -    QEMU_LOCK_GUARD(&qemu_logfile_mutex);
> -    if (qemu_logfile && !need_to_open_file) {
> -        logfile = qemu_logfile;
> +    need_to_open_file = log_flags && (!is_daemonized() || filename);
> +
> +    if (logfile && !need_to_open_file) {
>          qatomic_rcu_set(&qemu_logfile, NULL);
>          call_rcu(logfile, qemu_logfile_free, rcu);
> -    } else if (!qemu_logfile && need_to_open_file) {
> -        logfile = g_new0(QemuLogFile, 1);
> -        if (logfilename) {
> -            logfile->fd = fopen(logfilename, log_append ? "a" : "w");
> -            if (!logfile->fd) {
> +        return true;
> +    }
> +    if (!logfile && need_to_open_file) {
> +        FILE *fd;
> +
> +        if (filename) {
> +            fd = fopen(filename, log_append ? "a" : "w");
> +            if (!fd) {
>                  error_setg_errno(errp, errno, "Error opening logfile %s",
> -                                 logfilename);
> +                                 filename);
>                  return false;
>              }
>              /* In case we are a daemon redirect stderr to logfile */
>              if (is_daemonized()) {
> -                dup2(fileno(logfile->fd), STDERR_FILENO);
> -                fclose(logfile->fd);
> +                dup2(fileno(fd), STDERR_FILENO);
> +                fclose(fd);
>                  /* This will skip closing logfile in qemu_log_close() */
> -                logfile->fd = stderr;
> +                fd = stderr;
>              }
>          } else {
>              /* Default to stderr if no log file specified */
>              assert(!is_daemonized());
> -            logfile->fd = stderr;
> +            fd = stderr;
>          }
>  
>          log_append = 1;
> +
> +        logfile = g_new0(QemuLogFile, 1);
> +        logfile->fd = fd;
>          qatomic_rcu_set(&qemu_logfile, logfile);

I was also pondering if flags should be part of the QemuLogFile
structure so it's consistent with each opened file. However I see it
gets repurposed just for clean-up later...

>      }
>      return true;
>  }
>  
> -/*
> - * Allow the user to include %d in their logfile which will be
> - * substituted with the current PID. This is useful for debugging many
> - * nested linux-user tasks but will result in lots of logs.
> - *
> - * filename may be NULL. In that case, log output is sent to stderr
> - */
> +bool qemu_set_log(int log_flags, Error **errp)
> +{
> +    return qemu_set_log_internal(NULL, false, log_flags, errp);
> +}
> +
>  bool qemu_set_log_filename(const char *filename, Error **errp)
>  {
> -    g_free(logfilename);
> -    logfilename = NULL;
> +    return qemu_set_log_internal(filename, true, qemu_loglevel, errp);
> +}
>  
> -    if (filename) {
> -            char *pidstr = strstr(filename, "%");
> -            if (pidstr) {
> -                /* We only accept one %d, no other format strings */
> -                if (pidstr[1] != 'd' || strchr(pidstr + 2, '%')) {
> -                    error_setg(errp, "Bad logfile format: %s", filename);
> -                    return false;
> -                } else {
> -                    logfilename = g_strdup_printf(filename, getpid());
> -                }
> -            } else {
> -                logfilename = g_strdup(filename);
> -            }
> -    }
> -
> -    qemu_log_close();
> -    return qemu_set_log(qemu_loglevel, errp);
> +bool qemu_set_log_filename_flags(const char *name, int flags, Error **errp)
> +{
> +    return qemu_set_log_internal(name, true, flags, errp);
>  }
>  
>  /* Returns true if addr is in our debug filter or no filter defined


-- 
Alex Bennée