[Qemu-devel] [RFC PATCH 0/7] Introduce error_[v]report_errno[val]

[Qemu-devel] [RFC PATCH 0/7] Introduce error_[v]report_errno[val]

[Ian Jackson]

From: Ian Jackson <Ian.Jackson@eu.citrix.com>

This is apropos if a suggestion from Philippe.  I've compile-tested
this but not executed it yet.  Let me know what you think.

Thanks,
Ian.

Ian Jackson (7):
  error reporting: Introduce errnoval parameter to vreport
  error reporting: Provide error_report_errno (and error_vreport_errno)
  error reporting: Use error_report_errno in obvious places
  error reporting: Fix some error messages to use ":" rather than ","
  error reporting: Provide error_report_errnoval (and
    error_vreport_errnoval)
  error reporting: Use error_report_errnoval in obvious places
  error reporting: HACKING: Say to use error_report_errno

 HACKING                      |  1 +
 audio/wavcapture.c           |  2 +-
 block/nvme.c                 |  2 +-
 block/rbd.c                  |  2 +-
 block/sheepdog.c             | 16 ++++-----
 cpus.c                       |  2 +-
 hw/i386/xen/xen-hvm.c        |  2 +-
 hw/ppc/spapr_hcall.c         |  2 +-
 hw/s390x/s390-stattrib-kvm.c |  6 ++--
 hw/s390x/s390-virtio-ccw.c   |  2 +-
 hw/scsi/vhost-scsi.c         |  2 +-
 hw/scsi/vhost-user-scsi.c    |  2 +-
 hw/tpm/tpm_emulator.c        |  2 +-
 hw/xen/xen_pt_load_rom.c     |  4 +--
 include/qemu/error-report.h  |  7 ++++
 migration/migration.c        |  2 +-
 migration/postcopy-ram.c     | 12 +++----
 net/tap-linux.c              |  2 +-
 os-posix.c                   |  8 ++---
 qemu-img.c                   | 10 +++---
 qemu-io-cmds.c               |  4 +--
 qemu-nbd.c                   | 10 +++---
 scsi/qemu-pr-helper.c        |  8 ++---
 slirp/misc.c                 |  4 +--
 target/arm/kvm64.c           |  4 +--
 util/osdep.c                 |  2 +-
 util/qemu-error.c            | 77 ++++++++++++++++++++++++++++++++++++++++----
 vl.c                         |  6 ++--
 28 files changed, 137 insertions(+), 66 deletions(-)

-- 
2.1.4

[Qemu-devel] [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport

[Ian Jackson]

This will allow new callers of vreport to specify that an errno value
should be printed too.  Update all existing callers.

We use strerror rather than strerror_r because strerror_r presents
portability difficulties.  Replacing strerror with strerror_r (or
something else) is left to the future.

No functional change yet.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 util/qemu-error.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/util/qemu-error.c b/util/qemu-error.c
index a25d3b9..9acc4b5 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -191,12 +191,14 @@ bool enable_timestamp_msg;
 /*
  * Print a message to current monitor if we have one, else to stderr.
  * @report_type is the type of message: error, warning or informational.
+ * If @errnoval is nonnegative it is fed to strerror and printed too.
  * Format arguments like vsprintf().  The resulting message should be
  * a single phrase, with no newline or trailing punctuation.
  * Prepend the current location and append a newline.
  * It's wrong to call this in a QMP monitor.  Use error_setg() there.
  */
-static void vreport(report_type type, const char *fmt, va_list ap)
+static void vreport(report_type type, int errnoval,
+                    const char *fmt, va_list ap)
 {
     GTimeVal tv;
     gchar *timestr;
@@ -222,6 +224,11 @@ static void vreport(report_type type, const char *fmt, va_list ap)
     }
 
     error_vprintf(fmt, ap);
+
+    if (errnoval >= 0) {
+        error_printf(": %s", strerror(errnoval));
+    }
+
     error_printf("\n");
 }
 
