[PATCH v2 0/6] misc: Improve error reporting on Windows

[PATCH v2 0/6] misc: Improve error reporting on Windows

[Philippe Mathieu-Daudé]

Two patches to improve bug reports on Windows.
(i.e. https://bugs.launchpad.net/qemu/+bug/1657841)

Since v1:
- Rebase an old patch from Alistair to use error_report()
  as suggested by Markus
- Fix other uses in util/osdep.c and QEMU guest-agent

Alistair Francis (1):
  util: Replace fprintf(stderr, "*\n" with error_report()

Philippe Mathieu-Daudé (5):
  chardev: Improve error report by calling error_setg_win32()
  util/oslib-win32: Improve error report by calling error_setg_win32()
  util/osdep: Improve error report by calling error_setg_win32()
  qga: Fix a memory leak
  qga: Improve error report by calling error_setg_win32()

 chardev/char-pipe.c          |  2 +-
 chardev/char-win.c           |  2 +-
 qga/channel-win32.c          |  7 ++++---
 qga/commands-win32.c         |  6 +++---
 util/coroutine-sigaltstack.c |  3 ++-
 util/mmap-alloc.c            | 11 ++++++-----
 util/module.c                | 13 ++++++-------
 util/osdep.c                 | 12 ++++++------
 util/oslib-posix.c           |  3 ++-
 util/oslib-win32.c           |  4 +++-
 util/qemu-coroutine.c        | 10 +++++-----
 util/qemu-thread-posix.c     |  5 +++--
 util/qemu-thread-win32.c     |  5 +++--
 util/qemu-timer-common.c     |  3 ++-
 14 files changed, 47 insertions(+), 39 deletions(-)

-- 
2.21.1

[PATCH v2 1/6] chardev: Improve error report by calling error_setg_win32()

[Philippe Mathieu-Daudé]

Use error_setg_win32() which adds a hint similar to strerror(errno)).

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 chardev/char-pipe.c | 2 +-
 chardev/char-win.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/chardev/char-pipe.c b/chardev/char-pipe.c
index 94d714ffcd..fd12c9e63b 100644
--- a/chardev/char-pipe.c
+++ b/chardev/char-pipe.c
@@ -70,7 +70,7 @@ static int win_chr_pipe_init(Chardev *chr, const char *filename,
                               MAXCONNECT, NSENDBUF, NRECVBUF, NTIMEOUT, NULL);
     g_free(openname);
     if (s->file == INVALID_HANDLE_VALUE) {
-        error_setg(errp, "Failed CreateNamedPipe (%lu)", GetLastError());
+        error_setg_win32(errp, GetLastError(), "Failed CreateNamedPipe");
         s->file = NULL;
         goto fail;
     }
diff --git a/chardev/char-win.c b/chardev/char-win.c
index 34825f683d..d4fb44c4dc 100644
--- a/chardev/char-win.c
+++ b/chardev/char-win.c
@@ -96,7 +96,7 @@ int win_chr_serial_init(Chardev *chr, const char *filename, Error **errp)
     s->file = CreateFile(filename, GENERIC_READ | GENERIC_WRITE, 0, NULL,
                       OPEN_EXISTING, FILE_FLAG_OVERLAPPED, 0);
     if (s->file == INVALID_HANDLE_VALUE) {
-        error_setg(errp, "Failed CreateFile (%lu)", GetLastError());
+        error_setg_win32(errp, GetLastError(), "Failed CreateFile");
         s->file = NULL;
         goto fail;
     }
-- 
2.21.1

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

[Philippe Mathieu-Daudé]

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

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

[Markus Armbruster]

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;

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

[Philippe Mathieu-Daudé]

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

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

[Philippe Mathieu-Daudé]

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

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

[Markus Armbruster]

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.

[PATCH v2 3/6] util/oslib-win32: Improve error report by calling error_setg_win32()

[Philippe Mathieu-Daudé]

Use error_setg_win32() which adds a hint similar to strerror(errno)).

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/oslib-win32.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 84b937865a..6a3e6174f6 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -47,7 +47,8 @@
 void *qemu_oom_check(void *ptr)
 {
     if (ptr == NULL) {
-        error_report("Failed to allocate memory: %lu", GetLastError());
+        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
+        error_report("Failed to allocate memory: %s", emsg);
         abort();
     }
     return ptr;
-- 
2.21.1

[PATCH v2 4/6] util/osdep: Improve error report by calling error_setg_win32()

[Philippe Mathieu-Daudé]

Use error_setg_win32() which adds a hint similar to strerror(errno)).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 util/osdep.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/osdep.c b/util/osdep.c
index ef40ae512a..144e217cb9 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -82,8 +82,8 @@ static int qemu_mprotect__osdep(void *addr, size_t size, int prot)
     DWORD old_protect;
 
     if (!VirtualProtect(addr, size, prot, &old_protect)) {
-        error_report("%s: VirtualProtect failed with error code %ld",
-                     __func__, GetLastError());
+        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
+        error_report("%s: VirtualProtect failed: %s", __func__, emsg);
         return -1;
     }
     return 0;
