Re: [PATCH 5/9] error-report: introduce ErrorReportDetailedFunc

[Markus Armbruster <armbru@redhat.com>]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Remove monitor dependency from error printing code, by allowing programs
> to set a callback for when to use "detailed" reporting or not.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/error-report.h          | 4 +++-
>  bsd-user/main.c                      | 2 +-
>  linux-user/main.c                    | 2 +-
>  qemu-img.c                           | 2 +-
>  qemu-io.c                            | 2 +-
>  qemu-nbd.c                           | 2 +-
>  scsi/qemu-pr-helper.c                | 2 +-
>  softmmu/vl.c                         | 7 ++++++-
>  storage-daemon/qemu-storage-daemon.c | 7 ++++++-
>  util/error-report.c                  | 8 +++++---
>  10 files changed, 26 insertions(+), 12 deletions(-)
>
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 3ae2357fda54..e2e630f207f0 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -13,6 +13,8 @@
>  #ifndef QEMU_ERROR_REPORT_H
>  #define QEMU_ERROR_REPORT_H
>  
> +typedef bool (*ErrorReportDetailedFunc)(void);
> +
>  typedef struct Location {
>      /* all members are private to qemu-error.c */
>      enum { LOC_NONE, LOC_CMDLINE, LOC_FILE } kind;
> @@ -46,7 +48,7 @@ bool error_report_once_cond(bool *printed, const char *fmt, ...)
>  bool warn_report_once_cond(bool *printed, const char *fmt, ...)
>      G_GNUC_PRINTF(2, 3);
>  
> -void error_init(const char *argv0);
> +void error_init(const char *argv0, ErrorReportDetailedFunc detailed_fn);
>  
>  /*
>   * Similar to error_report(), except it prints the message just once.
> diff --git a/bsd-user/main.c b/bsd-user/main.c
> index 6f09180d6541..d5f8fca863d7 100644
> --- a/bsd-user/main.c
> +++ b/bsd-user/main.c
> @@ -292,7 +292,7 @@ int main(int argc, char **argv)
>  
>      save_proc_pathname(argv[0]);
>  
> -    error_init(argv[0]);
> +    error_init(argv[0], NULL);
>      module_call_init(MODULE_INIT_TRACE);
>      qemu_init_cpu_list();
>      module_call_init(MODULE_INIT_QOM);
[More such calls of error_init()...]
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 54e920ada1a1..3b46fc9c1fc5 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2590,6 +2590,11 @@ void qmp_x_exit_preconfig(Error **errp)
>      }
>  }
>  
> +static bool error_is_detailed(void)
> +{
> +    return !monitor_cur();
> +}
> +
>  void qemu_init(int argc, char **argv, char **envp)
>  {
>      QemuOpts *opts;
> @@ -2634,7 +2639,7 @@ void qemu_init(int argc, char **argv, char **envp)
>      qemu_add_opts(&qemu_action_opts);
>      module_call_init(MODULE_INIT_OPTS);
>  
> -    error_init(argv[0]);
> +    error_init(argv[0], error_is_detailed);
>      qemu_init_exec_dir(argv[0]);
>  
>      qemu_init_arch_modules();
> diff --git a/storage-daemon/qemu-storage-daemon.c b/storage-daemon/qemu-storage-daemon.c
> index c104817cdddc..7e4d5030a045 100644
> --- a/storage-daemon/qemu-storage-daemon.c
> +++ b/storage-daemon/qemu-storage-daemon.c
> @@ -368,13 +368,18 @@ static void pid_file_init(void)
>      atexit(pid_file_cleanup);
>  }
>  
> +static bool error_is_detailed(void)
> +{
> +    return !monitor_cur();
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  #ifdef CONFIG_POSIX
>      signal(SIGPIPE, SIG_IGN);
>  #endif
>  
> -    error_init(argv[0]);
> +    error_init(argv[0], error_is_detailed);
>      qemu_init_exec_dir(argv[0]);
>      os_setup_signal_handling();
>  
> diff --git a/util/error-report.c b/util/error-report.c
> index c43227a975e2..c2181f80a83d 100644
> --- a/util/error-report.c
> +++ b/util/error-report.c
> @@ -11,7 +11,6 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "monitor/monitor.h"
>  #include "qemu/error-report.h"
>  
>  /*
> @@ -28,6 +27,7 @@ typedef enum {
>  bool message_with_timestamp;
>  bool error_with_guestname;
>  const char *error_guest_name;
> +ErrorReportDetailedFunc detailed_fn = NULL;
>  
>  int error_printf(const char *fmt, ...)
>  {
> @@ -195,7 +195,7 @@ real_time_iso8601(void)
>   */
>  static void vreport(report_type type, const char *fmt, va_list ap)
>  {
> -    bool detailed = !monitor_cur();
> +    bool detailed = detailed_fn ? detailed_fn() : TRUE;
>      gchar *timestr;
>  
>      if (message_with_timestamp && detailed) {
> @@ -387,7 +387,7 @@ static void qemu_log_func(const gchar *log_domain,
>      }
>  }
>  
> -void error_init(const char *argv0)
> +void error_init(const char *argv0, ErrorReportDetailedFunc detailed)
>  {
>      const char *p = strrchr(argv0, '/');
>  
> @@ -401,4 +401,6 @@ void error_init(const char *argv0)
>      g_log_set_default_handler(qemu_log_func, NULL);
>      g_warn_if_fail(qemu_glog_domains == NULL);
>      qemu_glog_domains = g_strdup(g_getenv("G_MESSAGES_DEBUG"));
> +
> +    detailed_fn = detailed;
>  }

A callback works, but note that each program's function is fixed.  Why
not use the linker to resolve it?  Have a .o in libqemuutil.a that
defines error_is_detailed() to return false always.  Have another
error_is_detailed() that returns !monitor_cur() in monitor.c.  A program
that links the monitor gets the latter, all the others the former.

What do you think?