@@ -234,7 +241,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
  */
 void error_vreport(const char *fmt, va_list ap)
 {
-    vreport(REPORT_TYPE_ERROR, fmt, ap);
+    vreport(REPORT_TYPE_ERROR, -1, fmt, ap);
 }
 
 /*
@@ -246,7 +253,7 @@ void error_vreport(const char *fmt, va_list ap)
  */
 void warn_vreport(const char *fmt, va_list ap)
 {
-    vreport(REPORT_TYPE_WARNING, fmt, ap);
+    vreport(REPORT_TYPE_WARNING, -1, fmt, ap);
 }
 
 /*
@@ -259,7 +266,7 @@ void warn_vreport(const char *fmt, va_list ap)
  */
 void info_vreport(const char *fmt, va_list ap)
 {
-    vreport(REPORT_TYPE_INFO, fmt, ap);
+    vreport(REPORT_TYPE_INFO, -1, fmt, ap);
 }
 
 /*
@@ -274,7 +281,7 @@ void error_report(const char *fmt, ...)
     va_list ap;
 
     va_start(ap, fmt);
-    vreport(REPORT_TYPE_ERROR, fmt, ap);
+    vreport(REPORT_TYPE_ERROR, -1, fmt, ap);
     va_end(ap);
 }
 
@@ -290,7 +297,7 @@ void warn_report(const char *fmt, ...)
     va_list ap;
 
     va_start(ap, fmt);
-    vreport(REPORT_TYPE_WARNING, fmt, ap);
+    vreport(REPORT_TYPE_WARNING, -1, fmt, ap);
     va_end(ap);
 }
 
@@ -307,6 +314,6 @@ void info_report(const char *fmt, ...)
     va_list ap;
 
     va_start(ap, fmt);
-    vreport(REPORT_TYPE_INFO, fmt, ap);
+    vreport(REPORT_TYPE_INFO, -1, fmt, ap);
     va_end(ap);
 }
-- 
2.1.4

[Qemu-devel] [RFC PATCH 2/7] error reporting: Provide error_report_errno (and error_vreport_errno)

[Ian Jackson]

This will let us replace a lot of open coded calls to perror,
strerror, etc.

No callers yet so no functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/qemu/error-report.h |  2 ++
 util/qemu-error.c           | 28 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index e1c8ae1..2b29678 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -37,10 +37,12 @@ void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void error_set_progname(const char *argv0);
 
 void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
+void error_vreport_errno(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
 void warn_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
 void info_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
 
 void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
+void error_report_errno(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 9acc4b5..428c762 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -245,6 +245,18 @@ void error_vreport(const char *fmt, va_list ap)
 }
 
 /*
+ * Print an error message to current monitor if we have one, else to stderr.
+ * Format arguments like vsprintf().  The resulting message should be
+ * a single phrase, with no newline or trailing punctuation.
+ * Prepend the current location and append ": " strerror(errno) "\n".
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+void error_vreport_errno(const char *fmt, va_list ap)
+{
+    vreport(REPORT_TYPE_ERROR, errno, fmt, ap);
+}
+
+/*
  * Print a warning message to current monitor if we have one, else to stderr.
  * Format arguments like vsprintf().  The resulting message should be
  * a single phrase, with no newline or trailing punctuation.
@@ -286,6 +298,22 @@ void error_report(const char *fmt, ...)
 }
 
 /*
+ * Print an error message to current monitor if we have one, else to stderr.
+ * Format arguments like sprintf().  The resulting message should be
+ * a single phrase, with no newline or trailing punctuation.
+ * Prepend the current location and append ": " strerror(errno) "\n".
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+void error_report_errno(const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    vreport(REPORT_TYPE_ERROR, errno, fmt, ap);
+    va_end(ap);
+}
+
+/*
  * Print a warning message to current monitor if we have one, else to stderr.
  * Format arguments like sprintf(). The resulting message should be a
  * single phrase, with no newline or trailing punctuation.
-- 
2.1.4

[Qemu-devel] [RFC PATCH 3/7] error reporting: Use error_report_errno in obvious places

[Ian Jackson]

This patch is the result of

  git-grep -l 'error_report.*strerror' | xargs perl -p -i~ ../t

with ../t containing

  s{error_report\("(.*): \%s"(, .*)?, strerror\(errno\)\)\;}{error_report_errno\("$1"$2)\;}

Since this is an automatically generated patch, it does not contain
any cleanups of the occasional idiosyncratic messages.  That is left
to the future.

No functional change, since error_report_errno does exactly what this
previous open-coded pattern does.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 audio/wavcapture.c       |  2 +-
 hw/i386/xen/xen-hvm.c    |  2 +-
 hw/tpm/tpm_emulator.c    |  2 +-
 hw/xen/xen_pt_load_rom.c |  4 ++--
 migration/postcopy-ram.c | 12 ++++++------
 net/tap-linux.c          |  2 +-
 os-posix.c               |  8 ++++----
 qemu-nbd.c               |  4 ++--
 scsi/qemu-pr-helper.c    |  4 ++--
 slirp/misc.c             |  4 ++--
 util/osdep.c             |  2 +-
 vl.c                     |  6 +++---
 12 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/audio/wavcapture.c b/audio/wavcapture.c
index cf31ed6..7046365 100644
--- a/audio/wavcapture.c
+++ b/audio/wavcapture.c
@@ -66,7 +66,7 @@ static void wav_destroy (void *opaque)
         }
     doclose:
         if (fclose (wav->f)) {
-            error_report("wav_destroy: fclose failed: %s", strerror(errno));
+            error_report_errno("wav_destroy: fclose failed");
         }
     }
 
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index caa563b..2778c92 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -414,7 +414,7 @@ go_physmap:
                                    (start_addr + size - 1) >> TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
     if (rc) {
-        error_report("pin_memory_cacheattr failed: %s", strerror(errno));
+        error_report_errno("pin_memory_cacheattr failed");
     }
     return xen_save_physmap(state, physmap);
 }
diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 6418ef0..4345c1d 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -190,7 +190,7 @@ static int tpm_emulator_probe_caps(TPMEmulator *tpm_emu)
 {
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_GET_CAPABILITY,
                              &tpm_emu->caps, 0, sizeof(tpm_emu->caps)) < 0) {
-        error_report("tpm-emulator: probing failed : %s", strerror(errno));
+        error_report_errno("tpm-emulator: probing failed ");
         return -1;
     }
 
diff --git a/hw/xen/xen_pt_load_rom.c b/hw/xen/xen_pt_load_rom.c
index 71063c4..bbfe423 100644
--- a/hw/xen/xen_pt_load_rom.c
+++ b/hw/xen/xen_pt_load_rom.c
@@ -43,12 +43,12 @@ void *pci_assign_dev_load_option_rom(PCIDevice *dev, struct Object *owner,
     fp = fopen(rom_file, "r+");
     if (fp == NULL) {
         if (errno != ENOENT) {
-            error_report("pci-assign: Cannot open %s: %s", rom_file, strerror(errno));
+            error_report_errno("pci-assign: Cannot open %s", rom_file);
         }
         return NULL;
     }
     if (fstat(fileno(fp), &st) == -1) {
-        error_report("pci-assign: Cannot stat %s: %s", rom_file, strerror(errno));
+        error_report_errno("pci-assign: Cannot stat %s", rom_file);
         goto close_rom;
     }
 
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 8ceeaa2..6bfe002 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -406,14 +406,14 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
 
     if (ioctl(ufd, UFFDIO_REGISTER, &reg_struct)) {
-        error_report("%s userfault register: %s", __func__, strerror(errno));
+        error_report_errno("%s userfault register", __func__);
         goto out;
     }
 
     range_struct.start = (uintptr_t)testarea;
     range_struct.len = pagesize;
     if (ioctl(ufd, UFFDIO_UNREGISTER, &range_struct)) {
-        error_report("%s userfault unregister: %s", __func__, strerror(errno));
+        error_report_errno("%s userfault unregister", __func__);
         goto out;
     }
 
@@ -543,7 +543,7 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
 
     if (enable_mlock) {
         if (os_mlock() < 0) {
-            error_report("mlock: %s", strerror(errno));
+            error_report_errno("mlock");
             /*
              * It doesn't feel right to fail at this point, we have a valid
              * VM state.
@@ -624,7 +624,7 @@ static int ram_block_enable_notify(const char *block_name, void *host_addr,
 
     /* Now tell our userfault_fd that it's responsible for this area */
     if (ioctl(mis->userfault_fd, UFFDIO_REGISTER, &reg_struct)) {
-        error_report("%s userfault register: %s", __func__, strerror(errno));
+        error_report_errno("%s userfault register", __func__);
         return -1;
     }
     if (!(reg_struct.ioctls & ((__u64)1 << _UFFDIO_COPY))) {
@@ -876,7 +876,7 @@ static void *postcopy_ram_fault_thread(void *opaque)
 
         poll_result = poll(pfd, pfd_len, -1 /* Wait forever */);
         if (poll_result == -1) {
-            error_report("%s: userfault poll: %s", __func__, strerror(errno));
+            error_report_errno("%s: userfault poll", __func__);
             break;
         }
 
@@ -1199,7 +1199,7 @@ void *postcopy_get_tmp_page(MigrationIncomingState *mis)
                              MAP_ANONYMOUS, -1, 0);
         if (mis->postcopy_tmp_page == MAP_FAILED) {
             mis->postcopy_tmp_page = NULL;
-            error_report("%s: %s", __func__, strerror(errno));
+            error_report_errno("%s", __func__);
             return NULL;
         }
     }
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 535b1dd..897e788 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -152,7 +152,7 @@ int tap_probe_vnet_hdr(int fd)
     struct ifreq ifr;
 
     if (ioctl(fd, TUNGETIFF, &ifr) != 0) {
-        error_report("TUNGETIFF ioctl() failed: %s", strerror(errno));
+        error_report_errno("TUNGETIFF ioctl() failed");
         return 0;
     }
 
diff --git a/os-posix.c b/os-posix.c
index 24eb700..e6edb7d 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -125,7 +125,7 @@ void os_set_proc_name(const char *s)
     /* Could rewrite argv[0] too, but that's a bit more complicated.
        This simple way is enough for `top'. */
     if (prctl(PR_SET_NAME, name)) {
-        error_report("unable to change process name: %s", strerror(errno));
+        error_report_errno("unable to change process name");
         exit(1);
     }
 #else