@@ -506,12 +506,12 @@ int socket_init(void)
 {
 #ifdef _WIN32
     WSADATA Data;
-    int ret, err;
+    int ret;
 
     ret = WSAStartup(MAKEWORD(2, 2), &Data);
     if (ret != 0) {
-        err = WSAGetLastError();
-        error_report("WSAStartup: %d", err);
+        g_autofree gchar *emsg = g_win32_error_message(WSAGetLastError());
+        error_report("WSAStartup: %s", emsg);
         return -1;
     }
     atexit(socket_cleanup);
-- 
2.21.1

Re: [PATCH v2 4/6] util/osdep: Improve error report by calling error_setg_win32()

[Marc-André Lureau]

On Thu, Feb 27, 2020 at 5:32 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Use error_setg_win32() which adds a hint similar to strerror(errno)).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  util/osdep.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/util/osdep.c b/util/osdep.c
> index ef40ae512a..144e217cb9 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -82,8 +82,8 @@ static int qemu_mprotect__osdep(void *addr, size_t size, int prot)
>      DWORD old_protect;
>
>      if (!VirtualProtect(addr, size, prot, &old_protect)) {
> -        error_report("%s: VirtualProtect failed with error code %ld",
> -                     __func__, GetLastError());
> +        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> +        error_report("%s: VirtualProtect failed: %s", __func__, emsg);
>          return -1;
>      }
>      return 0;
> @@ -506,12 +506,12 @@ int socket_init(void)
>  {
>  #ifdef _WIN32
>      WSADATA Data;
> -    int ret, err;
> +    int ret;
>
>      ret = WSAStartup(MAKEWORD(2, 2), &Data);
>      if (ret != 0) {
> -        err = WSAGetLastError();
> -        error_report("WSAStartup: %d", err);
> +        g_autofree gchar *emsg = g_win32_error_message(WSAGetLastError());
> +        error_report("WSAStartup: %s", emsg);
>          return -1;
>      }
>      atexit(socket_cleanup);
> --
> 2.21.1
>

[PATCH v2 5/6] qga: Fix a memory leak

[Philippe Mathieu-Daudé]

The string returned by g_win32_error_message() has to be
deallocated with g_free().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/channel-win32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qga/channel-win32.c b/qga/channel-win32.c
index c86f4388db..774205e017 100644
--- a/qga/channel-win32.c
+++ b/qga/channel-win32.c
@@ -302,8 +302,8 @@ static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
                            OPEN_EXISTING,
                            FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL);
     if (c->handle == INVALID_HANDLE_VALUE) {
-        g_critical("error opening path %s: %s", newpath,
-                   g_win32_error_message(GetLastError()));
+        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
+        g_critical("error opening path %s: %s", newpath, emsg);
         return false;
     }
 
-- 
2.21.1

Re: [PATCH v2 5/6] qga: Fix a memory leak

[Marc-André Lureau]

On Thu, Feb 27, 2020 at 5:32 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> The string returned by g_win32_error_message() has to be
> deallocated with g_free().
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  qga/channel-win32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qga/channel-win32.c b/qga/channel-win32.c
> index c86f4388db..774205e017 100644
> --- a/qga/channel-win32.c
> +++ b/qga/channel-win32.c
> @@ -302,8 +302,8 @@ static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
>                             OPEN_EXISTING,
>                             FILE_FLAG_NO_BUFFERING | FILE_FLAG_OVERLAPPED, NULL);
>      if (c->handle == INVALID_HANDLE_VALUE) {
> -        g_critical("error opening path %s: %s", newpath,
> -                   g_win32_error_message(GetLastError()));
> +        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> +        g_critical("error opening path %s: %s", newpath, emsg);
>          return false;
>      }
>
> --
> 2.21.1
>

[PATCH v2 6/6] qga: Improve error report by calling error_setg_win32()

[Philippe Mathieu-Daudé]

Use error_setg_win32() which adds a hint similar to strerror(errno)).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/channel-win32.c  | 3 ++-
 qga/commands-win32.c | 6 +++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/qga/channel-win32.c b/qga/channel-win32.c
index 774205e017..4f04868a76 100644
--- a/qga/channel-win32.c
+++ b/qga/channel-win32.c
@@ -308,7 +308,8 @@ static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
     }
 
     if (method == GA_CHANNEL_ISA_SERIAL && !SetCommTimeouts(c->handle,&comTimeOut)) {
-        g_critical("error setting timeout for com port: %lu",GetLastError());
+        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
+        g_critical("error setting timeout for com port: %s", emsg);
         CloseHandle(c->handle);
         return false;
     }
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 2461fd19bf..8e1f32ea23 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -315,8 +315,7 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
     }
 
     if (!ExitWindowsEx(shutdown_flag, SHTDN_REASON_FLAG_PLANNED)) {
-        slog("guest-shutdown failed: %lu", GetLastError());
-        error_setg(errp, QERR_UNDEFINED_ERROR);
+        error_setg_win32(errp, GetLastError(), "guest-shutdown failed");
     }
 }
 
