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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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