@@ -247,7 +247,7 @@ static void change_root(void)
             exit(1);
         }
         if (chdir("/")) {
-            error_report("not able to chdir to /: %s", strerror(errno));
+            error_report_errno("not able to chdir to /");
             exit(1);
         }
     }
@@ -309,7 +309,7 @@ void os_setup_post(void)
 
     if (daemonize) {
         if (chdir("/")) {
-            error_report("not able to chdir to /: %s", strerror(errno));
+            error_report_errno("not able to chdir to /");
             exit(1);
         }
         TFR(fd = qemu_open("/dev/null", O_RDWR));
@@ -383,7 +383,7 @@ int os_mlock(void)
 
     ret = mlockall(MCL_CURRENT | MCL_FUTURE);
     if (ret < 0) {
-        error_report("mlockall: %s", strerror(errno));
+        error_report_errno("mlockall");
     }
 
     return ret;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 0af0560..47b6957 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -847,7 +847,7 @@ int main(int argc, char **argv)
          */
         pid = fork();
         if (pid < 0) {
-            error_report("Failed to fork: %s", strerror(errno));
+            error_report_errno("Failed to fork");
             exit(EXIT_FAILURE);
         } else if (pid == 0) {
             close(stderr_fd[0]);
@@ -857,7 +857,7 @@ int main(int argc, char **argv)
             old_stderr = dup(STDERR_FILENO);
             dup2(stderr_fd[1], STDERR_FILENO);
             if (ret < 0) {
-                error_report("Failed to daemonize: %s", strerror(errno));
+                error_report_errno("Failed to daemonize");
                 exit(EXIT_FAILURE);
             }
 
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index d0f8317..663a8a8 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -1085,7 +1085,7 @@ int main(int argc, char **argv)
 
     if (daemonize) {
         if (daemon(0, 0) < 0) {
-            error_report("Failed to daemonize: %s", strerror(errno));
+            error_report_errno("Failed to daemonize");
             exit(EXIT_FAILURE);
         }
     }
@@ -1095,7 +1095,7 @@ int main(int argc, char **argv)
 
 #ifdef CONFIG_LIBCAP
     if (drop_privileges() < 0) {
-        error_report("Failed to drop privileges: %s", strerror(errno));
+        error_report_errno("Failed to drop privileges");
         exit(EXIT_FAILURE);
     }
 #endif
diff --git a/slirp/misc.c b/slirp/misc.c
index 260187b..11a8bd1 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -111,7 +111,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
 		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
 		    listen(s, 1) < 0) {
-			error_report("Error: inet socket: %s", strerror(errno));
+			error_report_errno("Error: inet socket");
 			if (s >= 0) {
 			    closesocket(s);
 			}
@@ -123,7 +123,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 	pid = fork();
 	switch(pid) {
 	 case -1:
-		error_report("Error: fork failed: %s", strerror(errno));
+		error_report_errno("Error: fork failed");
 		close(s);
 		return 0;
 
diff --git a/util/osdep.c b/util/osdep.c
index a73de0e..b78b98b 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -89,7 +89,7 @@ static int qemu_mprotect__osdep(void *addr, size_t size, int prot)
     return 0;
 #else
     if (mprotect(addr, size, prot)) {
-        error_report("%s: mprotect failed: %s", __func__, strerror(errno));
+        error_report_errno("%s: mprotect failed", __func__);
         return -1;
     }
     return 0;
diff --git a/vl.c b/vl.c
index d37e857..2f09210 100644
--- a/vl.c
+++ b/vl.c
@@ -1194,7 +1194,7 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
     }
 #endif
     if (dupfd == -1) {
-        error_report("error duplicating fd: %s", strerror(errno));
+        error_report_errno("error duplicating fd");
         return -1;
     }
 
@@ -4069,7 +4069,7 @@ int main(int argc, char **argv, char **envp)
                 }
                 vmstate_dump_file = fopen(optarg, "w");
                 if (vmstate_dump_file == NULL) {
-                    error_report("open %s: %s", optarg, strerror(errno));
+                    error_report_errno("open %s", optarg);
                     exit(1);
                 }
                 break;
@@ -4094,7 +4094,7 @@ int main(int argc, char **argv, char **envp)
     rcu_disable_atfork();
 
     if (pid_file && qemu_create_pidfile(pid_file) != 0) {
-        error_report("could not acquire pid file: %s", strerror(errno));
+        error_report_errno("could not acquire pid file");
         exit(1);
     }
 
-- 
2.1.4

[Qemu-devel] [RFC PATCH 4/7] error reporting: Fix some error messages to use ":" rather than ", "

[Ian Jackson]

This patch is the result of

  git-grep -l 'error_report.*strerror' | xargs perl -p -i~ ../t

with ../t containing

  s{error_report\("(.*), \%s"(, .*)?, strerror\(errno\)\)\;}{error_report_errno\("$1"$2)\;}

Note the comma here    ^
which is a one-character difference from the previous patch.

The effect is that a lot of messages which used to say

  something went wrong, <errno string>

now say

  something went wrong: <errno string>

And of course we remove a lot of open-coded calls.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 block/sheepdog.c      | 16 ++++++++--------
 scsi/qemu-pr-helper.c |  4 ++--
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 387f59c..70e4bba 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -603,13 +603,13 @@ static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
 
     ret = qemu_co_send(sockfd, hdr, sizeof(*hdr));
     if (ret != sizeof(*hdr)) {
-        error_report("failed to send a req, %s", strerror(errno));
+        error_report_errno("failed to send a req");
         return -errno;
     }
 
     ret = qemu_co_send(sockfd, data, *wlen);
     if (ret != *wlen) {
-        error_report("failed to send a req, %s", strerror(errno));
+        error_report_errno("failed to send a req");
         return -errno;
     }
 
@@ -660,7 +660,7 @@ static coroutine_fn void do_co_req(void *opaque)
 
     ret = qemu_co_recv(sockfd, hdr, sizeof(*hdr));
     if (ret != sizeof(*hdr)) {
-        error_report("failed to get a rsp, %s", strerror(errno));
+        error_report_errno("failed to get a rsp");
         ret = -errno;
         goto out;
     }
@@ -672,7 +672,7 @@ static coroutine_fn void do_co_req(void *opaque)
     if (*rlen) {
         ret = qemu_co_recv(sockfd, data, *rlen);
         if (ret != *rlen) {
-            error_report("failed to get the data, %s", strerror(errno));
+            error_report_errno("failed to get the data");
             ret = -errno;
             goto out;
         }
@@ -809,7 +809,7 @@ static void coroutine_fn aio_read_response(void *opaque)
     /* read a header */
     ret = qemu_co_recv(fd, &rsp, sizeof(rsp));
     if (ret != sizeof(rsp)) {
-        error_report("failed to get the header, %s", strerror(errno));
+        error_report_errno("failed to get the header");
         goto err;
     }
 
@@ -851,7 +851,7 @@ static void coroutine_fn aio_read_response(void *opaque)
         ret = qemu_co_recvv(fd, acb->qiov->iov, acb->qiov->niov,
                             aio_req->iov_offset, rsp.data_length);
         if (ret != rsp.data_length) {
-            error_report("failed to get the data, %s", strerror(errno));
+            error_report_errno("failed to get the data");
             goto err;
         }
         break;
@@ -1362,14 +1362,14 @@ static void coroutine_fn add_aio_request(BDRVSheepdogState *s, AIOReq *aio_req,
     /* send a header */
     ret = qemu_co_send(s->fd, &hdr, sizeof(hdr));
     if (ret != sizeof(hdr)) {
-        error_report("failed to send a req, %s", strerror(errno));
+        error_report_errno("failed to send a req");
         goto out;
     }
 
     if (wlen) {
         ret = qemu_co_sendv(s->fd, iov, niov, aio_req->iov_offset, wlen);
         if (ret != wlen) {
-            error_report("failed to send a data, %s", strerror(errno));
+            error_report_errno("failed to send a data");
         }
     }
 out:
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 663a8a8..ff56313 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -118,12 +118,12 @@ static void write_pidfile(void)
 
     pidfd = qemu_open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
     if (pidfd == -1) {
-        error_report("Cannot open pid file, %s", strerror(errno));
+        error_report_errno("Cannot open pid file");
         exit(EXIT_FAILURE);
     }
 
     if (lockf(pidfd, F_TLOCK, 0)) {
-        error_report("Cannot lock pid file, %s", strerror(errno));
+        error_report_errno("Cannot lock pid file");
         goto fail;
     }
     if (ftruncate(pidfd, 0)) {
-- 
2.1.4

[Qemu-devel] [RFC PATCH 5/7] error reporting: Provide error_report_errnoval (and error_vreport_errnoval)

[Ian Jackson]

This will let us replace more open coded calls to error_report and
strerror.

I have chosen to provide all of
   error_report_errno       error_vreport_errno
   error_report_errnoval    error_vreport_errnoval
because the former are much more common, and deserve a short spelling;
whereas there are still at least 30-40 potential callers of the latter.

No callers yet so no functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/qemu/error-report.h |  5 +++++
 util/qemu-error.c           | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 2b29678..5178173 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -46,6 +46,11 @@ void error_report_errno(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 
+void error_vreport_errnoval(int errnoval,
+                            const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
+void error_report_errnoval(int errnoval,
+                           const char *fmt, ...) GCC_FMT_ATTR(2, 3);
+
 const char *error_get_progname(void);
 extern bool enable_timestamp_msg;
 
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 428c762..8add1f3 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -257,6 +257,18 @@ void error_vreport_errno(const char *fmt, va_list ap)
 }
 
 /*
+ * Print an error message to current monitor if we have one, else to stderr.
+ * Format arguments like vsprintf().  The resulting message should be
+ * a single phrase, with no newline or trailing punctuation.
+ * Prepend the current location and append ": " strerror(errnoval) "\n".
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+void error_vreport_errnoval(int errnoval, const char *fmt, va_list ap)
+{
+    vreport(REPORT_TYPE_ERROR, errnoval, fmt, ap);
+}
+
+/*
  * Print a warning message to current monitor if we have one, else to stderr.
  * Format arguments like vsprintf().  The resulting message should be
  * a single phrase, with no newline or trailing punctuation.
@@ -314,6 +326,22 @@ void error_report_errno(const char *fmt, ...)
 }
 
 /*
+ * Print an error message to current monitor if we have one, else to stderr.
+ * Format arguments like sprintf().  The resulting message should be
+ * a single phrase, with no newline or trailing punctuation.
+ * Prepend the current location and append ": " strerror(errnoval) "\n".
+ * It's wrong to call this in a QMP monitor.  Use error_setg() there.
+ */
+void error_report_errnoval(int errnoval, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    vreport(REPORT_TYPE_ERROR, errnoval, fmt, ap);
+    va_end(ap);
+}
+
+/*
  * Print a warning message to current monitor if we have one, else to stderr.
  * Format arguments like sprintf(). The resulting message should be a
  * single phrase, with no newline or trailing punctuation.
-- 
2.1.4

[Qemu-devel] [RFC PATCH 6/7] error reporting: Use error_report_errnoval in obvious places

[Ian Jackson]

This patch is the result of

  git-grep -l 'error_report.*strerror' | xargs perl -p -i~ ../t

with ../t containing

  s{error_report\("(.*): \%s"(, .*)?, strerror\((.*)\)\)\;}{error_report_errnoval\($3, "$1"$2)\;}

Like the previous patch to use error_report_errno, this patch does not
contain any cleanups of the occasional idiosyncratic messages.  That
is left to the future.

No functional change, since error_report_errnoval does exactly what
this previous open-coded pattern does.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 block/nvme.c                 |  2 +-
 block/rbd.c                  |  2 +-
 cpus.c                       |  2 +-
 hw/ppc/spapr_hcall.c         |  2 +-
 hw/s390x/s390-stattrib-kvm.c |  6 +++---
 hw/s390x/s390-virtio-ccw.c   |  2 +-
 hw/scsi/vhost-scsi.c         |  2 +-
 hw/scsi/vhost-user-scsi.c    |  2 +-
 migration/migration.c        |  2 +-
 qemu-img.c                   | 10 +++++-----
 qemu-io-cmds.c               |  4 ++--
 qemu-nbd.c                   |  6 +++---
 target/arm/kvm64.c           |  4 ++--
 13 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index c4f3a7b..d479ea9 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -1144,7 +1144,7 @@ static void nvme_register_buf(BlockDriverState *bs, void *host, size_t size)
         /* FIXME: we may run out of IOVA addresses after repeated
          * bdrv_register_buf/bdrv_unregister_buf, because nvme_vfio_dma_unmap
          * doesn't reclaim addresses for fixed mappings. */
-        error_report("nvme_register_buf failed: %s", strerror(-ret));
+        error_report_errnoval(-ret, "nvme_register_buf failed");
     }
 }
 
diff --git a/block/rbd.c b/block/rbd.c
index c9359d0..e7b5d15 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1020,7 +1020,7 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
 
     r = rbd_snap_create(s->image, sn_info->name);
     if (r < 0) {
-        error_report("failed to create snap: %s", strerror(-r));
+        error_report_errnoval(-r, "failed to create snap");
         return r;
     }
 
diff --git a/cpus.c b/cpus.c
index 38eba8b..f62b3a8 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1200,7 +1200,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
 
     r = kvm_init_vcpu(cpu);
     if (r < 0) {
-        error_report("kvm_init_vcpu failed: %s", strerror(-r));
+        error_report_errnoval(-r, "kvm_init_vcpu failed");
         exit(1);
     }
 
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 16bccdd..112156f 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -693,7 +693,7 @@ static void do_push_sregs_to_kvm_pr(CPUState *cs, run_on_cpu_data data)
 
     ret = kvmppc_put_books_sregs(POWERPC_CPU(cs));
     if (ret < 0) {
-        error_report("failed to push sregs to KVM: %s", strerror(-ret));
+        error_report_errnoval(-ret, "failed to push sregs to KVM");
         exit(1);
     }
 }
diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
index 480551c..fea2bce 100644
--- a/hw/s390x/s390-stattrib-kvm.c
+++ b/hw/s390x/s390-stattrib-kvm.c
@@ -52,7 +52,7 @@ static int kvm_s390_stattrib_read_helper(S390StAttribState *sa,
 
     r = kvm_vm_ioctl(kvm_state, KVM_S390_GET_CMMA_BITS, &clog);
     if (r < 0) {
-        error_report("KVM_S390_GET_CMMA_BITS failed: %s", strerror(-r));
+        error_report_errnoval(-r, "KVM_S390_GET_CMMA_BITS failed");
         return r;
     }
 
@@ -119,7 +119,7 @@ static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
             clog.values = (uint64_t)(sas->incoming_buffer + cx);
             r = kvm_vm_ioctl(kvm_state, KVM_S390_SET_CMMA_BITS, &clog);
             if (r) {
-                error_report("KVM_S390_SET_CMMA_BITS failed: %s", strerror(-r));
+                error_report_errnoval(-r, "KVM_S390_SET_CMMA_BITS failed");
                 return;
             }
         }
@@ -129,7 +129,7 @@ static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
             clog.values = (uint64_t)(sas->incoming_buffer + cx);
             r = kvm_vm_ioctl(kvm_state, KVM_S390_SET_CMMA_BITS, &clog);
             if (r) {
-                error_report("KVM_S390_SET_CMMA_BITS failed: %s", strerror(-r));
+                error_report_errnoval(-r, "KVM_S390_SET_CMMA_BITS failed");
             }
         }
         g_free(sas->incoming_buffer);
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 435f7c9..d580b48 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -228,7 +228,7 @@ static int gtod_load(QEMUFile *f, void *opaque, int version_id)
 
     r = s390_set_clock(&tod_high, &tod_low);
     if (r) {
-        error_report("Unable to set KVM guest TOD clock: %s", strerror(-r));
+        error_report_errnoval(-r, "Unable to set KVM guest TOD clock");
     }
 
     return r;
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 9c1bea8..354aaef 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -123,7 +123,7 @@ static void vhost_scsi_set_status(VirtIODevice *vdev, uint8_t val)
 
         ret = vhost_scsi_start(s);
         if (ret < 0) {
-            error_report("unable to start vhost-scsi: %s", strerror(-ret));
+            error_report_errnoval(-ret, "unable to start vhost-scsi");
             exit(1);
         }
     } else {
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 9389ed4..aa027e1 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -52,7 +52,7 @@ static void vhost_user_scsi_set_status(VirtIODevice *vdev, uint8_t status)
 
         ret = vhost_scsi_common_start(vsc);
         if (ret < 0) {
-            error_report("unable to start vhost-user-scsi: %s", strerror(-ret));
+            error_report_errnoval(-ret, "unable to start vhost-user-scsi");
             exit(1);
         }
     } else {
diff --git a/migration/migration.c b/migration/migration.c
index 0bdb28e..2732519 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -403,7 +403,7 @@ static void process_incoming_migration_co(void *opaque)
 
         migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                           MIGRATION_STATUS_FAILED);