@@ -1319,7 +1318,8 @@ static DWORD WINAPI do_suspend(LPVOID opaque)
     DWORD ret = 0;
 
     if (!SetSuspendState(*mode == GUEST_SUSPEND_MODE_DISK, TRUE, TRUE)) {
-        slog("failed to suspend guest, %lu", GetLastError());
+        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
+        slog("failed to suspend guest: %s", emsg);
         ret = -1;
     }
     g_free(mode);
-- 
2.21.1

Re: [PATCH v2 6/6] qga: Improve error report by calling error_setg_win32()

[Marc-André Lureau]

Hi

On Thu, Feb 27, 2020 at 5:32 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Use error_setg_win32() which adds a hint similar to strerror(errno)).
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qga/channel-win32.c  | 3 ++-
>  qga/commands-win32.c | 6 +++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/qga/channel-win32.c b/qga/channel-win32.c
> index 774205e017..4f04868a76 100644
> --- a/qga/channel-win32.c
> +++ b/qga/channel-win32.c
> @@ -308,7 +308,8 @@ static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
>      }
>
>      if (method == GA_CHANNEL_ISA_SERIAL && !SetCommTimeouts(c->handle,&comTimeOut)) {
> -        g_critical("error setting timeout for com port: %lu",GetLastError());
> +        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> +        g_critical("error setting timeout for com port: %s", emsg);
>          CloseHandle(c->handle);
>          return false;
>      }
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 2461fd19bf..8e1f32ea23 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -315,8 +315,7 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
>      }
>
>      if (!ExitWindowsEx(shutdown_flag, SHTDN_REASON_FLAG_PLANNED)) {
> -        slog("guest-shutdown failed: %lu", GetLastError());
> -        error_setg(errp, QERR_UNDEFINED_ERROR);
> +        error_setg_win32(errp, GetLastError(), "guest-shutdown failed");

did you drop the slog() intentionally?

>      }
>  }
>
> @@ -1319,7 +1318,8 @@ static DWORD WINAPI do_suspend(LPVOID opaque)
>      DWORD ret = 0;
>
>      if (!SetSuspendState(*mode == GUEST_SUSPEND_MODE_DISK, TRUE, TRUE)) {
> -        slog("failed to suspend guest, %lu", GetLastError());
> +        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
> +        slog("failed to suspend guest: %s", emsg);
>          ret = -1;
>      }
>      g_free(mode);
> --
> 2.21.1
>

Re: [PATCH v2 6/6] qga: Improve error report by calling error_setg_win32()

[Philippe Mathieu-Daudé]

On 2/27/20 6:20 PM, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Feb 27, 2020 at 5:32 PM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> Use error_setg_win32() which adds a hint similar to strerror(errno)).
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   qga/channel-win32.c  | 3 ++-
>>   qga/commands-win32.c | 6 +++---
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/qga/channel-win32.c b/qga/channel-win32.c
>> index 774205e017..4f04868a76 100644
>> --- a/qga/channel-win32.c
>> +++ b/qga/channel-win32.c
>> @@ -308,7 +308,8 @@ static gboolean ga_channel_open(GAChannel *c, GAChannelMethod method,
>>       }
>>
>>       if (method == GA_CHANNEL_ISA_SERIAL && !SetCommTimeouts(c->handle,&comTimeOut)) {
>> -        g_critical("error setting timeout for com port: %lu",GetLastError());
>> +        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
>> +        g_critical("error setting timeout for com port: %s", emsg);
>>           CloseHandle(c->handle);
>>           return false;
>>       }
>> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
>> index 2461fd19bf..8e1f32ea23 100644
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -315,8 +315,7 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
>>       }
>>
>>       if (!ExitWindowsEx(shutdown_flag, SHTDN_REASON_FLAG_PLANNED)) {
>> -        slog("guest-shutdown failed: %lu", GetLastError());
>> -        error_setg(errp, QERR_UNDEFINED_ERROR);
>> +        error_setg_win32(errp, GetLastError(), "guest-shutdown failed");
> 
> did you drop the slog() intentionally?

Oops no :(

> 
>>       }
>>   }
>>
>> @@ -1319,7 +1318,8 @@ static DWORD WINAPI do_suspend(LPVOID opaque)
>>       DWORD ret = 0;
>>
>>       if (!SetSuspendState(*mode == GUEST_SUSPEND_MODE_DISK, TRUE, TRUE)) {
>> -        slog("failed to suspend guest, %lu", GetLastError());
>> +        g_autofree gchar *emsg = g_win32_error_message(GetLastError());
>> +        slog("failed to suspend guest: %s", emsg);
>>           ret = -1;
>>       }
>>       g_free(mode);
>> --
>> 2.21.1
>>
>