diff mbox series

[v2,2/6] util: Replace fprintf(stderr, "*\n" with error_report()

Message ID 20200227163101.414-3-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series misc: Improve error reporting on Windows | expand

Commit Message

Philippe Mathieu-Daudé Feb. 27, 2020, 4:30 p.m. UTC
From: Alistair Francis <alistair.francis@xilinx.com>

Replace a large number of the fprintf(stderr, "*\n" calls with
error_report(). The functions were renamed with these commands and then
compiler issues where manually fixed.

find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +
find ./* -type f -exec sed -i \
    'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
    {} +

The error in aio_poll() was removed manually.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <f71203227749e2afb8564b3388b2b34f6652b009.1510181732.git.alistair.francis@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
[PMD: Rebased]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Alistair Francis <Alistair.Francis@wdc.com>
Cc: Alistair Francis <alistair@alistair23.me>
---
 util/coroutine-sigaltstack.c |  3 ++-
 util/mmap-alloc.c            | 11 ++++++-----
 util/module.c                | 13 ++++++-------
 util/osdep.c                 |  4 ++--
 util/oslib-posix.c           |  3 ++-
 util/oslib-win32.c           |  3 ++-
 util/qemu-coroutine.c        | 10 +++++-----
 util/qemu-thread-posix.c     |  5 +++--
 util/qemu-thread-win32.c     |  5 +++--
 util/qemu-timer-common.c     |  3 ++-
 10 files changed, 33 insertions(+), 27 deletions(-)

Comments

Markus Armbruster Feb. 28, 2020, 7:43 a.m. UTC | #1
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> From: Alistair Francis <alistair.francis@xilinx.com>
>
> Replace a large number of the fprintf(stderr, "*\n" calls with
> error_report(). The functions were renamed with these commands and then
> compiler issues where manually fixed.
>
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
> find ./* -type f -exec sed -i \
>     'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>     {} +
>
> The error in aio_poll() was removed manually.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Message-Id: <f71203227749e2afb8564b3388b2b34f6652b009.1510181732.git.alistair.francis@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> [PMD: Rebased]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Alistair Francis <alistair@alistair23.me>
> ---
>  util/coroutine-sigaltstack.c |  3 ++-
>  util/mmap-alloc.c            | 11 ++++++-----
>  util/module.c                | 13 ++++++-------
>  util/osdep.c                 |  4 ++--
>  util/oslib-posix.c           |  3 ++-
>  util/oslib-win32.c           |  3 ++-
>  util/qemu-coroutine.c        | 10 +++++-----
>  util/qemu-thread-posix.c     |  5 +++--
>  util/qemu-thread-win32.c     |  5 +++--
>  util/qemu-timer-common.c     |  3 ++-
>  10 files changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
> index f6fc49a0e5..63decd4d1d 100644
> --- a/util/coroutine-sigaltstack.c
> +++ b/util/coroutine-sigaltstack.c
> @@ -29,6 +29,7 @@
>  #include <pthread.h>
>  #include "qemu-common.h"
>  #include "qemu/coroutine_int.h"
> +#include "qemu/error-report.h"
>  
>  typedef struct {
>      Coroutine base;
> @@ -80,7 +81,7 @@ static void __attribute__((constructor)) coroutine_init(void)
>  
>      ret = pthread_key_create(&thread_state_key, qemu_coroutine_thread_cleanup);
>      if (ret != 0) {
> -        fprintf(stderr, "unable to create leader key: %s\n", strerror(errno));
> +        error_report("unable to create leader key: %s", strerror(errno));
>          abort();
>      }
>  }
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 27dcccd8ec..3ac6e10404 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -18,6 +18,7 @@
>  #endif /* CONFIG_LINUX */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
>  #include "qemu/mmap-alloc.h"
>  #include "qemu/host-utils.h"
>  
> @@ -63,7 +64,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
>          } while (ret != 0 && errno == EINTR);
>  
>          if (ret != 0) {
> -            fprintf(stderr, "Couldn't statfs() memory path: %s\n",
> +            error_report("Couldn't statfs() memory path: %s",
>                      strerror(errno));

Indentation is off.

>              exit(1);
>          }
> @@ -160,10 +161,10 @@ void *qemu_ram_mmap(int fd,
>                  len = 0;
>              }
>              file_name[len] = '\0';
> -            fprintf(stderr, "Warning: requesting persistence across crashes "
> -                    "for backend file %s failed. Proceeding without "
> -                    "persistence, data might become corrupted in case of host "
> -                    "crash.\n", file_name);
> +            error_report("Warning: requesting persistence across crashes "
> +                         "for backend file %s failed. Proceeding without "
> +                         "persistence, data might become corrupted in case "
> +                         "of host crash.", file_name);

This should be something like

               warn_report("requesting persistence across crashes"
                           " for backend file %s failed",
                           file_name);
               error_printf("Proceeding without persistence, data might"
                            " become corrupted in case of host crash.\n");

Precedence: commit db0754df88 "file-posix: Use error API properly".

>              g_free(proc_link);
>              g_free(file_name);
>          }
> diff --git a/util/module.c b/util/module.c
> index 236a7bb52a..28efa1f891 100644
> --- a/util/module.c
> +++ b/util/module.c
> @@ -19,6 +19,7 @@
>  #endif
>  #include "qemu/queue.h"
>  #include "qemu/module.h"
> +#include "qemu/error-report.h"
>  
>  typedef struct ModuleEntry
>  {
> @@ -130,19 +131,17 @@ static int module_load_file(const char *fname)
>  
>      g_module = g_module_open(fname, G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL);
>      if (!g_module) {
> -        fprintf(stderr, "Failed to open module: %s\n",
> -                g_module_error());
> +        error_report("Failed to open module: %s", g_module_error());
>          ret = -EINVAL;
>          goto out;
>      }
>      if (!g_module_symbol(g_module, DSO_STAMP_FUN_STR, (gpointer *)&sym)) {
> -        fprintf(stderr, "Failed to initialize module: %s\n",
> -                fname);
> +        error_report("Failed to initialize module: %s", fname);
>          /* Print some info if this is a QEMU module (but from different build),
>           * this will make debugging user problems easier. */
>          if (g_module_symbol(g_module, "qemu_module_dummy", (gpointer *)&sym)) {
> -            fprintf(stderr,
> -                    "Note: only modules from the same build can be loaded.\n");
> +            error_report("Note: "
> +                         "only modules from the same build can be loaded.");

Use error_printf() to print the additional note.

>          }
>          g_module_close(g_module);
>          ret = -EINVAL;
> @@ -178,7 +177,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
>      static GHashTable *loaded_modules;
>  
>      if (!g_module_supported()) {
> -        fprintf(stderr, "Module is not supported by system.\n");
> +        error_report("Module is not supported by system.");
>          return false;
>      }
>  
> diff --git a/util/osdep.c b/util/osdep.c
> index f7d06050f7..ef40ae512a 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -484,7 +484,7 @@ void fips_set_state(bool requested)
>  #endif /* __linux__ */
>  
>  #ifdef _FIPS_DEBUG
> -    fprintf(stderr, "FIPS mode %s (requested %s)\n",
> +    error_report("FIPS mode %s (requested %s)",
>              (fips_enabled ? "enabled" : "disabled"),
>              (requested ? "enabled" : "disabled"));
>  #endif
> @@ -511,7 +511,7 @@ int socket_init(void)
>      ret = WSAStartup(MAKEWORD(2, 2), &Data);
>      if (ret != 0) {
>          err = WSAGetLastError();
> -        fprintf(stderr, "WSAStartup: %d\n", err);
> +        error_report("WSAStartup: %d", err);
>          return -1;
>      }
>      atexit(socket_cleanup);
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 897e8f3ba6..4977594a43 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -35,6 +35,7 @@
>  #include "sysemu/sysemu.h"
>  #include "trace.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include "qemu/sockets.h"
>  #include "qemu/thread.h"
>  #include <libgen.h>
> @@ -170,7 +171,7 @@ fail_close:
>  void *qemu_oom_check(void *ptr)
>  {
>      if (ptr == NULL) {
> -        fprintf(stderr, "Failed to allocate memory: %s\n", strerror(errno));
> +        error_report("Failed to allocate memory: %s", strerror(errno));
>          abort();
>      }
>      return ptr;
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index e9b14ab178..84b937865a 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -39,6 +39,7 @@
>  #include "trace.h"
>  #include "qemu/sockets.h"
>  #include "qemu/cutils.h"
> +#include "qemu/error-report.h"
>  
>  /* this must come after including "trace.h" */
>  #include <shlobj.h>
> @@ -46,7 +47,7 @@
>  void *qemu_oom_check(void *ptr)
>  {
>      if (ptr == NULL) {
> -        fprintf(stderr, "Failed to allocate memory: %lu\n", GetLastError());
> +        error_report("Failed to allocate memory: %lu", GetLastError());
>          abort();
>      }
>      return ptr;
> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
> index c3caa6c770..62d1dd09df 100644
> --- a/util/qemu-coroutine.c
> +++ b/util/qemu-coroutine.c
> @@ -14,6 +14,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "trace.h"
> +#include "qemu/error-report.h"
>  #include "qemu/thread.h"
>  #include "qemu/atomic.h"
>  #include "qemu/coroutine.h"
> @@ -125,14 +126,13 @@ void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)
>           * cause us to enter it twice, potentially even after the coroutine has
>           * been deleted */
>          if (scheduled) {
> -            fprintf(stderr,
> -                    "%s: Co-routine was already scheduled in '%s'\n",
> -                    __func__, scheduled);
> +            error_report("%s: Co-routine was already scheduled in '%s'",
> +                         __func__, scheduled);
>              abort();
>          }
>  
>          if (to->caller) {
> -            fprintf(stderr, "Co-routine re-entered recursively\n");
> +            error_report("Co-routine re-entered recursively");
>              abort();
>          }
>  
> @@ -185,7 +185,7 @@ void coroutine_fn qemu_coroutine_yield(void)
>      trace_qemu_coroutine_yield(self, to);
>  
>      if (!to) {
> -        fprintf(stderr, "Co-routine is yielding to no one\n");
> +        error_report("Co-routine is yielding to no one");
>          abort();
>      }
>  
> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> index 838980aaa5..b4d8de376c 100644
> --- a/util/qemu-thread-posix.c
> +++ b/util/qemu-thread-posix.c
> @@ -14,6 +14,7 @@
>  #include "qemu/thread.h"
>  #include "qemu/atomic.h"
>  #include "qemu/notify.h"
> +#include "qemu/error-report.h"
>  #include "qemu-thread-common.h"
>  
>  static bool name_threads;
> @@ -25,14 +26,14 @@ void qemu_thread_naming(bool enable)
>  #ifndef CONFIG_THREAD_SETNAME_BYTHREAD
>      /* This is a debugging option, not fatal */
>      if (enable) {
> -        fprintf(stderr, "qemu: thread naming not supported on this host\n");
> +        error_report("qemu: thread naming not supported on this host");

This isn't an error.  It's in response to -name debug-threads=on, and
tells the user debug-threads=on is being ignored.  Let's use
warn_report().

Drop the "qemu: ", please; error_report() & friends take care of that.
More of the same below.

>      }
>  #endif
>  }
>  
>  static void error_exit(int err, const char *msg)
>  {
> -    fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
> +    error_report("qemu: %s: %s", msg, strerror(err));
>      abort();
>  }
>  
> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> index 56a83333da..9bed338d7e 100644
> --- a/util/qemu-thread-win32.c
> +++ b/util/qemu-thread-win32.c
> @@ -15,6 +15,7 @@
>  #include "qemu-common.h"
>  #include "qemu/thread.h"
>  #include "qemu/notify.h"
> +#include "qemu/error-report.h"
>  #include "qemu-thread-common.h"
>  #include <process.h>
>  
> @@ -25,7 +26,7 @@ void qemu_thread_naming(bool enable)
>      /* But note we don't actually name them on Windows yet */
>      name_threads = enable;
>  
> -    fprintf(stderr, "qemu: thread naming not supported on this host\n");
> +    error_report("qemu: thread naming not supported on this host");

Likewise.

>  }
>  
>  static void error_exit(int err, const char *msg)
> @@ -34,7 +35,7 @@ static void error_exit(int err, const char *msg)
>  
>      FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER,
>                    NULL, err, 0, (LPTSTR)&pstr, 2, NULL);
> -    fprintf(stderr, "qemu: %s: %s\n", msg, pstr);
> +    error_report("qemu: %s: %s", msg, pstr);
>      LocalFree(pstr);
>      abort();
>  }
> diff --git a/util/qemu-timer-common.c b/util/qemu-timer-common.c
> index baf3317f74..527944da1c 100644
> --- a/util/qemu-timer-common.c
> +++ b/util/qemu-timer-common.c
> @@ -23,6 +23,7 @@
>   */
>  #include "qemu/osdep.h"
>  #include "qemu/timer.h"
> +#include "qemu/error-report.h"
>  
>  /***********************************************************/
>  /* real time host monotonic timer */
> @@ -37,7 +38,7 @@ static void __attribute__((constructor)) init_get_clock(void)
>      int ret;
>      ret = QueryPerformanceFrequency(&freq);
>      if (ret == 0) {
> -        fprintf(stderr, "Could not calibrate ticks\n");
> +        error_report("Could not calibrate ticks");
>          exit(1);
>      }
>      clock_freq = freq.QuadPart;
Philippe Mathieu-Daudé Feb. 28, 2020, 9:50 a.m. UTC | #2
On 2/28/20 8:43 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> From: Alistair Francis <alistair.francis@xilinx.com>
>>
>> Replace a large number of the fprintf(stderr, "*\n" calls with
>> error_report(). The functions were renamed with these commands and then
>> compiler issues where manually fixed.
>>
>> find ./* -type f -exec sed -i \
>>      'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N;N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>> find ./* -type f -exec sed -i \
>>      'N; {s|fprintf(stderr, "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>      {} +
>>
>> The error in aio_poll() was removed manually.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Message-Id: <f71203227749e2afb8564b3388b2b34f6652b009.1510181732.git.alistair.francis@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> [PMD: Rebased]
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Alistair Francis <Alistair.Francis@wdc.com>
>> Cc: Alistair Francis <alistair@alistair23.me>
>> ---
>>   util/coroutine-sigaltstack.c |  3 ++-
>>   util/mmap-alloc.c            | 11 ++++++-----
>>   util/module.c                | 13 ++++++-------
>>   util/osdep.c                 |  4 ++--
>>   util/oslib-posix.c           |  3 ++-
>>   util/oslib-win32.c           |  3 ++-
>>   util/qemu-coroutine.c        | 10 +++++-----
>>   util/qemu-thread-posix.c     |  5 +++--
>>   util/qemu-thread-win32.c     |  5 +++--
>>   util/qemu-timer-common.c     |  3 ++-
>>   10 files changed, 33 insertions(+), 27 deletions(-)
>>
>> diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
>> index f6fc49a0e5..63decd4d1d 100644
>> --- a/util/coroutine-sigaltstack.c
>> +++ b/util/coroutine-sigaltstack.c
>> @@ -29,6 +29,7 @@
>>   #include <pthread.h>
>>   #include "qemu-common.h"
>>   #include "qemu/coroutine_int.h"
>> +#include "qemu/error-report.h"
>>   
>>   typedef struct {
>>       Coroutine base;
>> @@ -80,7 +81,7 @@ static void __attribute__((constructor)) coroutine_init(void)
>>   
>>       ret = pthread_key_create(&thread_state_key, qemu_coroutine_thread_cleanup);
>>       if (ret != 0) {
>> -        fprintf(stderr, "unable to create leader key: %s\n", strerror(errno));
>> +        error_report("unable to create leader key: %s", strerror(errno));
>>           abort();
>>       }
>>   }
>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
>> index 27dcccd8ec..3ac6e10404 100644
>> --- a/util/mmap-alloc.c
>> +++ b/util/mmap-alloc.c
>> @@ -18,6 +18,7 @@
>>   #endif /* CONFIG_LINUX */
>>   
>>   #include "qemu/osdep.h"
>> +#include "qemu/error-report.h"
>>   #include "qemu/mmap-alloc.h"
>>   #include "qemu/host-utils.h"
>>   
>> @@ -63,7 +64,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
>>           } while (ret != 0 && errno == EINTR);
>>   
>>           if (ret != 0) {
>> -            fprintf(stderr, "Couldn't statfs() memory path: %s\n",
>> +            error_report("Couldn't statfs() memory path: %s",
>>                       strerror(errno));
> 
> Indentation is off.
> 
>>               exit(1);
>>           }
>> @@ -160,10 +161,10 @@ void *qemu_ram_mmap(int fd,
>>                   len = 0;
>>               }
>>               file_name[len] = '\0';
>> -            fprintf(stderr, "Warning: requesting persistence across crashes "
>> -                    "for backend file %s failed. Proceeding without "
>> -                    "persistence, data might become corrupted in case of host "
>> -                    "crash.\n", file_name);
>> +            error_report("Warning: requesting persistence across crashes "
>> +                         "for backend file %s failed. Proceeding without "
>> +                         "persistence, data might become corrupted in case "
>> +                         "of host crash.", file_name);
> 
> This should be something like
> 
>                 warn_report("requesting persistence across crashes"
>                             " for backend file %s failed",
>                             file_name);
>                 error_printf("Proceeding without persistence, data might"
>                              " become corrupted in case of host crash.\n");
> 
> Precedence: commit db0754df88 "file-posix: Use error API properly".
> 
>>               g_free(proc_link);
>>               g_free(file_name);
>>           }
>> diff --git a/util/module.c b/util/module.c
>> index 236a7bb52a..28efa1f891 100644
>> --- a/util/module.c
>> +++ b/util/module.c
>> @@ -19,6 +19,7 @@
>>   #endif
>>   #include "qemu/queue.h"
>>   #include "qemu/module.h"
>> +#include "qemu/error-report.h"
>>   
>>   typedef struct ModuleEntry
>>   {
>> @@ -130,19 +131,17 @@ static int module_load_file(const char *fname)
>>   
>>       g_module = g_module_open(fname, G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL);
>>       if (!g_module) {
>> -        fprintf(stderr, "Failed to open module: %s\n",
>> -                g_module_error());
>> +        error_report("Failed to open module: %s", g_module_error());
>>           ret = -EINVAL;
>>           goto out;
>>       }
>>       if (!g_module_symbol(g_module, DSO_STAMP_FUN_STR, (gpointer *)&sym)) {
>> -        fprintf(stderr, "Failed to initialize module: %s\n",
>> -                fname);
>> +        error_report("Failed to initialize module: %s", fname);
>>           /* Print some info if this is a QEMU module (but from different build),
>>            * this will make debugging user problems easier. */
>>           if (g_module_symbol(g_module, "qemu_module_dummy", (gpointer *)&sym)) {
>> -            fprintf(stderr,
>> -                    "Note: only modules from the same build can be loaded.\n");
>> +            error_report("Note: "
>> +                         "only modules from the same build can be loaded.");
> 
> Use error_printf() to print the additional note.
> 
>>           }
>>           g_module_close(g_module);
>>           ret = -EINVAL;
>> @@ -178,7 +177,7 @@ bool module_load_one(const char *prefix, const char *lib_name)
>>       static GHashTable *loaded_modules;
>>   
>>       if (!g_module_supported()) {
>> -        fprintf(stderr, "Module is not supported by system.\n");
>> +        error_report("Module is not supported by system.");
>>           return false;
>>       }
>>   
>> diff --git a/util/osdep.c b/util/osdep.c
>> index f7d06050f7..ef40ae512a 100644
>> --- a/util/osdep.c
>> +++ b/util/osdep.c
>> @@ -484,7 +484,7 @@ void fips_set_state(bool requested)
>>   #endif /* __linux__ */
>>   
>>   #ifdef _FIPS_DEBUG
>> -    fprintf(stderr, "FIPS mode %s (requested %s)\n",
>> +    error_report("FIPS mode %s (requested %s)",
>>               (fips_enabled ? "enabled" : "disabled"),
>>               (requested ? "enabled" : "disabled"));
>>   #endif
>> @@ -511,7 +511,7 @@ int socket_init(void)
>>       ret = WSAStartup(MAKEWORD(2, 2), &Data);
>>       if (ret != 0) {
>>           err = WSAGetLastError();
>> -        fprintf(stderr, "WSAStartup: %d\n", err);
>> +        error_report("WSAStartup: %d", err);
>>           return -1;
>>       }
>>       atexit(socket_cleanup);
>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>> index 897e8f3ba6..4977594a43 100644
>> --- a/util/oslib-posix.c
>> +++ b/util/oslib-posix.c
>> @@ -35,6 +35,7 @@
>>   #include "sysemu/sysemu.h"
>>   #include "trace.h"
>>   #include "qapi/error.h"
>> +#include "qemu/error-report.h"
>>   #include "qemu/sockets.h"
>>   #include "qemu/thread.h"
>>   #include <libgen.h>
>> @@ -170,7 +171,7 @@ fail_close:
>>   void *qemu_oom_check(void *ptr)
>>   {
>>       if (ptr == NULL) {
>> -        fprintf(stderr, "Failed to allocate memory: %s\n", strerror(errno));
>> +        error_report("Failed to allocate memory: %s", strerror(errno));
>>           abort();
>>       }
>>       return ptr;
>> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>> index e9b14ab178..84b937865a 100644
>> --- a/util/oslib-win32.c
>> +++ b/util/oslib-win32.c
>> @@ -39,6 +39,7 @@
>>   #include "trace.h"
>>   #include "qemu/sockets.h"
>>   #include "qemu/cutils.h"
>> +#include "qemu/error-report.h"
>>   
>>   /* this must come after including "trace.h" */
>>   #include <shlobj.h>
>> @@ -46,7 +47,7 @@
>>   void *qemu_oom_check(void *ptr)
>>   {
>>       if (ptr == NULL) {
>> -        fprintf(stderr, "Failed to allocate memory: %lu\n", GetLastError());
>> +        error_report("Failed to allocate memory: %lu", GetLastError());
>>           abort();
>>       }
>>       return ptr;
>> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
>> index c3caa6c770..62d1dd09df 100644
>> --- a/util/qemu-coroutine.c
>> +++ b/util/qemu-coroutine.c
>> @@ -14,6 +14,7 @@
>>   
>>   #include "qemu/osdep.h"
>>   #include "trace.h"
>> +#include "qemu/error-report.h"
>>   #include "qemu/thread.h"
>>   #include "qemu/atomic.h"
>>   #include "qemu/coroutine.h"
>> @@ -125,14 +126,13 @@ void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)
>>            * cause us to enter it twice, potentially even after the coroutine has
>>            * been deleted */
>>           if (scheduled) {
>> -            fprintf(stderr,
>> -                    "%s: Co-routine was already scheduled in '%s'\n",
>> -                    __func__, scheduled);
>> +            error_report("%s: Co-routine was already scheduled in '%s'",
>> +                         __func__, scheduled);
>>               abort();
>>           }
>>   
>>           if (to->caller) {
>> -            fprintf(stderr, "Co-routine re-entered recursively\n");
>> +            error_report("Co-routine re-entered recursively");
>>               abort();
>>           }
>>   
>> @@ -185,7 +185,7 @@ void coroutine_fn qemu_coroutine_yield(void)
>>       trace_qemu_coroutine_yield(self, to);
>>   
>>       if (!to) {
>> -        fprintf(stderr, "Co-routine is yielding to no one\n");
>> +        error_report("Co-routine is yielding to no one");
>>           abort();
>>       }
>>   
>> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
>> index 838980aaa5..b4d8de376c 100644
>> --- a/util/qemu-thread-posix.c
>> +++ b/util/qemu-thread-posix.c
>> @@ -14,6 +14,7 @@
>>   #include "qemu/thread.h"
>>   #include "qemu/atomic.h"
>>   #include "qemu/notify.h"
>> +#include "qemu/error-report.h"
>>   #include "qemu-thread-common.h"
>>   
>>   static bool name_threads;
>> @@ -25,14 +26,14 @@ void qemu_thread_naming(bool enable)
>>   #ifndef CONFIG_THREAD_SETNAME_BYTHREAD
>>       /* This is a debugging option, not fatal */
>>       if (enable) {
>> -        fprintf(stderr, "qemu: thread naming not supported on this host\n");
>> +        error_report("qemu: thread naming not supported on this host");
> 
> This isn't an error.  It's in response to -name debug-threads=on, and
> tells the user debug-threads=on is being ignored.  Let's use
> warn_report().
> 
> Drop the "qemu: ", please; error_report() & friends take care of that.
> More of the same below.
> 
>>       }
>>   #endif
>>   }
>>   
>>   static void error_exit(int err, const char *msg)
>>   {
>> -    fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
>> +    error_report("qemu: %s: %s", msg, strerror(err));
>>       abort();
>>   }
>>   
>> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
>> index 56a83333da..9bed338d7e 100644
>> --- a/util/qemu-thread-win32.c
>> +++ b/util/qemu-thread-win32.c
>> @@ -15,6 +15,7 @@
>>   #include "qemu-common.h"
>>   #include "qemu/thread.h"
>>   #include "qemu/notify.h"
>> +#include "qemu/error-report.h"
>>   #include "qemu-thread-common.h"
>>   #include <process.h>
>>   
>> @@ -25,7 +26,7 @@ void qemu_thread_naming(bool enable)
>>       /* But note we don't actually name them on Windows yet */
>>       name_threads = enable;
>>   
>> -    fprintf(stderr, "qemu: thread naming not supported on this host\n");
>> +    error_report("qemu: thread naming not supported on this host");
> 
> Likewise.

Thanks for your review. I'll drop the changes in util/oslib-win32.c for 
for now, and add a note in my TODO for after the 5.0 release.

> 
>>   }
>>   
>>   static void error_exit(int err, const char *msg)
>> @@ -34,7 +35,7 @@ static void error_exit(int err, const char *msg)
>>   
>>       FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER,
>>                     NULL, err, 0, (LPTSTR)&pstr, 2, NULL);
>> -    fprintf(stderr, "qemu: %s: %s\n", msg, pstr);
>> +    error_report("qemu: %s: %s", msg, pstr);
>>       LocalFree(pstr);
>>       abort();
>>   }
>> diff --git a/util/qemu-timer-common.c b/util/qemu-timer-common.c
>> index baf3317f74..527944da1c 100644
>> --- a/util/qemu-timer-common.c
>> +++ b/util/qemu-timer-common.c
>> @@ -23,6 +23,7 @@
>>    */
>>   #include "qemu/osdep.h"
>>   #include "qemu/timer.h"
>> +#include "qemu/error-report.h"
>>   
>>   /***********************************************************/
>>   /* real time host monotonic timer */
>> @@ -37,7 +38,7 @@ static void __attribute__((constructor)) init_get_clock(void)
>>       int ret;
>>       ret = QueryPerformanceFrequency(&freq);
>>       if (ret == 0) {
>> -        fprintf(stderr, "Could not calibrate ticks\n");
>> +        error_report("Could not calibrate ticks");
>>           exit(1);
>>       }
>>       clock_freq = freq.QuadPart;
>
Philippe Mathieu-Daudé Feb. 28, 2020, 9:57 a.m. UTC | #3
On 2/28/20 10:50 AM, Philippe Mathieu-Daudé wrote:
> On 2/28/20 8:43 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> From: Alistair Francis <alistair.francis@xilinx.com>
>>>
>>> Replace a large number of the fprintf(stderr, "*\n" calls with
>>> error_report(). The functions were renamed with these commands and then
>>> compiler issues where manually fixed.
>>>
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N;N;N;N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N;N;N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N;N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N;N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>> find ./* -type f -exec sed -i \
>>>      'N; {s|fprintf(stderr, 
>>> "\(.*\)\\n"\(.*\));|error_report("\1"\2);|Ig}' \
>>>      {} +
>>>
>>> The error in aio_poll() was removed manually.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Message-Id: 
>>> <f71203227749e2afb8564b3388b2b34f6652b009.1510181732.git.alistair.francis@xilinx.com> 
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> [PMD: Rebased]
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> Cc: Alistair Francis <Alistair.Francis@wdc.com>
>>> Cc: Alistair Francis <alistair@alistair23.me>
>>> ---
>>>   util/coroutine-sigaltstack.c |  3 ++-
>>>   util/mmap-alloc.c            | 11 ++++++-----
>>>   util/module.c                | 13 ++++++-------
>>>   util/osdep.c                 |  4 ++--
>>>   util/oslib-posix.c           |  3 ++-
>>>   util/oslib-win32.c           |  3 ++-
>>>   util/qemu-coroutine.c        | 10 +++++-----
>>>   util/qemu-thread-posix.c     |  5 +++--
>>>   util/qemu-thread-win32.c     |  5 +++--
>>>   util/qemu-timer-common.c     |  3 ++-
>>>   10 files changed, 33 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
>>> index f6fc49a0e5..63decd4d1d 100644
>>> --- a/util/coroutine-sigaltstack.c
>>> +++ b/util/coroutine-sigaltstack.c
>>> @@ -29,6 +29,7 @@
>>>   #include <pthread.h>
>>>   #include "qemu-common.h"
>>>   #include "qemu/coroutine_int.h"
>>> +#include "qemu/error-report.h"
>>>   typedef struct {
>>>       Coroutine base;
>>> @@ -80,7 +81,7 @@ static void __attribute__((constructor)) 
>>> coroutine_init(void)
>>>       ret = pthread_key_create(&thread_state_key, 
>>> qemu_coroutine_thread_cleanup);
>>>       if (ret != 0) {
>>> -        fprintf(stderr, "unable to create leader key: %s\n", 
>>> strerror(errno));
>>> +        error_report("unable to create leader key: %s", 
>>> strerror(errno));
>>>           abort();
>>>       }
>>>   }
>>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
>>> index 27dcccd8ec..3ac6e10404 100644
>>> --- a/util/mmap-alloc.c
>>> +++ b/util/mmap-alloc.c
>>> @@ -18,6 +18,7 @@
>>>   #endif /* CONFIG_LINUX */
>>>   #include "qemu/osdep.h"
>>> +#include "qemu/error-report.h"
>>>   #include "qemu/mmap-alloc.h"
>>>   #include "qemu/host-utils.h"
>>> @@ -63,7 +64,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
>>>           } while (ret != 0 && errno == EINTR);
>>>           if (ret != 0) {
>>> -            fprintf(stderr, "Couldn't statfs() memory path: %s\n",
>>> +            error_report("Couldn't statfs() memory path: %s",
>>>                       strerror(errno));
>>
>> Indentation is off.
>>
>>>               exit(1);
>>>           }
>>> @@ -160,10 +161,10 @@ void *qemu_ram_mmap(int fd,
>>>                   len = 0;
>>>               }
>>>               file_name[len] = '\0';
>>> -            fprintf(stderr, "Warning: requesting persistence across 
>>> crashes "
>>> -                    "for backend file %s failed. Proceeding without "
>>> -                    "persistence, data might become corrupted in 
>>> case of host "
>>> -                    "crash.\n", file_name);
>>> +            error_report("Warning: requesting persistence across 
>>> crashes "
>>> +                         "for backend file %s failed. Proceeding 
>>> without "
>>> +                         "persistence, data might become corrupted 
>>> in case "
>>> +                         "of host crash.", file_name);
>>
>> This should be something like
>>
>>                 warn_report("requesting persistence across crashes"
>>                             " for backend file %s failed",
>>                             file_name);
>>                 error_printf("Proceeding without persistence, data might"
>>                              " become corrupted in case of host 
>> crash.\n");
>>
>> Precedence: commit db0754df88 "file-posix: Use error API properly".
>>
>>>               g_free(proc_link);
>>>               g_free(file_name);
>>>           }
>>> diff --git a/util/module.c b/util/module.c
>>> index 236a7bb52a..28efa1f891 100644
>>> --- a/util/module.c
>>> +++ b/util/module.c
>>> @@ -19,6 +19,7 @@
>>>   #endif
>>>   #include "qemu/queue.h"
>>>   #include "qemu/module.h"
>>> +#include "qemu/error-report.h"
>>>   typedef struct ModuleEntry
>>>   {
>>> @@ -130,19 +131,17 @@ static int module_load_file(const char *fname)
>>>       g_module = g_module_open(fname, G_MODULE_BIND_LAZY | 
>>> G_MODULE_BIND_LOCAL);
>>>       if (!g_module) {
>>> -        fprintf(stderr, "Failed to open module: %s\n",
>>> -                g_module_error());
>>> +        error_report("Failed to open module: %s", g_module_error());
>>>           ret = -EINVAL;
>>>           goto out;
>>>       }
>>>       if (!g_module_symbol(g_module, DSO_STAMP_FUN_STR, (gpointer 
>>> *)&sym)) {
>>> -        fprintf(stderr, "Failed to initialize module: %s\n",
>>> -                fname);
>>> +        error_report("Failed to initialize module: %s", fname);
>>>           /* Print some info if this is a QEMU module (but from 
>>> different build),
>>>            * this will make debugging user problems easier. */
>>>           if (g_module_symbol(g_module, "qemu_module_dummy", 
>>> (gpointer *)&sym)) {
>>> -            fprintf(stderr,
>>> -                    "Note: only modules from the same build can be 
>>> loaded.\n");
>>> +            error_report("Note: "
>>> +                         "only modules from the same build can be 
>>> loaded.");
>>
>> Use error_printf() to print the additional note.
>>
>>>           }
>>>           g_module_close(g_module);
>>>           ret = -EINVAL;
>>> @@ -178,7 +177,7 @@ bool module_load_one(const char *prefix, const 
>>> char *lib_name)
>>>       static GHashTable *loaded_modules;
>>>       if (!g_module_supported()) {
>>> -        fprintf(stderr, "Module is not supported by system.\n");
>>> +        error_report("Module is not supported by system.");
>>>           return false;
>>>       }
>>> diff --git a/util/osdep.c b/util/osdep.c
>>> index f7d06050f7..ef40ae512a 100644
>>> --- a/util/osdep.c
>>> +++ b/util/osdep.c
>>> @@ -484,7 +484,7 @@ void fips_set_state(bool requested)
>>>   #endif /* __linux__ */
>>>   #ifdef _FIPS_DEBUG
>>> -    fprintf(stderr, "FIPS mode %s (requested %s)\n",
>>> +    error_report("FIPS mode %s (requested %s)",
>>>               (fips_enabled ? "enabled" : "disabled"),
>>>               (requested ? "enabled" : "disabled"));
>>>   #endif
>>> @@ -511,7 +511,7 @@ int socket_init(void)
>>>       ret = WSAStartup(MAKEWORD(2, 2), &Data);
>>>       if (ret != 0) {
>>>           err = WSAGetLastError();
>>> -        fprintf(stderr, "WSAStartup: %d\n", err);
>>> +        error_report("WSAStartup: %d", err);
>>>           return -1;
>>>       }
>>>       atexit(socket_cleanup);
>>> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>>> index 897e8f3ba6..4977594a43 100644
>>> --- a/util/oslib-posix.c
>>> +++ b/util/oslib-posix.c
>>> @@ -35,6 +35,7 @@
>>>   #include "sysemu/sysemu.h"
>>>   #include "trace.h"
>>>   #include "qapi/error.h"
>>> +#include "qemu/error-report.h"
>>>   #include "qemu/sockets.h"
>>>   #include "qemu/thread.h"
>>>   #include <libgen.h>
>>> @@ -170,7 +171,7 @@ fail_close:
>>>   void *qemu_oom_check(void *ptr)
>>>   {
>>>       if (ptr == NULL) {
>>> -        fprintf(stderr, "Failed to allocate memory: %s\n", 
>>> strerror(errno));
>>> +        error_report("Failed to allocate memory: %s", strerror(errno));
>>>           abort();
>>>       }
>>>       return ptr;
>>> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>>> index e9b14ab178..84b937865a 100644
>>> --- a/util/oslib-win32.c
>>> +++ b/util/oslib-win32.c
>>> @@ -39,6 +39,7 @@
>>>   #include "trace.h"
>>>   #include "qemu/sockets.h"
>>>   #include "qemu/cutils.h"
>>> +#include "qemu/error-report.h"
>>>   /* this must come after including "trace.h" */
>>>   #include <shlobj.h>
>>> @@ -46,7 +47,7 @@
>>>   void *qemu_oom_check(void *ptr)
>>>   {
>>>       if (ptr == NULL) {
>>> -        fprintf(stderr, "Failed to allocate memory: %lu\n", 
>>> GetLastError());
>>> +        error_report("Failed to allocate memory: %lu", GetLastError());
>>>           abort();
>>>       }
>>>       return ptr;
>>> diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
>>> index c3caa6c770..62d1dd09df 100644
>>> --- a/util/qemu-coroutine.c
>>> +++ b/util/qemu-coroutine.c
>>> @@ -14,6 +14,7 @@
>>>   #include "qemu/osdep.h"
>>>   #include "trace.h"
>>> +#include "qemu/error-report.h"
>>>   #include "qemu/thread.h"
>>>   #include "qemu/atomic.h"
>>>   #include "qemu/coroutine.h"
>>> @@ -125,14 +126,13 @@ void qemu_aio_coroutine_enter(AioContext *ctx, 
>>> Coroutine *co)
>>>            * cause us to enter it twice, potentially even after the 
>>> coroutine has
>>>            * been deleted */
>>>           if (scheduled) {
>>> -            fprintf(stderr,
>>> -                    "%s: Co-routine was already scheduled in '%s'\n",
>>> -                    __func__, scheduled);
>>> +            error_report("%s: Co-routine was already scheduled in 
>>> '%s'",
>>> +                         __func__, scheduled);
>>>               abort();
>>>           }
>>>           if (to->caller) {
>>> -            fprintf(stderr, "Co-routine re-entered recursively\n");
>>> +            error_report("Co-routine re-entered recursively");
>>>               abort();
>>>           }
>>> @@ -185,7 +185,7 @@ void coroutine_fn qemu_coroutine_yield(void)
>>>       trace_qemu_coroutine_yield(self, to);
>>>       if (!to) {
>>> -        fprintf(stderr, "Co-routine is yielding to no one\n");
>>> +        error_report("Co-routine is yielding to no one");
>>>           abort();
>>>       }
>>> diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
>>> index 838980aaa5..b4d8de376c 100644
>>> --- a/util/qemu-thread-posix.c
>>> +++ b/util/qemu-thread-posix.c
>>> @@ -14,6 +14,7 @@
>>>   #include "qemu/thread.h"
>>>   #include "qemu/atomic.h"
>>>   #include "qemu/notify.h"
>>> +#include "qemu/error-report.h"
>>>   #include "qemu-thread-common.h"
>>>   static bool name_threads;
>>> @@ -25,14 +26,14 @@ void qemu_thread_naming(bool enable)
>>>   #ifndef CONFIG_THREAD_SETNAME_BYTHREAD
>>>       /* This is a debugging option, not fatal */
>>>       if (enable) {
>>> -        fprintf(stderr, "qemu: thread naming not supported on this 
>>> host\n");
>>> +        error_report("qemu: thread naming not supported on this host");
>>
>> This isn't an error.  It's in response to -name debug-threads=on, and
>> tells the user debug-threads=on is being ignored.  Let's use
>> warn_report().
>>
>> Drop the "qemu: ", please; error_report() & friends take care of that.
>> More of the same below.
>>
>>>       }
>>>   #endif
>>>   }
>>>   static void error_exit(int err, const char *msg)
>>>   {
>>> -    fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
>>> +    error_report("qemu: %s: %s", msg, strerror(err));
>>>       abort();
>>>   }
>>> diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
>>> index 56a83333da..9bed338d7e 100644
>>> --- a/util/qemu-thread-win32.c
>>> +++ b/util/qemu-thread-win32.c
>>> @@ -15,6 +15,7 @@
>>>   #include "qemu-common.h"
>>>   #include "qemu/thread.h"
>>>   #include "qemu/notify.h"
>>> +#include "qemu/error-report.h"
>>>   #include "qemu-thread-common.h"
>>>   #include <process.h>
>>> @@ -25,7 +26,7 @@ void qemu_thread_naming(bool enable)
>>>       /* But note we don't actually name them on Windows yet */
>>>       name_threads = enable;
>>> -    fprintf(stderr, "qemu: thread naming not supported on this 
>>> host\n");
>>> +    error_report("qemu: thread naming not supported on this host");
>>
>> Likewise.
> 
> Thanks for your review. I'll drop the changes in util/oslib-win32.c for 
> for now, and add a note in my TODO for after the 5.0 release.

Well if I follow this line, I'v to drop the changes in util/osdep.c too.
Maybe we can keep fprintf() for now and improve the error message, and 
do the fprintf -> error_report cleanup later?

> 
>>
>>>   }
>>>   static void error_exit(int err, const char *msg)
>>> @@ -34,7 +35,7 @@ static void error_exit(int err, const char *msg)
>>>       FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM | 
>>> FORMAT_MESSAGE_ALLOCATE_BUFFER,
>>>                     NULL, err, 0, (LPTSTR)&pstr, 2, NULL);
>>> -    fprintf(stderr, "qemu: %s: %s\n", msg, pstr);
>>> +    error_report("qemu: %s: %s", msg, pstr);
>>>       LocalFree(pstr);
>>>       abort();
>>>   }
>>> diff --git a/util/qemu-timer-common.c b/util/qemu-timer-common.c
>>> index baf3317f74..527944da1c 100644
>>> --- a/util/qemu-timer-common.c
>>> +++ b/util/qemu-timer-common.c
>>> @@ -23,6 +23,7 @@
>>>    */
>>>   #include "qemu/osdep.h"
>>>   #include "qemu/timer.h"
>>> +#include "qemu/error-report.h"
>>>   /***********************************************************/
>>>   /* real time host monotonic timer */
>>> @@ -37,7 +38,7 @@ static void __attribute__((constructor)) 
>>> init_get_clock(void)
>>>       int ret;
>>>       ret = QueryPerformanceFrequency(&freq);
>>>       if (ret == 0) {
>>> -        fprintf(stderr, "Could not calibrate ticks\n");
>>> +        error_report("Could not calibrate ticks");
>>>           exit(1);
>>>       }
>>>       clock_freq = freq.QuadPart;
>>
Markus Armbruster Feb. 28, 2020, 5:41 p.m. UTC | #4
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 2/28/20 10:50 AM, Philippe Mathieu-Daudé wrote:
[...]
>> Thanks for your review. I'll drop the changes in util/oslib-win32.c
>> for for now, and add a note in my TODO for after the 5.0 release.
>
> Well if I follow this line, I'v to drop the changes in util/osdep.c too.
> Maybe we can keep fprintf() for now and improve the error message, and
> do the fprintf -> error_report cleanup later?

I recommend to convert from fprintf() to error_report() & friends and
improve the message all in one go.

Separating different kinds of changes makes sense when some kinds are
mechanical and the resulting mechanical patches are large.  These
patches aren't large.

But it's really up to you.  I'm not going to veto an improvement only
because further improvement is called for.
diff mbox series

Patch

diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index f6fc49a0e5..63decd4d1d 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -29,6 +29,7 @@ 
 #include <pthread.h>
 #include "qemu-common.h"
 #include "qemu/coroutine_int.h"
+#include "qemu/error-report.h"
 
 typedef struct {
     Coroutine base;
@@ -80,7 +81,7 @@  static void __attribute__((constructor)) coroutine_init(void)
 
     ret = pthread_key_create(&thread_state_key, qemu_coroutine_thread_cleanup);
     if (ret != 0) {
-        fprintf(stderr, "unable to create leader key: %s\n", strerror(errno));
+        error_report("unable to create leader key: %s", strerror(errno));
         abort();
     }
 }
diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 27dcccd8ec..3ac6e10404 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -18,6 +18,7 @@ 
 #endif /* CONFIG_LINUX */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 #include "qemu/mmap-alloc.h"
 #include "qemu/host-utils.h"
 
@@ -63,7 +64,7 @@  size_t qemu_mempath_getpagesize(const char *mem_path)
         } while (ret != 0 && errno == EINTR);
 
         if (ret != 0) {
-            fprintf(stderr, "Couldn't statfs() memory path: %s\n",
+            error_report("Couldn't statfs() memory path: %s",
                     strerror(errno));
             exit(1);
         }
@@ -160,10 +161,10 @@  void *qemu_ram_mmap(int fd,
                 len = 0;
             }
             file_name[len] = '\0';
-            fprintf(stderr, "Warning: requesting persistence across crashes "
-                    "for backend file %s failed. Proceeding without "
-                    "persistence, data might become corrupted in case of host "
-                    "crash.\n", file_name);
+            error_report("Warning: requesting persistence across crashes "
+                         "for backend file %s failed. Proceeding without "
+                         "persistence, data might become corrupted in case "
+                         "of host crash.", file_name);
             g_free(proc_link);
             g_free(file_name);
         }
diff --git a/util/module.c b/util/module.c
index 236a7bb52a..28efa1f891 100644
--- a/util/module.c
+++ b/util/module.c
@@ -19,6 +19,7 @@ 
 #endif
 #include "qemu/queue.h"
 #include "qemu/module.h"
+#include "qemu/error-report.h"
 
 typedef struct ModuleEntry
 {
@@ -130,19 +131,17 @@  static int module_load_file(const char *fname)
 
     g_module = g_module_open(fname, G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL);
     if (!g_module) {
-        fprintf(stderr, "Failed to open module: %s\n",
-                g_module_error());
+        error_report("Failed to open module: %s", g_module_error());
         ret = -EINVAL;
         goto out;
     }
     if (!g_module_symbol(g_module, DSO_STAMP_FUN_STR, (gpointer *)&sym)) {
-        fprintf(stderr, "Failed to initialize module: %s\n",
-                fname);
+        error_report("Failed to initialize module: %s", fname);
         /* Print some info if this is a QEMU module (but from different build),
          * this will make debugging user problems easier. */
         if (g_module_symbol(g_module, "qemu_module_dummy", (gpointer *)&sym)) {
-            fprintf(stderr,
-                    "Note: only modules from the same build can be loaded.\n");
+            error_report("Note: "
+                         "only modules from the same build can be loaded.");
         }
         g_module_close(g_module);
         ret = -EINVAL;
@@ -178,7 +177,7 @@  bool module_load_one(const char *prefix, const char *lib_name)
     static GHashTable *loaded_modules;
 
     if (!g_module_supported()) {
-        fprintf(stderr, "Module is not supported by system.\n");
+        error_report("Module is not supported by system.");
         return false;
     }
 
diff --git a/util/osdep.c b/util/osdep.c
index f7d06050f7..ef40ae512a 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -484,7 +484,7 @@  void fips_set_state(bool requested)
 #endif /* __linux__ */
 
 #ifdef _FIPS_DEBUG
-    fprintf(stderr, "FIPS mode %s (requested %s)\n",
+    error_report("FIPS mode %s (requested %s)",
             (fips_enabled ? "enabled" : "disabled"),
             (requested ? "enabled" : "disabled"));
 #endif
@@ -511,7 +511,7 @@  int socket_init(void)
     ret = WSAStartup(MAKEWORD(2, 2), &Data);
     if (ret != 0) {
         err = WSAGetLastError();
-        fprintf(stderr, "WSAStartup: %d\n", err);
+        error_report("WSAStartup: %d", err);
         return -1;
     }
     atexit(socket_cleanup);
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 897e8f3ba6..4977594a43 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -35,6 +35,7 @@ 
 #include "sysemu/sysemu.h"
 #include "trace.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/sockets.h"
 #include "qemu/thread.h"
 #include <libgen.h>
@@ -170,7 +171,7 @@  fail_close:
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
-        fprintf(stderr, "Failed to allocate memory: %s\n", strerror(errno));
+        error_report("Failed to allocate memory: %s", strerror(errno));
         abort();
     }
     return ptr;
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index e9b14ab178..84b937865a 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -39,6 +39,7 @@ 
 #include "trace.h"
 #include "qemu/sockets.h"
 #include "qemu/cutils.h"
+#include "qemu/error-report.h"
 
 /* this must come after including "trace.h" */
 #include <shlobj.h>
@@ -46,7 +47,7 @@ 
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
-        fprintf(stderr, "Failed to allocate memory: %lu\n", GetLastError());
+        error_report("Failed to allocate memory: %lu", GetLastError());
         abort();
     }
     return ptr;
diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
index c3caa6c770..62d1dd09df 100644
--- a/util/qemu-coroutine.c
+++ b/util/qemu-coroutine.c
@@ -14,6 +14,7 @@ 
 
 #include "qemu/osdep.h"
 #include "trace.h"
+#include "qemu/error-report.h"
 #include "qemu/thread.h"
 #include "qemu/atomic.h"
 #include "qemu/coroutine.h"
@@ -125,14 +126,13 @@  void qemu_aio_coroutine_enter(AioContext *ctx, Coroutine *co)
          * cause us to enter it twice, potentially even after the coroutine has
          * been deleted */
         if (scheduled) {
-            fprintf(stderr,
-                    "%s: Co-routine was already scheduled in '%s'\n",
-                    __func__, scheduled);
+            error_report("%s: Co-routine was already scheduled in '%s'",
+                         __func__, scheduled);
             abort();
         }
 
         if (to->caller) {
-            fprintf(stderr, "Co-routine re-entered recursively\n");
+            error_report("Co-routine re-entered recursively");
             abort();
         }
 
@@ -185,7 +185,7 @@  void coroutine_fn qemu_coroutine_yield(void)
     trace_qemu_coroutine_yield(self, to);
 
     if (!to) {
-        fprintf(stderr, "Co-routine is yielding to no one\n");
+        error_report("Co-routine is yielding to no one");
         abort();
     }
 
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 838980aaa5..b4d8de376c 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -14,6 +14,7 @@ 
 #include "qemu/thread.h"
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
+#include "qemu/error-report.h"
 #include "qemu-thread-common.h"
 
 static bool name_threads;
@@ -25,14 +26,14 @@  void qemu_thread_naming(bool enable)
 #ifndef CONFIG_THREAD_SETNAME_BYTHREAD
     /* This is a debugging option, not fatal */
     if (enable) {
-        fprintf(stderr, "qemu: thread naming not supported on this host\n");
+        error_report("qemu: thread naming not supported on this host");
     }
 #endif
 }
 
 static void error_exit(int err, const char *msg)
 {
-    fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
+    error_report("qemu: %s: %s", msg, strerror(err));
     abort();
 }
 
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index 56a83333da..9bed338d7e 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -15,6 +15,7 @@ 
 #include "qemu-common.h"
 #include "qemu/thread.h"
 #include "qemu/notify.h"
+#include "qemu/error-report.h"
 #include "qemu-thread-common.h"
 #include <process.h>
 
@@ -25,7 +26,7 @@  void qemu_thread_naming(bool enable)
     /* But note we don't actually name them on Windows yet */
     name_threads = enable;
 
-    fprintf(stderr, "qemu: thread naming not supported on this host\n");
+    error_report("qemu: thread naming not supported on this host");
 }
 
 static void error_exit(int err, const char *msg)
@@ -34,7 +35,7 @@  static void error_exit(int err, const char *msg)
 
     FormatMessage(FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_ALLOCATE_BUFFER,
                   NULL, err, 0, (LPTSTR)&pstr, 2, NULL);
-    fprintf(stderr, "qemu: %s: %s\n", msg, pstr);
+    error_report("qemu: %s: %s", msg, pstr);
     LocalFree(pstr);
     abort();
 }
diff --git a/util/qemu-timer-common.c b/util/qemu-timer-common.c
index baf3317f74..527944da1c 100644
--- a/util/qemu-timer-common.c
+++ b/util/qemu-timer-common.c
@@ -23,6 +23,7 @@ 
  */
 #include "qemu/osdep.h"
 #include "qemu/timer.h"
+#include "qemu/error-report.h"
 
 /***********************************************************/
 /* real time host monotonic timer */
@@ -37,7 +38,7 @@  static void __attribute__((constructor)) init_get_clock(void)
     int ret;
     ret = QueryPerformanceFrequency(&freq);
     if (ret == 0) {
-        fprintf(stderr, "Could not calibrate ticks\n");
+        error_report("Could not calibrate ticks");
         exit(1);
     }
     clock_freq = freq.QuadPart;