-        error_report("load of migration failed: %s", strerror(-ret));
+        error_report_errnoval(-ret, "load of migration failed");
         qemu_fclose(mis->from_src_file);
         if (multifd_load_cleanup(&local_err) != 0) {
             error_report_err(local_err);
diff --git a/qemu-img.c b/qemu-img.c
index 855fa52..3527455 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -820,7 +820,7 @@ static int img_check(int argc, char **argv)
 
     if (ret || check->check_errors) {
         if (ret) {
-            error_report("Check failed: %s", strerror(-ret));
+            error_report_errnoval(-ret, "Check failed");
         } else {
             error_report("Check failed");
         }
@@ -2831,7 +2831,7 @@ static int img_map(int argc, char **argv)
         ret = get_block_status(bs, offset, n, &next);
 
         if (ret < 0) {
-            error_report("Could not read file metadata: %s", strerror(-ret));
+            error_report_errnoval(-ret, "Could not read file metadata");
             goto out;
         }
 
@@ -3730,7 +3730,7 @@ static int img_amend(int argc, char **argv)
     ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL);
     qemu_progress_print(100.f, 0);
     if (ret < 0) {
-        error_report("Error while amending options: %s", strerror(-ret));
+        error_report_errnoval(-ret, "Error while amending options");
         goto out;
     }
 
@@ -3770,7 +3770,7 @@ typedef struct BenchData {
 static void bench_undrained_flush_cb(void *opaque, int ret)
 {
     if (ret < 0) {
-        error_report("Failed flush request: %s", strerror(-ret));
+        error_report_errnoval(-ret, "Failed flush request");
         exit(EXIT_FAILURE);
     }
 }
@@ -3781,7 +3781,7 @@ static void bench_cb(void *opaque, int ret)
     BlockAIOCB *acb;
 
     if (ret < 0) {
-        error_report("Failed request: %s", strerror(-ret));
+        error_report_errnoval(-ret, "Failed request");
         exit(EXIT_FAILURE);
     }
 
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 9b3cd00..379a1ee 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1862,14 +1862,14 @@ static int map_f(BlockBackend *blk, int argc, char **argv)
     offset = 0;
     bytes = blk_getlength(blk);
     if (bytes < 0) {
-        error_report("Failed to query image length: %s", strerror(-bytes));
+        error_report_errnoval(-bytes, "Failed to query image length");
         return 0;
     }
 
     while (bytes) {
         ret = map_is_allocated(blk_bs(blk), offset, bytes, &num);
         if (ret < 0) {
-            error_report("Failed to get allocation status: %s", strerror(-ret));
+            error_report_errnoval(-ret, "Failed to get allocation status");
             return 0;
         } else if (!num) {
             error_report("Unexpected end of image");
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 47b6957..70cb564 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -179,7 +179,7 @@ static int find_partition(BlockBackend *blk, int partition,
 
     ret = blk_pread(blk, 0, data, sizeof(data));
     if (ret < 0) {
-        error_report("error while reading: %s", strerror(-ret));
+        error_report_errnoval(-ret, "error while reading");
         exit(EXIT_FAILURE);
     }
 
@@ -202,7 +202,7 @@ static int find_partition(BlockBackend *blk, int partition,
             ret = blk_pread(blk, mbr[i].start_sector_abs * MBR_SIZE,
                             data1, sizeof(data1));
             if (ret < 0) {
-                error_report("error while reading: %s", strerror(-ret));
+                error_report_errnoval(-ret, "error while reading");
                 exit(EXIT_FAILURE);
             }
 
@@ -1021,7 +1021,7 @@ int main(int argc, char **argv)
 
         ret = pthread_create(&client_thread, NULL, nbd_client_thread, device);
         if (ret != 0) {
-            error_report("Failed to create client thread: %s", strerror(ret));
+            error_report_errnoval(ret, "Failed to create client thread");
             exit(EXIT_FAILURE);
         }
     } else {
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index e0b8246..64b47b6 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -387,13 +387,13 @@ static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
 
     err = kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr);
     if (err != 0) {
-        error_report("PMU: KVM_HAS_DEVICE_ATTR: %s", strerror(-err));
+        error_report_errnoval(-err, "PMU: KVM_HAS_DEVICE_ATTR");
         return false;
     }
 
     err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, attr);
     if (err != 0) {
-        error_report("PMU: KVM_SET_DEVICE_ATTR: %s", strerror(-err));
+        error_report_errnoval(-err, "PMU: KVM_SET_DEVICE_ATTR");
         return false;
     }
 
-- 
2.1.4

[Qemu-devel] [RFC PATCH 7/7] error reporting: HACKING: Say to use error_report_errno

[Ian Jackson]

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 HACKING | 1 +
 1 file changed, 1 insertion(+)

diff --git a/HACKING b/HACKING
index 4125c97..95563a3 100644
--- a/HACKING
+++ b/HACKING
@@ -189,6 +189,7 @@ error_report() or error_vreport() from error-report.h.  This ensures the
 error is reported in the right place (current monitor or stderr), and in
 a uniform format.
 
+Use error_report_errno rather than open-coding calls to strerror().
 Use error_printf() & friends to print additional information.
 
 error_report() prints the current location.  In certain common cases
-- 
2.1.4

Re: [Qemu-devel] [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On 04/26/2018 11:53 AM, Ian Jackson wrote:
> This will allow new callers of vreport to specify that an errno value
> should be printed too.  Update all existing callers.
> 
> We use strerror rather than strerror_r because strerror_r presents
> portability difficulties.  Replacing strerror with strerror_r (or
> something else) is left to the future.

Is g_strerror() suitably easy to use, which would at least avoid the
portability difficulties?

> 
> No functional change yet.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  util/qemu-error.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index a25d3b9..9acc4b5 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -191,12 +191,14 @@ bool enable_timestamp_msg;
>  /*
>   * Print a message to current monitor if we have one, else to stderr.
>   * @report_type is the type of message: error, warning or informational.
> + * If @errnoval is nonnegative it is fed to strerror and printed too.

That implies 0 is fed to strerror(), which is not the case.  Better
would be "If @errnoval is positive...".  Or, if we wanted, we could say
"If @errnoval is not zero, its absolute value is fed to strerror" to let
people pass in both EIO and -EIO for the same output (something that
strerror() can't do, but our wrapper can) - but I don't know if that
convenience is a good thing.

>   * Format arguments like vsprintf().  The resulting message should be
>   * a single phrase, with no newline or trailing punctuation.
>   * Prepend the current location and append a newline.
>   * It's wrong to call this in a QMP monitor.  Use error_setg() there.
>   */
> -static void vreport(report_type type, const char *fmt, va_list ap)
> +static void vreport(report_type type, int errnoval,

Bikeshedding: Is 'err' or 'errval' a better name in terms of length and
legibility?

> +                    const char *fmt, va_list ap)
>  {
>      GTimeVal tv;
>      gchar *timestr;
> @@ -222,6 +224,11 @@ static void vreport(report_type type, const char *fmt, va_list ap)
>      }
>  
>      error_vprintf(fmt, ap);
> +
> +    if (errnoval >= 0) {
> +        error_printf(": %s", strerror(errnoval));

Off-by-one.  You do NOT want to print strerror(0) (that results in
appending ": Success" or similar to what is supposed to be an error
message).

> +    }
> +
>      error_printf("\n");
>  }
>  
> @@ -234,7 +241,7 @@ static void vreport(report_type type, const char *fmt, va_list ap)
>   */
>  void error_vreport(const char *fmt, va_list ap)
>  {
> -    vreport(REPORT_TYPE_ERROR, fmt, ap);
> +    vreport(REPORT_TYPE_ERROR, -1, fmt, ap);

Passing -1 to suppress the output feels awkward, especially if 0 can be
used for the same purpose and would then let us use -errno and errno
interchangeable by passing the absolute value to sterror.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 2/7] error reporting: Provide error_report_errno (and error_vreport_errno)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1596 bytes --]

On 04/26/2018 11:53 AM, Ian Jackson wrote:
> This will let us replace a lot of open coded calls to perror,
> strerror, etc.
> 
> No callers yet so no functional change.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  include/qemu/error-report.h |  2 ++
>  util/qemu-error.c           | 28 ++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index e1c8ae1..2b29678 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -37,10 +37,12 @@ void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  void error_set_progname(const char *argv0);
>  
>  void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
> +void error_vreport_errno(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);

I'd rather us ALWAYS require an explicit argument for the error value,
rather than reading errno.  That way, it's easier to audit that the
caller is passing in a known error value, rather than checking if errno
has been inadvertently changed during intermediate code between when the
error was detected and when the message is generated.  Furthermore, this
is inconsistent with the existing error_setg_errno().

Thus, I do not think we want this function.  Your addition in 5/7 is
better, but a rather verbose name, so I think THAT addition deserves to
be named error_vreport_errno.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 5/7] error reporting: Provide error_report_errnoval (and error_vreport_errnoval)

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

On 04/26/2018 11:53 AM, Ian Jackson wrote:
> This will let us replace more open coded calls to error_report and
> strerror.
> 
> I have chosen to provide all of
>    error_report_errno       error_vreport_errno
>    error_report_errnoval    error_vreport_errnoval
> because the former are much more common, and deserve a short spelling;
> whereas there are still at least 30-40 potential callers of the latter.

As mentioned in 2/7, that's inconsistent with error_setg_errno().  I'd
MUCH rather see us have JUST error_[v]report_errno with a mandatory
error parameter, rather than blindly relying on implicit use of errno;
it's not that much harder to write:

error_report_errno(errno, "fmt string...", args);

in the common case when directly using errno is intended.

>  
> +void error_vreport_errnoval(int errnoval,
> +                            const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
> +void error_report_errnoval(int errnoval,
> +                           const char *fmt, ...) GCC_FMT_ATTR(2, 3);

Bikeshedding - can we name the parameter 'os_error' (as in
error_seg_errno), or 'err' or 'errval', rather than the longer 'errnoval'?


> +++ b/util/qemu-error.c
> @@ -257,6 +257,18 @@ void error_vreport_errno(const char *fmt, va_list ap)
>  }
>  
>  /*
> + * Print an error message to current monitor if we have one, else to stderr.
> + * Format arguments like vsprintf().  The resulting message should be
> + * a single phrase, with no newline or trailing punctuation.
> + * Prepend the current location and append ": " strerror(errnoval) "\n".
> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> + */
> +void error_vreport_errnoval(int errnoval, const char *fmt, va_list ap)
> +{
> +    vreport(REPORT_TYPE_ERROR, errnoval, fmt, ap);
> +}

Should this explicitly document that passing 0 for errnoval is
acceptable or forbidden?  If acceptable, does that mean no suffix (other
than \n) is added?  If forbidden, do we want to assert() that?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport

[Ian Jackson]

Eric Blake writes ("Re: [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport"):
> On 04/26/2018 11:53 AM, Ian Jackson wrote:
> > We use strerror rather than strerror_r because strerror_r presents
> > portability difficulties.  Replacing strerror with strerror_r (or
> > something else) is left to the future.
> 
> Is g_strerror() suitably easy to use, which would at least avoid the
> portability difficulties?

Possibly.  I would prefer to leave that to a future cleanup so as not
to block the centralisation of the strerror calls...

> >   * Print a message to current monitor if we have one, else to stderr.
> >   * @report_type is the type of message: error, warning or informational.
> > + * If @errnoval is nonnegative it is fed to strerror and printed too.
> 
> That implies 0 is fed to strerror(), which is not the case.

But, 0 might be fed to strerror.  This is not normally deliberate, but
it does occor.  It is not unusual for people to write code which can
feed 0 to strerror.  strerror then conventionally returns "Error 0".

This is why I chose -1 as the sentinel value meaning "there is no
errno being passed".

> "If @errnoval is not zero, its absolute value is fed to strerror" to let
> people pass in both EIO and -EIO for the same output (something that
> strerror() can't do, but our wrapper can) - but I don't know if that
> convenience is a good thing.

I think it is more important not to turn inintended situations where 0
would previously have been passed to strerror, into situations where
the errno value string simply vanishes.

> > -static void vreport(report_type type, const char *fmt, va_list ap)
> > +static void vreport(report_type type, int errnoval,
> 
> Bikeshedding: Is 'err' or 'errval' a better name in terms of length and
> legibility?

`err' could be some kind of qemu-specific or library-specific error
code.  `errno' is the name for the specific error code space for Unix
errno.

> > +    if (errnoval >= 0) {
> > +        error_printf(": %s", strerror(errnoval));
> 
> Off-by-one.  You do NOT want to print strerror(0) (that results in
> appending ": Success" or similar to what is supposed to be an error
> message).

See above.

> > -    vreport(REPORT_TYPE_ERROR, fmt, ap);
> > +    vreport(REPORT_TYPE_ERROR, -1, fmt, ap);
> 
> Passing -1 to suppress the output feels awkward, especially if 0 can be
> used for the same purpose and would then let us use -errno and errno
> interchangeable by passing the absolute value to sterror.

I think no good can come of taking the absolute value.  The confusion
resulting from -errnoval is bad enough already IMO.

Regards,
Ian.

Re: [Qemu-devel] [RFC PATCH 3/7] error reporting: Use error_report_errno in obvious places

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1874 bytes --]

On 04/26/2018 11:53 AM, Ian Jackson wrote:
> This patch is the result of
> 
>   git-grep -l 'error_report.*strerror' | xargs perl -p -i~ ../t
> 
> with ../t containing
> 
>   s{error_report\("(.*): \%s"(, .*)?, strerror\(errno\)\)\;}{error_report_errno\("$1"$2)\;}
> 
> Since this is an automatically generated patch, it does not contain
> any cleanups of the occasional idiosyncratic messages.  That is left
> to the future.
> 
> No functional change, since error_report_errno does exactly what this
> previous open-coded pattern does.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  audio/wavcapture.c       |  2 +-
>  hw/i386/xen/xen-hvm.c    |  2 +-
>  hw/tpm/tpm_emulator.c    |  2 +-
>  hw/xen/xen_pt_load_rom.c |  4 ++--
>  migration/postcopy-ram.c | 12 ++++++------
>  net/tap-linux.c          |  2 +-
>  os-posix.c               |  8 ++++----
>  qemu-nbd.c               |  4 ++--
>  scsi/qemu-pr-helper.c    |  4 ++--
>  slirp/misc.c             |  4 ++--
>  util/osdep.c             |  2 +-
>  vl.c                     |  6 +++---
>  12 files changed, 26 insertions(+), 26 deletions(-)

See my other emails about whether this should be
error_report_errno(errno, "...").

Misses a lot of two-line instances, such as:
$ git grep -A1 error_report | grep -C1 strerror.errno
...
block/file-posix.c:            error_report("Failed to restore old file
length:
%s",
block/file-posix.c-                         strerror(errno));
...

If we're going to clean up these instances, we might as well look harder
for them.  Can Coccinelle be coaxed into helping us (at least for
identifying callsites, even if I can't figure out how to make it
slice-and-dice format strings)?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 5/7] error reporting: Provide error_report_errnoval (and error_vreport_errnoval)

[Ian Jackson]

Eric Blake writes ("Re: [RFC PATCH 5/7] error reporting: Provide error_report_errnoval (and error_vreport_errnoval)"):
> On 04/26/2018 11:53 AM, Ian Jackson wrote:
> > I have chosen to provide all of
> >    error_report_errno       error_vreport_errno
> >    error_report_errnoval    error_vreport_errnoval
> > because the former are much more common, and deserve a short spelling;
> > whereas there are still at least 30-40 potential callers of the latter.
> 
> As mentioned in 2/7, that's inconsistent with error_setg_errno().

I didn't see error_setg_errno().  But I don't mind your alternative
approach, with the explicit errno, if that's what people prefer.  I
agree about the names of the functions, in that case.

> > +void error_vreport_errnoval(int errnoval,
> > +                            const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
> > +void error_report_errnoval(int errnoval,
> > +                           const char *fmt, ...) GCC_FMT_ATTR(2, 3);
> 
> Bikeshedding - can we name the parameter 'os_error' (as in
> error_seg_errno), or 'err' or 'errval', rather than the longer 'errnoval'?

In speech, one always refers to these things as `errno values'.
`os_error' and `errnoval' are the same length of course.  See my other
mail about this particular bikeshed.

> Should this explicitly document that passing 0 for errnoval is
> acceptable or forbidden?  If acceptable, does that mean no suffix (other
> than \n) is added?  If forbidden, do we want to assert() that?

My intend was that passing 0 for errnoval is exactly as permitted or
forbidden as passing 0 to strerror.  I think that this is neatly
implied by the specification:

> > + * Prepend the current location and append ": " strerror(errnoval) "\n".

These functions already have a problem with excessive boilerplate in
their language.  I'm loathe to make that worse by being other than
terse.  I wouldn't mind a note somewhere else.

I would be open to asserting that the value is not negative, because
we do have some of the confusing -errno convention in some of qemu.
OTOH that turns a mishandling of -errno on an error path from a wrong
error message into a crash which is a bit unfortunate.

Ian.

Re: [Qemu-devel] [RFC PATCH 3/7] error reporting: Use error_report_errno in obvious places

[Ian Jackson]

Eric Blake writes ("Re: [RFC PATCH 3/7] error reporting: Use error_report_errno in obvious places"):
> Misses a lot of two-line instances, such as:
> $ git grep -A1 error_report | grep -C1 strerror.errno
> ...
...
> If we're going to clean up these instances, we might as well look harder
> for them.  Can Coccinelle be coaxed into helping us (at least for
> identifying callsites, even if I can't figure out how to make it
> slice-and-dice format strings)?

I have never used Coccinelle, so I don't know.

Ian.

Re: [Qemu-devel] [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]

On 04/26/2018 12:32 PM, Ian Jackson wrote:

>>>   * Print a message to current monitor if we have one, else to stderr.
>>>   * @report_type is the type of message: error, warning or informational.
>>> + * If @errnoval is nonnegative it is fed to strerror and printed too.
>>
>> That implies 0 is fed to strerror(), which is not the case.
> 
> But, 0 might be fed to strerror.  This is not normally deliberate, but
> it does occor.  It is not unusual for people to write code which can
> feed 0 to strerror.  strerror then conventionally returns "Error 0".

No, POSIX requires:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strerror.html
"if the value of errnum is zero, the message string shall either be an
empty string or indicate that no error occurred;"

and at least in glibc, strerror(0) returns "Success", which looks dumb
in an error message.

> 
> This is why I chose -1 as the sentinel value meaning "there is no
> errno being passed".
> 
>> "If @errnoval is not zero, its absolute value is fed to strerror" to let
>> people pass in both EIO and -EIO for the same output (something that
>> strerror() can't do, but our wrapper can) - but I don't know if that
>> convenience is a good thing.
> 
> I think it is more important not to turn inintended situations where 0
> would previously have been passed to strerror, into situations where
> the errno value string simply vanishes.

Passing 0 as an errno value to indicate an error is almost always wrong.
But if you don't have an errno value to report, having the error string
disappear is preferable to outputting garbage or even assert()ing that
the error value was nonzero.

>>
>> Passing -1 to suppress the output feels awkward, especially if 0 can be
>> used for the same purpose and would then let us use -errno and errno
>> interchangeable by passing the absolute value to sterror.
> 
> I think no good can come of taking the absolute value.  The confusion
> resulting from -errnoval is bad enough already IMO.

You are correct that in the past, we've had code passing or returning
the wrong sign without realizing it.  So the question is whether, at
least for reporting purposes, it looks better to report "Operation not
permitted" instead of "Unknown error -1", or whether munging the error
message as a convenience for better output makes it harder for the
programmer to realize that they are returning the wrong sign up the
stack to the caller.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 7/7] error reporting: HACKING: Say to use error_report_errno

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

On 04/26/2018 11:53 AM, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  HACKING | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/HACKING b/HACKING
> index 4125c97..95563a3 100644
> --- a/HACKING
> +++ b/HACKING
> @@ -189,6 +189,7 @@ error_report() or error_vreport() from error-report.h.  This ensures the
>  error is reported in the right place (current monitor or stderr), and in
>  a uniform format.
>  
> +Use error_report_errno rather than open-coding calls to strerror().
>  Use error_printf() & friends to print additional information.

Our style is to write this as error_report_errno().  Or even copy the
approach used in the next line, stating 'error_report_errno() & friends'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 3/7] error reporting: Use error_report_errno in obvious places

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2222 bytes --]

On 04/26/2018 12:43 PM, Ian Jackson wrote:
> Eric Blake writes ("Re: [RFC PATCH 3/7] error reporting: Use error_report_errno in obvious places"):
>> Misses a lot of two-line instances, such as:
>> $ git grep -A1 error_report | grep -C1 strerror.errno
>> ...
> ...
>> If we're going to clean up these instances, we might as well look harder
>> for them.  Can Coccinelle be coaxed into helping us (at least for
>> identifying callsites, even if I can't figure out how to make it
>> slice-and-dice format strings)?
> 
> I have never used Coccinelle, so I don't know.

It was quite easy to detect spots that would benefit from this pattern;
I'm less certain about getting Coccinelle to rewrite them, but at least
knowing where they are makes it easier to ensure you aren't missing
obvious candidates:

$ cat error_report.cocci
@@
expression E;
@@
* error_report(..., strerror(E))

$ spatch --sp-file error_report.cocci \
  --macro-file scripts/cocci-macro-file.h --dir . 2>/dev/null \
  grep -c error_report
149

Between patch 2 and 5, I thus see 149 instances of a call to strerror()
embedded as the last parameter to error_report(), which is more than you
found.

As an example, the tail of the Coccinelle output shows the one in
block/file-posix.c that I demonstrated that you missed, plus the one in
util/osdep.c that your simpler perl script caught:

...
--- ./block/file-posix.c
+++ /tmp/nothing/block/file-posix.c
@@ -1762,8 +1762,6 @@ static int raw_regular_truncate(int fd,
 out:
     if (result < 0) {
         if (ftruncate(fd, current_length) < 0) {
-            error_report("Failed to restore old file length: %s",
-                         strerror(errno));
         }
     }

diff -u -p ./util/osdep.c /tmp/nothing/util/osdep.c
--- ./util/osdep.c
+++ /tmp/nothing/util/osdep.c
@@ -89,7 +89,6 @@ static int qemu_mprotect__osdep(void *ad
     return 0;
 #else
     if (mprotect(addr, size, prot)) {
-        error_report("%s: mprotect failed: %s", __func__, strerror(errno));
         return -1;
     }
     return 0;


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

On 04/26/2018 12:24 PM, Eric Blake wrote:

>> @@ -222,6 +224,11 @@ static void vreport(report_type type, const char *fmt, va_list ap)
>>      }
>>  
>>      error_vprintf(fmt, ap);
>> +
>> +    if (errnoval >= 0) {
>> +        error_printf(": %s", strerror(errnoval));
> 
> Off-by-one.  You do NOT want to print strerror(0) (that results in
> appending ": Success" or similar to what is supposed to be an error
> message).

> 
> Passing -1 to suppress the output feels awkward, especially if 0 can be
> used for the same purpose and would then let us use -errno and errno
> interchangeable by passing the absolute value to sterror.
> 

I'm also using, as my reference, the glibc 'man 3 error' function, which
specifically documents:

  if  errnum  is  nonzero,  a second colon and a space followed by the
string
       given by strerror(errnum).

making it a very versatile function for outputting messages to stderr
whether or not you also have an errno value to convert via strerror(),
by using 0 (not -1) as the sentinel value.

Consistency argues that even if we are going to invent our own qemu
functions, we're better off sticking to well-known paradigms already in
the wild (such as 0 to suppress appending strerror()) rather than making
readers learn yet another paradigm.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport

[Ian Jackson]

Eric Blake writes ("Re: [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport"):
> On 04/26/2018 12:32 PM, Ian Jackson wrote:
> > But, 0 might be fed to strerror.  This is not normally deliberate, but
> > it does occor.  It is not unusual for people to write code which can
> > feed 0 to strerror.  strerror then conventionally returns "Error 0".
> 
> No, POSIX requires:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/strerror.html
> "if the value of errnum is zero, the message string shall either be an
> empty string or indicate that no error occurred;"
> 
> and at least in glibc, strerror(0) returns "Success", which looks dumb
> in an error message.

I agree that it looks dumb.  About as dumb as "Error 0".

> > I think it is more important not to turn inintended situations where 0
> > would previously have been passed to strerror, into situations where
> > the errno value string simply vanishes.
> 
> Passing 0 as an errno value to indicate an error is almost always wrong.
> But if you don't have an errno value to report, having the error string
> disappear is preferable to outputting garbage or even assert()ing that
> the error value was nonzero.

"Success" is not garbage.

> You are correct that in the past, we've had code passing or returning
> the wrong sign without realizing it.  So the question is whether, at
> least for reporting purposes, it looks better to report "Operation not
> permitted" instead of "Unknown error -1", or whether munging the error
> message as a convenience for better output makes it harder for the
> programmer to realize that they are returning the wrong sign up the
> stack to the caller.

I think in both the above bug cases it is preferable to continue to
print an errno value message, and to print one which indicates that
there is a bug in the program's error handling.

In ordinary code it is not common to want to print an error message
with an error code which is an augmented errno value type containing a
sentinel.  Using 0 as a sentinel for this purpose would mostly
disguise bugs rather than be a convenience.

If you disapprove of the use of -1 as the internal sentinel because of
the possibility of -errno confusion bugs, we could use INT_MIN as the
sentinel.

Ian.