[PATCH 00/14] Error message improvements

[PATCH 00/14] Error message improvements

[Markus Armbruster]

Markus Armbruster (14):
  error: Strip trailing '\n' from error string arguments (again)
  hw/usb: Use error_setg_file_open() for a better error message
  tap-solaris: Use error_setg_file_open() for better error messages
  qga: Use error_setg_file_open() for better error messages
  hw/scsi: Use error_setg_file_open() for a better error message
  hw/virtio: Use error_setg_file_open() for a better error message
  net/tap: Use error_setg_file_open() for a better error message
  blkdebug: Use error_setg_file_open() for a better error message
  error: Use error_setg_file_open() for simplicity and consistency
  net/slirp: Improve file open error message
  error: Use error_setg_errno() to improve error messages
  error: Use error_setg_errno() for simplicity and consistency
  qga/commands-win32: Use error_setg_win32() for better error messages
  block/file-win32: Improve an error message

 backends/cryptodev-lkcf.c   |  2 +-
 backends/spdm-socket.c      |  4 ++--
 backends/tpm/tpm_emulator.c | 13 +++++--------
 block/blkdebug.c            |  2 +-
 block/file-win32.c          |  2 +-
 hw/9pfs/9p-local.c          |  2 +-
 hw/9pfs/9p.c                |  3 +--
 hw/acpi/core.c              |  5 ++---
 hw/audio/es1370.c           |  2 +-
 hw/core/loader.c            |  2 +-
 hw/intc/openpic_kvm.c       |  3 +--
 hw/intc/xics_kvm.c          |  5 +++--
 hw/pci-host/xen_igd_pt.c    |  2 +-
 hw/ppc/spapr.c              |  6 +++---
 hw/remote/vfio-user-obj.c   | 18 +++++++++---------
 hw/scsi/vhost-scsi.c        |  3 +--
 hw/sensor/emc141x.c         |  4 ++--
 hw/sensor/tmp421.c          |  4 ++--
 hw/smbios/smbios.c          |  4 ++--
 hw/usb/bus.c                |  2 +-
 hw/vfio/migration-multifd.c |  5 +++--
 hw/virtio/vdpa-dev.c        |  4 ++--
 hw/virtio/vhost-vsock.c     |  3 +--
 migration/postcopy-ram.c    | 10 +++++-----
 migration/rdma.c            |  3 +--
 monitor/hmp-cmds-target.c   |  2 +-
 net/dump.c                  |  2 +-
 net/l2tpv3.c                |  6 ++----
 net/slirp.c                 |  9 ++++++---
 net/tap-bsd.c               |  6 +++---
 net/tap-linux.c             |  2 +-
 net/tap-solaris.c           |  6 +++---
 net/tap.c                   |  3 +--
 qga/commands-linux.c        | 11 ++++++-----
 qga/commands-posix-ssh.c    | 23 +++++++++++++----------
 qga/commands-win32.c        | 16 ++++++++--------
 system/vl.c                 |  2 +-
 target/i386/sev.c           |  6 ++----
 target/ppc/kvm.c            |  5 ++---
 target/riscv/kvm/kvm-cpu.c  | 11 ++++++-----
 ui/gtk.c                    |  2 +-
 ui/ui-qmp-cmds.c            |  3 +--
 util/vfio-helpers.c         |  5 ++---
 43 files changed, 113 insertions(+), 120 deletions(-)

-- 
2.49.0

[PATCH 01/14] error: Strip trailing '\n' from error string arguments (again)

[Markus Armbruster]

Tracked down with scripts/coccinelle/err-bad-newline.cocci.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/audio/es1370.c | 2 +-
 ui/gtk.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c
index 9873ffadab..566f93f1ea 100644
--- a/hw/audio/es1370.c
+++ b/hw/audio/es1370.c
@@ -228,7 +228,7 @@ static void print_sctl(uint32_t val)
 #undef a
         error_report("es1370: "
                 "%s p2_end_inc %d, p2_st_inc %d,"
-                " r1_fmt %s, p2_fmt %s, p1_fmt %s\n",
+                " r1_fmt %s, p2_fmt %s, p1_fmt %s",
                 buf,
                 (val & SCTRL_P2ENDINC) >> SCTRL_SH_P2ENDINC,
                 (val & SCTRL_P2STINC) >> SCTRL_SH_P2STINC,
diff --git a/ui/gtk.c b/ui/gtk.c
index 48571bedbf..e83a366625 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1197,7 +1197,7 @@ static gboolean gd_touch_event(GtkWidget *widget, GdkEventTouch *touch,
         type = INPUT_MULTI_TOUCH_TYPE_END;
         break;
     default:
-        warn_report("gtk: unexpected touch event type\n");
+        warn_report("gtk: unexpected touch event type");
         return FALSE;
     }
 
-- 
2.49.0

[PATCH 02/14] hw/usb: Use error_setg_file_open() for a better error message

[Markus Armbruster]

The error message changes from

    open FILENAME failed

to

    Could not open 'FILENAME': REASON

where REASON is the value of strerror(errno).

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 8dd2ce415e..47d42ca3c1 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -262,7 +262,7 @@ static void usb_qdev_realize(DeviceState *qdev, Error **errp)
         int fd = qemu_open_old(dev->pcap_filename,
                                O_CREAT | O_WRONLY | O_TRUNC | O_BINARY, 0666);
         if (fd < 0) {
-            error_setg(errp, "open %s failed", dev->pcap_filename);
+            error_setg_file_open(errp, errno, dev->pcap_filename);
             usb_qdev_unrealize(qdev);
             return;
         }
-- 
2.49.0

[PATCH 03/14] tap-solaris: Use error_setg_file_open() for better error messages

[Markus Armbruster]

Error messages change from

    Can't open /dev/ip (actually /dev/udp)
    Can't open /dev/tap
    Can't open /dev/tap (2)

to

    Could not open '/dev/udp': REASON
    Could not open '/dev/tap': REASON

where REASON is the value of strerror(errno).

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap-solaris.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/tap-solaris.c b/net/tap-solaris.c
index 75397e6c54..faf7922ea8 100644
--- a/net/tap-solaris.c
+++ b/net/tap-solaris.c
@@ -87,13 +87,13 @@ static int tap_alloc(char *dev, size_t dev_size, Error **errp)
 
     ip_fd = RETRY_ON_EINTR(open("/dev/udp", O_RDWR, 0));
     if (ip_fd < 0) {
-        error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
+        error_setg_file_open(errp, errno, "/dev/udp");
         return -1;
     }
 
     tap_fd = RETRY_ON_EINTR(open("/dev/tap", O_RDWR, 0));
     if (tap_fd < 0) {
-        error_setg(errp, "Can't open /dev/tap");
+        error_setg_file_open(errp, errno, "/dev/tap");
         return -1;
     }
 
@@ -107,7 +107,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error **errp)
 
     if_fd = RETRY_ON_EINTR(open("/dev/tap", O_RDWR, 0));
     if (if_fd < 0) {
-        error_setg(errp, "Can't open /dev/tap (2)");
+        error_setg_file_open(errp, errno, "/dev/tap");
         return -1;
     }
     if(ioctl(if_fd, I_PUSH, "ip") < 0){
-- 
2.49.0

[PATCH 04/14] qga: Use error_setg_file_open() for better error messages

[Markus Armbruster]

Error messages change from

    open("FNAME"): REASON

to

    Could not open 'FNAME': REASON

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-linux.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/qga/commands-linux.c b/qga/commands-linux.c
index 4a09ddc760..5cf76ca2d9 100644
--- a/qga/commands-linux.c
+++ b/qga/commands-linux.c
@@ -1502,14 +1502,15 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
 
     dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
     if (dirfd == -1) {
-        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+        error_setg_file_open(errp, errno, dirpath);
         return;
     }
 
     fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
     if (fd == -1) {
         if (errno != ENOENT) {
-            error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
+            error_setg_errno(errp, errno, "could not open %s/%s",
+                             dirpath, fn);
         } else if (sys2vcpu) {
             vcpu->online = true;
             vcpu->can_offline = false;
@@ -1711,7 +1712,7 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
     dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
     if (dirfd == -1) {
         if (sys2memblk) {
-            error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
+            error_setg_file_open(errp, errno, dirpath);
         } else {
             if (errno == ENOENT) {
                 result->response = GUEST_MEMORY_BLOCK_RESPONSE_TYPE_NOT_FOUND;
@@ -1936,7 +1937,7 @@ static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
 
     fp = fopen(diskstats, "r");
     if (fp  == NULL) {
-        error_setg_errno(errp, errno, "open(\"%s\")", diskstats);
+        error_setg_file_open(errp, errno, diskstats);
         return NULL;
     }
 
@@ -2047,7 +2048,7 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
 
     fp = fopen(cpustats, "r");
     if (fp  == NULL) {
-        error_setg_errno(errp, errno, "open(\"%s\")", cpustats);
+        error_setg_file_open(errp, errno, cpustats);
         return NULL;
     }
 
-- 
2.49.0

[PATCH 05/14] hw/scsi: Use error_setg_file_open() for a better error message

[Markus Armbruster]

The error message changes from

    vhost-scsi: open vhost char device failed: REASON

to

    Could not open '/dev/vhost-scsi': REASON

I think the exact file name is more useful to know than the file's
purpose.

We could put back the "vhost-scsi: " prefix with error_prepend().  Not
worth the bother.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi/vhost-scsi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index cdf405b0f8..239138c931 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -245,8 +245,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
     } else {
         vhostfd = open("/dev/vhost-scsi", O_RDWR);
         if (vhostfd < 0) {
-            error_setg(errp, "vhost-scsi: open vhost char device failed: %s",
-                       strerror(errno));
+            error_setg_file_open(errp, errno, "/dev/vhost-scsi");
             return;
         }
     }
-- 
2.49.0

[PATCH 06/14] hw/virtio: Use error_setg_file_open() for a better error message

[Markus Armbruster]

The error message changes from

    vhost-vsock: failed to open vhost device: REASON

to

    Could not open '/dev/vhost-vsock': REASON

I think the exact file name is more useful to know than the file's
purpose.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio/vhost-vsock.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 107d88babe..7940b60d8a 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -153,8 +153,7 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
     } else {
         vhostfd = open("/dev/vhost-vsock", O_RDWR);
         if (vhostfd < 0) {
-            error_setg_errno(errp, errno,
-                             "vhost-vsock: failed to open vhost device");
+            error_setg_file_open(errp, errno, "/dev/vhost-vsock");
             return;
         }
 
-- 
2.49.0

[PATCH 07/14] net/tap: Use error_setg_file_open() for a better error message

[Markus Armbruster]

The error message changes from

    tap: open vhost char device failed

to

    Could not open '/dev/vhost-net': REASON

I think the exact file name is more useful to know than the file's
purpose.

We could put back the "tap: " prefix with error_prepend().  Not
worth the bother.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/tap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/tap.c b/net/tap.c
index abe3b2d036..bfba3fd7a7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -747,8 +747,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
         } else {
             vhostfd = open("/dev/vhost-net", O_RDWR);
             if (vhostfd < 0) {
-                error_setg_errno(errp, errno,
-                                 "tap: open vhost char device failed");
+                error_setg_file_open(errp, errno, "/dev/vhost-net");
                 goto failed;
             }
             if (!qemu_set_blocking(vhostfd, false, errp)) {
-- 
2.49.0

[PATCH 08/14] blkdebug: Use error_setg_file_open() for a better error message

[Markus Armbruster]

The error message changes from

    Could not read blkdebug config file: REASON

to

    Could not open 'FNAME': REASON

I think the exact file name is more useful to know than the file's
purpose.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/blkdebug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index c54aee0c84..8a4a8cb85e 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -288,7 +288,7 @@ static int read_config(BDRVBlkdebugState *s, const char *filename,
     if (filename) {
         f = fopen(filename, "r");
         if (f == NULL) {
-            error_setg_errno(errp, errno, "Could not read blkdebug config file");
+            error_setg_file_open(errp, errno, filename);
             return -errno;
         }
 
-- 
2.49.0

[PATCH 09/14] error: Use error_setg_file_open() for simplicity and consistency

[Markus Armbruster]

Replace

    error_setg_errno(errp, errno, MSG, FNAME);

by

    error_setg_file_open(errp, errno, FNAME);

where MSG is "Could not open '%s'" or similar.

Also replace equivalent uses of error_setg().

A few messages lose prefixes ("net dump: ", "SEV: ", __func__ ": ").
We could put them back with error_prepend().  Not worth the bother.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/9pfs/9p-local.c        | 2 +-
 hw/acpi/core.c            | 2 +-
 hw/core/loader.c          | 2 +-
 hw/pci-host/xen_igd_pt.c  | 2 +-
 monitor/hmp-cmds-target.c | 2 +-
 net/dump.c                | 2 +-
 net/tap-bsd.c             | 6 +++---
 net/tap-linux.c           | 2 +-
 target/i386/sev.c         | 6 ++----
 ui/ui-qmp-cmds.c          | 3 +--
 util/vfio-helpers.c       | 5 ++---
 11 files changed, 15 insertions(+), 19 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 31e216227c..376b377698 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1456,7 +1456,7 @@ static int local_init(FsContext *ctx, Error **errp)
 
     data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
     if (data->mountfd == -1) {
-        error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
+        error_setg_file_open(errp, errno, ctx->fs_root);
         goto err;
     }
 
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index ff16582803..d2677332af 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -277,7 +277,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
         int fd = open(*cur, O_RDONLY | O_BINARY);
 
         if (fd < 0) {
-            error_setg(errp, "can't open file %s: %s", *cur, strerror(errno));
+            error_setg_file_open(errp, errno, *cur);
             goto out;
         }
 
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 590c5b02aa..b56e5eb2f5 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -379,7 +379,7 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
 
     fd = open(filename, O_RDONLY | O_BINARY);
     if (fd < 0) {
-        error_setg_errno(errp, errno, "Failed to open file: %s", filename);
+        error_setg_file_open(errp, errno, filename);
         return;
     }
     if (read(fd, hdr, EI_NIDENT) != EI_NIDENT) {
diff --git a/hw/pci-host/xen_igd_pt.c b/hw/pci-host/xen_igd_pt.c
index 5dd17ef236..f6016f2cd5 100644
--- a/hw/pci-host/xen_igd_pt.c
+++ b/hw/pci-host/xen_igd_pt.c
@@ -55,7 +55,7 @@ static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
 
     config_fd = open(path, O_RDWR);
     if (config_fd < 0) {
-        error_setg_errno(errp, errno, "Failed to open: %s", path);
+        error_setg_file_open(errp, errno, path);
         goto out;
     }
 
diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
index e982061146..ad4ed2167d 100644
--- a/monitor/hmp-cmds-target.c
+++ b/monitor/hmp-cmds-target.c
@@ -331,7 +331,7 @@ static uint64_t vtop(void *ptr, Error **errp)
 
     fd = open("/proc/self/pagemap", O_RDONLY);
     if (fd == -1) {
-        error_setg_errno(errp, errno, "Cannot open /proc/self/pagemap");
+        error_setg_file_open(errp, errno, "/proc/self/pagemap");
         return -1;
     }
 
diff --git a/net/dump.c b/net/dump.c
index 581234b775..0c39f09892 100644
--- a/net/dump.c
+++ b/net/dump.c
@@ -111,7 +111,7 @@ static int net_dump_state_init(DumpState *s, const char *filename,
 
     fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY | O_BINARY, 0644);
     if (fd < 0) {
-        error_setg_errno(errp, errno, "net dump: can't open %s", filename);
+        error_setg_file_open(errp, errno, filename);
         return -1;
     }
 
diff --git a/net/tap-bsd.c b/net/tap-bsd.c
index bbf84d1828..3fd300d46f 100644
--- a/net/tap-bsd.c
+++ b/net/tap-bsd.c
@@ -68,7 +68,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         }
     }
     if (fd < 0) {
-        error_setg_errno(errp, errno, "could not open %s", dname);
+        error_setg_file_open(errp, errno, dname);
         return -1;
     }
 
@@ -118,7 +118,7 @@ static int tap_open_clone(char *ifname, int ifname_size, Error **errp)
 
     fd = RETRY_ON_EINTR(open(PATH_NET_TAP, O_RDWR));
     if (fd < 0) {
-        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TAP);
+        error_setg_file_open(errp, errno, PATH_NET_TAP);
         return -1;
     }
 
@@ -166,7 +166,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
         snprintf(dname, sizeof dname, "/dev/%s", ifname);
         fd = RETRY_ON_EINTR(open(dname, O_RDWR));
         if (fd < 0 && errno != ENOENT) {
-            error_setg_errno(errp, errno, "could not open %s", dname);
+            error_setg_file_open(errp, errno, dname);
             return -1;
         }
     }
diff --git a/net/tap-linux.c b/net/tap-linux.c
index 2a90b58467..909c4f1fcf 100644
--- a/net/tap-linux.c
+++ b/net/tap-linux.c
@@ -57,7 +57,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
     if (fd < 0) {
         fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
         if (fd < 0) {
-            error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
+            error_setg_file_open(errp, errno, PATH_NET_TUN);
             return -1;
         }
     }
diff --git a/target/i386/sev.c b/target/i386/sev.c
index fd2dada013..8660ecd9e4 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -891,8 +891,7 @@ static SevCapability *sev_get_capabilities(Error **errp)
 
     fd = open(sev_device, O_RDWR);
     if (fd < 0) {
-        error_setg_errno(errp, errno, "SEV: Failed to open %s",
-                         sev_device);
+        error_setg_file_open(errp, errno, sev_device);
         g_free(sev_device);
         return NULL;
     }
@@ -1819,8 +1818,7 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
     devname = object_property_get_str(OBJECT(sev_common), "sev-device", NULL);
     sev_common->sev_fd = open(devname, O_RDWR);
     if (sev_common->sev_fd < 0) {
-        error_setg(errp, "%s: Failed to open %s '%s'", __func__,
-                   devname, strerror(errno));
+        error_setg_file_open(errp, errno, devname);
         g_free(devname);
         return -1;
     }
diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
index 74fa6c6ec5..d927121676 100644
--- a/ui/ui-qmp-cmds.c
+++ b/ui/ui-qmp-cmds.c
@@ -371,8 +371,7 @@ qmp_screendump(const char *filename, const char *device,
 
     fd = qemu_open_old(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666);
     if (fd == -1) {
-        error_setg(errp, "failed to open file '%s': %s", filename,
-                   strerror(errno));
+        error_setg_file_open(errp, errno, filename);
         return;
     }
 
diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index fdff042ab4..8b1b2e2f05 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -309,7 +309,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
     s->container = open("/dev/vfio/vfio", O_RDWR);
 
     if (s->container == -1) {
-        error_setg_errno(errp, errno, "Failed to open /dev/vfio/vfio");
+        error_setg_file_open(errp, errno, "/dev/vfio/vfio");
         return -errno;
     }
     if (ioctl(s->container, VFIO_GET_API_VERSION) != VFIO_API_VERSION) {
@@ -333,8 +333,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
 
     s->group = open(group_file, O_RDWR);
     if (s->group == -1) {
-        error_setg_errno(errp, errno, "Failed to open VFIO group file: %s",
-                         group_file);
+        error_setg_file_open(errp, errno, group_file);
         g_free(group_file);
         ret = -errno;
         goto fail_container;
-- 
2.49.0

[PATCH 10/14] net/slirp: Improve file open error message

[Markus Armbruster]

This error reports failure to create a temporary file, and
error_setg_file_open() would probably be too terse, so merely switch
to error_setg_errno() to add errno information.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 net/slirp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/slirp.c b/net/slirp.c
index 120eef6122..5996fec512 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -1034,8 +1034,10 @@ static int slirp_smb(SlirpState* s, const char *exported_dir,
 
     f = fopen(smb_conf, "w");
     if (!f) {
+        int eno = errno;
+
         slirp_smb_cleanup(s);
-        error_setg(errp,
+        error_setg_errno(errp, eno,
                    "Could not create samba server configuration file '%s'",
                     smb_conf);
         g_free(smb_conf);
-- 
2.49.0

[PATCH 11/14] error: Use error_setg_errno() to improve error messages

[Markus Armbruster]

A few error messages show numeric errno codes.  Use error_setg_errno()
to show human-readable text instead.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 backends/cryptodev-lkcf.c   |  2 +-
 hw/ppc/spapr.c              |  6 +++---
 hw/vfio/migration-multifd.c |  5 +++--
 migration/rdma.c            |  3 +--
 net/l2tpv3.c                |  6 ++----
 target/riscv/kvm/kvm-cpu.c  | 11 ++++++-----
 6 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/backends/cryptodev-lkcf.c b/backends/cryptodev-lkcf.c
index 97a8a8812c..40c7bd3c5a 100644
--- a/backends/cryptodev-lkcf.c
+++ b/backends/cryptodev-lkcf.c
@@ -218,7 +218,7 @@ static void cryptodev_lkcf_init(CryptoDevBackend *backend, Error **errp)
     }
     lkcf->eventfd = eventfd(0, 0);
     if (lkcf->eventfd < 0) {
-        error_setg(errp, "Failed to create eventfd: %d", errno);
+        error_setg_errno(errp, errno, "Failed to create eventfd");
         return;
     }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 99b843ba2f..cdab822c88 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2699,9 +2699,9 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
         ret = kvmppc_set_smt_threads(spapr->vsmt);
         if (ret) {
             /* Looks like KVM isn't able to change VSMT mode */
-            error_setg(&local_err,
-                       "Failed to set KVM's VSMT mode to %d (errno %d)",
-                       spapr->vsmt, ret);
+            error_setg_errno(&local_err, -ret,
+                             "Failed to set KVM's VSMT mode to %d",
+                             spapr->vsmt);
             /* We can live with that if the default one is big enough
              * for the number of threads, and a submultiple of the one
              * we want.  In this case we'll waste some vcpu ids, but
diff --git a/hw/vfio/migration-multifd.c b/hw/vfio/migration-multifd.c
index e4785031a7..4a855f4e12 100644
--- a/hw/vfio/migration-multifd.c
+++ b/hw/vfio/migration-multifd.c
@@ -725,8 +725,9 @@ vfio_multifd_save_complete_precopy_thread(SaveCompletePrecopyThreadData *d,
         data_size = read(migration->data_fd, &packet->data,
                          migration->data_buffer_size);
         if (data_size < 0) {
-            error_setg(errp, "%s: reading state buffer %" PRIu32 " failed: %d",
-                       vbasedev->name, idx, errno);
+            error_setg_errno(errp, errno,
+                             "%s: reading state buffer %" PRIu32 " failed",
+                             vbasedev->name, idx);
             goto thread_exit;
         } else if (data_size == 0) {
             break;
diff --git a/migration/rdma.c b/migration/rdma.c
index 337b415889..ef4885ef5f 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2349,8 +2349,7 @@ static int qemu_get_cm_event_timeout(RDMAContext *rdma,
         error_setg(errp, "RDMA ERROR: poll cm event timeout");
         return -1;
     } else if (ret < 0) {
-        error_setg(errp, "RDMA ERROR: failed to poll cm event, errno=%i",
-                   errno);
+        error_setg_errno(errp, "RDMA ERROR: failed to poll cm event");
         return -1;
     } else if (poll_fd.revents & POLLIN) {
         if (rdma_get_cm_event(rdma->channel, cm_event) < 0) {
diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index cdfc641aa6..3044fa4608 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -639,13 +639,11 @@ int net_init_l2tpv3(const Netdev *netdev,
     }
     fd = socket(result->ai_family, result->ai_socktype, result->ai_protocol);
     if (fd == -1) {
-        fd = -errno;
-        error_setg(errp, "socket creation failed, errno = %d",
-                   -fd);
+        error_setg_errno(errp, errno, "socket creation failed");
         goto outerr;
     }
     if (bind(fd, (struct sockaddr *) result->ai_addr, result->ai_addrlen)) {
-        error_setg(errp, "could not bind socket err=%i", errno);
+        error_setg_errno(errp, errno, "could not bind socket");
         goto outerr;
     }
     if (!qemu_set_blocking(fd, false, errp)) {
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 47e672c7aa..c73f1cd5cf 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1996,8 +1996,8 @@ static bool kvm_cpu_realize(CPUState *cs, Error **errp)
     if (riscv_has_ext(&cpu->env, RVV)) {
         ret = prctl(PR_RISCV_V_SET_CONTROL, PR_RISCV_V_VSTATE_CTRL_ON);
         if (ret) {
-            error_setg(errp, "Error in prctl PR_RISCV_V_SET_CONTROL, code: %s",
-                       strerrorname_np(errno));
+            error_setg_errno(errp, errno,
+                             "Error in prctl PR_RISCV_V_SET_CONTROL");
             return false;
         }
     }
@@ -2032,7 +2032,8 @@ void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
         reg.addr = (uint64_t)&val;
         ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
         if (ret != 0) {
-            error_setg(errp, "Unable to read cbom_blocksize, error %d", errno);
+            error_setg(errp, errno,
+                       "Unable to read cbom_blocksize");
             return;
         }
 
@@ -2051,7 +2052,7 @@ void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
         reg.addr = (uint64_t)&val;
         ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
         if (ret != 0) {
-            error_setg(errp, "Unable to read cboz_blocksize, error %d", errno);
+            error_setg_errno(errp, errno, "Unable to read cboz_blocksize");
             return;
         }
 
@@ -2073,7 +2074,7 @@ void riscv_kvm_cpu_finalize_features(RISCVCPU *cpu, Error **errp)
         reg.addr = (uint64_t)&val;
         ret = ioctl(kvmcpu.cpufd, KVM_GET_ONE_REG, &reg);
         if (ret != 0) {
-            error_setg(errp, "Unable to read vlenb register, error %d", errno);
+            error_setg_errno(errp, errno, "Unable to read vlenb register");
             return;
         }
 
-- 
2.49.0

[PATCH 12/14] error: Use error_setg_errno() for simplicity and consistency

[Markus Armbruster]

Use error_setg_errno() instead of passing the value of strerror() or
g_strerror() to error_setg().

The separator between the error message proper and the value of
strerror() changes from " : ", "", " - ", "- " to ": " in places.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 backends/spdm-socket.c      |  4 ++--
 backends/tpm/tpm_emulator.c | 13 +++++--------
 hw/9pfs/9p.c                |  3 +--
 hw/acpi/core.c              |  3 +--
 hw/intc/openpic_kvm.c       |  3 +--
 hw/intc/xics_kvm.c          |  5 +++--
 hw/remote/vfio-user-obj.c   | 18 +++++++++---------
 hw/sensor/emc141x.c         |  4 ++--
 hw/sensor/tmp421.c          |  4 ++--
 hw/smbios/smbios.c          |  4 ++--
 hw/virtio/vdpa-dev.c        |  4 ++--
 migration/postcopy-ram.c    | 10 +++++-----
 migration/rdma.c            |  2 +-
 net/slirp.c                 |  5 +++--
 qga/commands-posix-ssh.c    | 23 +++++++++++++----------
 system/vl.c                 |  2 +-
 target/ppc/kvm.c            |  5 ++---
 17 files changed, 55 insertions(+), 57 deletions(-)

diff --git a/backends/spdm-socket.c b/backends/spdm-socket.c
index 6d8f02d3b9..a12bc47f77 100644
--- a/backends/spdm-socket.c
+++ b/backends/spdm-socket.c
@@ -167,7 +167,7 @@ int spdm_socket_connect(uint16_t port, Error **errp)
 
     client_socket = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
     if (client_socket < 0) {
-        error_setg(errp, "cannot create socket: %s", strerror(errno));
+        error_setg_errno(errp, errno, "cannot create socket");
         return -1;
     }
 
@@ -179,7 +179,7 @@ int spdm_socket_connect(uint16_t port, Error **errp)
 
     if (connect(client_socket, (struct sockaddr *)&server_addr,
                 sizeof(server_addr)) < 0) {
-        error_setg(errp, "cannot connect: %s", strerror(errno));
+        error_setg_errno(errp, errno, "cannot connect");
         close(client_socket);
         return -1;
     }
diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index f10b9074fb..f52cb4d435 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -225,8 +225,7 @@ static int tpm_emulator_set_locality(TPMEmulator *tpm_emu, uint8_t locty_number,
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_LOCALITY, &loc,
                              sizeof(loc), sizeof(loc.u.resp.tpm_result),
                              sizeof(loc)) < 0) {
-        error_setg(errp, "tpm-emulator: could not set locality : %s",
-                   strerror(errno));
+        error_setg_errno(errp, errno, "tpm-emulator: could not set locality");
         return -1;
     }
 
@@ -315,8 +314,7 @@ static int tpm_emulator_stop_tpm(TPMBackend *tb, Error **errp)
 
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_STOP, &res, 0,
                              sizeof(ptm_res), sizeof(res)) < 0) {
-        error_setg(errp, "tpm-emulator: Could not stop TPM: %s",
-                   strerror(errno));
+        error_setg_errno(errp, errno, "tpm-emulator: Could not stop TPM");
         return -1;
     }
 
@@ -377,8 +375,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_SET_BUFFERSIZE, &psbs,
                              sizeof(psbs.u.req), sizeof(psbs.u.resp.tpm_result),
                              sizeof(psbs.u.resp)) < 0) {
-        error_setg(errp, "tpm-emulator: Could not set buffer size: %s",
-                   strerror(errno));
+        error_setg_errno(errp, errno,
+                         "tpm-emulator: Could not set buffer size");
         return -1;
     }
 
@@ -426,8 +424,7 @@ static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
     if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, &init, sizeof(init),
                              sizeof(init.u.resp.tpm_result),
                              sizeof(init)) < 0) {
-        error_setg(errp, "tpm-emulator: could not send INIT: %s",
-                   strerror(errno));
+        error_setg_errno(errp, errno, "tpm-emulator: could not send INIT");
         goto err_exit;
     }
 
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index bc4a016ee3..6fbe604ce8 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -4345,8 +4345,7 @@ int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
      * use co-routines here.
      */
     if (s->ops->name_to_path(&s->ctx, NULL, "/", &path) < 0) {
-        error_setg(errp,
-                   "error in converting name to path %s", strerror(errno));
+        error_setg_errno(errp, errno, "error in converting name to path");
         goto out;
     }
     if (s->ops->lstat(&s->ctx, &path, &stat)) {
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index d2677332af..82974eb257 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -293,8 +293,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
                 memcpy(blob + bloblen, data, r);
                 bloblen += r;
             } else if (errno != EINTR) {
-                error_setg(errp, "can't read file %s: %s", *cur,
-                           strerror(errno));
+                error_setg_errno(errp, errno, "can't read file %s", *cur);
                 close(fd);
                 goto out;
             }
diff --git a/hw/intc/openpic_kvm.c b/hw/intc/openpic_kvm.c
index 673ea9ca05..0c11bbc963 100644
--- a/hw/intc/openpic_kvm.c
+++ b/hw/intc/openpic_kvm.c
@@ -223,8 +223,7 @@ static void kvm_openpic_realize(DeviceState *dev, Error **errp)
     cd.type = kvm_openpic_model;
     ret = kvm_vm_ioctl(s, KVM_CREATE_DEVICE, &cd);
     if (ret < 0) {
-        error_setg(errp, "Can't create device %d: %s",
-                   cd.type, strerror(errno));
+        error_setg_errno(errp, errno, "Can't create device %d", cd.type);
         return;
     }
     opp->fd = cd.fd;
diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index ee72969f5f..61f66d1019 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -165,8 +165,9 @@ void icp_kvm_realize(DeviceState *dev, Error **errp)
     if (ret < 0) {
         Error *local_err = NULL;
 
-        error_setg(&local_err, "Unable to connect CPU%ld to kernel XICS: %s",
-                   vcpu_id, strerror(errno));
+        error_setg_errno(&local_err, errno,
+                         "Unable to connect CPU%ld to kernel XICS",
+                         vcpu_id);
         if (errno == ENOSPC) {
             error_append_hint(&local_err, "Try -smp maxcpus=N with N < %u\n",
                               MACHINE(qdev_get_machine())->smp.max_cpus);
diff --git a/hw/remote/vfio-user-obj.c b/hw/remote/vfio-user-obj.c
index 216b4876e2..9ab02b7abc 100644
--- a/hw/remote/vfio-user-obj.c
+++ b/hw/remote/vfio-user-obj.c
@@ -751,7 +751,7 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
                                 LIBVFIO_USER_FLAG_ATTACH_NB,
                                 o, VFU_DEV_TYPE_PCI);
     if (o->vfu_ctx == NULL) {
-        error_setg(errp, "vfu: Failed to create context - %s", strerror(errno));
+        error_setg_errno(errp, errno, "vfu: Failed to create context");
         return;
     }
 
@@ -776,9 +776,9 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
 
     ret = vfu_pci_init(o->vfu_ctx, pci_type, PCI_HEADER_TYPE_NORMAL, 0);
     if (ret < 0) {
-        error_setg(errp,
-                   "vfu: Failed to attach PCI device %s to context - %s",
-                   o->device, strerror(errno));
+        error_setg_errno(errp, errno,
+                         "vfu: Failed to attach PCI device %s to context",
+                         o->device);
         goto fail;
     }
 
@@ -792,9 +792,9 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
                            VFU_REGION_FLAG_RW | VFU_REGION_FLAG_ALWAYS_CB,
                            NULL, 0, -1, 0);
     if (ret < 0) {
-        error_setg(errp,
-                   "vfu: Failed to setup config space handlers for %s- %s",
-                   o->device, strerror(errno));
+        error_setg_errno(errp,
+                         "vfu: Failed to setup config space handlers for %s",
+                         o->device);
         goto fail;
     }
 
@@ -822,8 +822,8 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
 
     ret = vfu_realize_ctx(o->vfu_ctx);
     if (ret < 0) {
-        error_setg(errp, "vfu: Failed to realize device %s- %s",
-                   o->device, strerror(errno));
+        error_setg_errno(errp, "vfu: Failed to realize device %s",
+                         o->device);
         goto fail;
     }
 
diff --git a/hw/sensor/emc141x.c b/hw/sensor/emc141x.c
index 7b2ce383a1..a51fc44395 100644
--- a/hw/sensor/emc141x.c
+++ b/hw/sensor/emc141x.c
@@ -59,7 +59,7 @@ static void emc141x_get_temperature(Object *obj, Visitor *v, const char *name,
     unsigned tempid;
 
     if (sscanf(name, "temperature%u", &tempid) != 1) {
-        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
+        error_setg_errno(errp, errno, "error reading %s", name);
         return;
     }
 
@@ -86,7 +86,7 @@ static void emc141x_set_temperature(Object *obj, Visitor *v, const char *name,
     }
 
     if (sscanf(name, "temperature%u", &tempid) != 1) {
-        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
+        error_setg_errno(errp, errno, "error reading %s", name);
         return;
     }
 
diff --git a/hw/sensor/tmp421.c b/hw/sensor/tmp421.c
index 3421c44086..127edd0ba5 100644
--- a/hw/sensor/tmp421.c
+++ b/hw/sensor/tmp421.c
@@ -117,7 +117,7 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name,
     int tempid;
 
     if (sscanf(name, "temperature%d", &tempid) != 1) {
-        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
+        error_setg_errno(errp, errno, "error reading %s", name);
         return;
     }
 
@@ -154,7 +154,7 @@ static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name,
     }
 
     if (sscanf(name, "temperature%d", &tempid) != 1) {
-        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
+        error_setg_errno(errp, errno, "error reading %s", name);
         return;
     }
 
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 7558b2ad83..b228f9eb85 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -1281,8 +1281,8 @@ static int save_opt_one(void *opaque,
                 break;
             }
             if (ret < 0) {
-                error_setg(errp, "Unable to read from %s: %s",
-                           value, strerror(errno));
+                error_setg_errno(errp, errno, "Unable to read from %s",
+                                 value);
                 qemu_close(fd);
                 return -1;
             }
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index 4a7b970976..f97d576171 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -41,8 +41,8 @@ vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp)
     uint32_t val = (uint32_t)-1;
 
     if (ioctl(fd, cmd, &val) < 0) {
-        error_setg(errp, "vhost-vdpa-device: cmd 0x%lx failed: %s",
-                   cmd, strerror(errno));
+        error_setg_errno(errp, errno, "vhost-vdpa-device: cmd 0x%lx failed",
+                         cmd);
     }
 
     return val;
diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 3f98dcb6fd..5454372ac6 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -582,7 +582,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
 
     ufd = uffd_open(O_CLOEXEC);
     if (ufd == -1) {
-        error_setg(errp, "Userfaultfd not available: %s", strerror(errno));
+        error_setg_errno(errp, errno, "Userfaultfd not available");
         goto out;
     }
 
@@ -620,7 +620,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
      * it was enabled.
      */
     if (munlockall()) {
-        error_setg(errp, "munlockall() failed: %s", strerror(errno));
+        error_setg_errno(errp, errno, "munlockall() failed");
         goto out;
     }
 
@@ -632,7 +632,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
     testarea = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE |
                                     MAP_ANONYMOUS, -1, 0);
     if (testarea == MAP_FAILED) {
-        error_setg(errp, "Failed to map test area: %s", strerror(errno));
+        error_setg_errno(errp, errno, "Failed to map test area");
         goto out;
     }
     g_assert(QEMU_PTR_IS_ALIGNED(testarea, pagesize));
@@ -642,14 +642,14 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
     reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
 
     if (ioctl(ufd, UFFDIO_REGISTER, &reg_struct)) {
-        error_setg(errp, "UFFDIO_REGISTER failed: %s", strerror(errno));
+        error_setg_errno(errp, errno, "UFFDIO_REGISTER failed");
         goto out;
     }
 
     range_struct.start = (uintptr_t)testarea;
     range_struct.len = pagesize;
     if (ioctl(ufd, UFFDIO_UNREGISTER, &range_struct)) {
-        error_setg(errp, "UFFDIO_UNREGISTER failed: %s", strerror(errno));
+        error_setg_errno(errp, errno, "UFFDIO_UNREGISTER failed");
         goto out;
     }
 
diff --git a/migration/rdma.c b/migration/rdma.c
index ef4885ef5f..9e301cf917 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2349,7 +2349,7 @@ static int qemu_get_cm_event_timeout(RDMAContext *rdma,
         error_setg(errp, "RDMA ERROR: poll cm event timeout");
         return -1;
     } else if (ret < 0) {
-        error_setg_errno(errp, "RDMA ERROR: failed to poll cm event");
+        error_setg_errno(errp, errno, "RDMA ERROR: failed to poll cm event");
         return -1;
     } else if (poll_fd.revents & POLLIN) {
         if (rdma_get_cm_event(rdma->channel, cm_event) < 0) {
diff --git a/net/slirp.c b/net/slirp.c
index 5996fec512..04925f3318 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -1020,8 +1020,9 @@ static int slirp_smb(SlirpState* s, const char *exported_dir,
     }
 
     if (access(exported_dir, R_OK | X_OK)) {
-        error_setg(errp, "Error accessing shared directory '%s': %s",
-                   exported_dir, strerror(errno));
+        error_setg_errno(errp, errno,
+                         "Error accessing shared directory '%s'",
+                         exported_dir);
         return -1;
     }
 
diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
index 246171d323..661972e34e 100644
--- a/qga/commands-posix-ssh.c
+++ b/qga/commands-posix-ssh.c
@@ -61,20 +61,22 @@ mkdir_for_user(const char *path, const struct passwd *p,
                mode_t mode, Error **errp)
 {
     if (g_mkdir(path, mode) == -1) {
-        error_setg(errp, "failed to create directory '%s': %s",
-                   path, g_strerror(errno));
+        error_setg_errno(errp, errno, "failed to create directory '%s'",
+                         path);
         return false;
     }
 
     if (chown(path, p->pw_uid, p->pw_gid) == -1) {
-        error_setg(errp, "failed to set ownership of directory '%s': %s",
-                   path, g_strerror(errno));
+        error_setg_errno(errp, errno,
+                         "failed to set ownership of directory '%s'",
+                         path);
         return false;
     }
 
     if (chmod(path, mode) == -1) {
-        error_setg(errp, "failed to set permissions of directory '%s': %s",
-                   path, g_strerror(errno));
+        error_setg_errno(errp, errno,
+                         "failed to set permissions of directory '%s'",
+                         path);
         return false;
     }
 
@@ -95,14 +97,15 @@ write_authkeys(const char *path, const GStrv keys,
     }
 
     if (chown(path, p->pw_uid, p->pw_gid) == -1) {
-        error_setg(errp, "failed to set ownership of directory '%s': %s",
-                   path, g_strerror(errno));
+        error_setg_errno(errp, errno,
+                         "failed to set ownership of directory '%s'",
+                         path);
         return false;
     }
 
     if (chmod(path, 0600) == -1) {
-        error_setg(errp, "failed to set permissions of '%s': %s",
-                   path, g_strerror(errno));
+        error_setg_errno(errp, errno, "failed to set permissions of '%s'",
+                         path);
         return false;
     }
 
diff --git a/system/vl.c b/system/vl.c
index 5091fe52d9..2ef5b4b3b2 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -619,7 +619,7 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
     }
 #endif
     if (dupfd == -1) {
-        error_setg(errp, "error duplicating fd: %s", strerror(errno));
+        error_setg_errno(errp, errno, "error duplicating fd");
         return -1;
     }
 
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 43124bf1c7..3501b5d546 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2699,9 +2699,8 @@ int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
 
     ret = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &s);
     if (ret < 0) {
-        error_setg(errp, "Unable to open fd for %s HPT %s KVM: %s",
-                   write ? "writing" : "reading", write ? "to" : "from",
-                   strerror(errno));
+        error_setg_errno(errp, errno, "Unable to open fd for %s HPT %s KVM",
+                   write ? "writing" : "reading", write ? "to" : "from");
         return -errno;
     }
 
-- 
2.49.0

[PATCH 13/14] qga/commands-win32: Use error_setg_win32() for better error messages

[Markus Armbruster]

We include numeric GetLastError() codes in error messages in a few
places, like this:

    error_setg(errp, "GRIPE: %d", (int)GetLastError());

Show text instead, like this:

    error_setg_win32(errp, GetLastError(), "GRIPE");

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qga/commands-win32.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index acc2c11589..0fd0c966e4 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1798,8 +1798,8 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
     tf.dwHighDateTime = (DWORD) (time >> 32);
 
     if (!FileTimeToSystemTime(&tf, &ts)) {
-        error_setg(errp, "Failed to convert system time %d",
-                   (int)GetLastError());
+        error_setg_win32(errp, GetLastError(),
+                         "Failed to convert system time");
         return;
     }
 
@@ -1810,7 +1810,8 @@ void qmp_guest_set_time(bool has_time, int64_t time_ns, Error **errp)
     }
 
     if (!SetSystemTime(&ts)) {
-        error_setg(errp, "Failed to set time to guest: %d", (int)GetLastError());
+        error_setg_win32(errp, GetLastError(),
+                         "Failed to set time to guest");
         return;
     }
 }
@@ -1834,13 +1835,12 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
         (length > sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION))) {
         ptr = pslpi = g_malloc0(length);
         if (GetLogicalProcessorInformation(pslpi, &length) == FALSE) {
-            error_setg(&local_err, "Failed to get processor information: %d",
-                       (int)GetLastError());
+            error_setg_win32(&local_err, GetLastError(),
+                             "Failed to get processor information");
         }
     } else {
-        error_setg(&local_err,
-                   "Failed to get processor information buffer length: %d",
-                   (int)GetLastError());
+        error_setg_win32(&local_err, GetLastError(),
+                         "Failed to get processor information buffer length");
     }
 
     while ((local_err == NULL) && (length > 0)) {
-- 
2.49.0

[PATCH 14/14] block/file-win32: Improve an error message

[Markus Armbruster]

Two out of three calls of CreateFile() use error_setg_win32() to
report errors.  The third uses error_setg_errno(), mapping
ERROR_ACCESS_DENIED to EACCES, and everything else to EINVAL, throwing
away detail.  Switch it to error_setg_win32().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/file-win32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/file-win32.c b/block/file-win32.c
index 0efb609e1d..78961837c8 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -904,7 +904,7 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
         } else {
             ret = -EINVAL;
         }
-        error_setg_errno(errp, -ret, "Could not open device");
+        error_setg_win32(errp, err, "Could not open device");
         goto done;
     }
 
-- 
2.49.0

Re: [PATCH 09/14] error: Use error_setg_file_open() for simplicity and consistency

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Replace
> 
>     error_setg_errno(errp, errno, MSG, FNAME);
> 
> by
> 
>     error_setg_file_open(errp, errno, FNAME);
> 
> where MSG is "Could not open '%s'" or similar.
> 
> Also replace equivalent uses of error_setg().
> 
> A few messages lose prefixes ("net dump: ", "SEV: ", __func__ ": ").
> We could put them back with error_prepend().  Not worth the bother.

Yeh, I guess you could just do it with another macro using
the same internal function just with string concatenation.

> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> ---
>  hw/9pfs/9p-local.c        | 2 +-
>  hw/acpi/core.c            | 2 +-
>  hw/core/loader.c          | 2 +-
>  hw/pci-host/xen_igd_pt.c  | 2 +-
>  monitor/hmp-cmds-target.c | 2 +-
>  net/dump.c                | 2 +-
>  net/tap-bsd.c             | 6 +++---
>  net/tap-linux.c           | 2 +-
>  target/i386/sev.c         | 6 ++----
>  ui/ui-qmp-cmds.c          | 3 +--
>  util/vfio-helpers.c       | 5 ++---
>  11 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 31e216227c..376b377698 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1456,7 +1456,7 @@ static int local_init(FsContext *ctx, Error **errp)
>  
>      data->mountfd = open(ctx->fs_root, O_DIRECTORY | O_RDONLY);
>      if (data->mountfd == -1) {
> -        error_setg_errno(errp, errno, "failed to open '%s'", ctx->fs_root);
> +        error_setg_file_open(errp, errno, ctx->fs_root);
>          goto err;
>      }
>  
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index ff16582803..d2677332af 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -277,7 +277,7 @@ void acpi_table_add(const QemuOpts *opts, Error **errp)
>          int fd = open(*cur, O_RDONLY | O_BINARY);
>  
>          if (fd < 0) {
> -            error_setg(errp, "can't open file %s: %s", *cur, strerror(errno));
> +            error_setg_file_open(errp, errno, *cur);
>              goto out;
>          }
>  
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 590c5b02aa..b56e5eb2f5 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -379,7 +379,7 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
>  
>      fd = open(filename, O_RDONLY | O_BINARY);
>      if (fd < 0) {
> -        error_setg_errno(errp, errno, "Failed to open file: %s", filename);
> +        error_setg_file_open(errp, errno, filename);
>          return;
>      }
>      if (read(fd, hdr, EI_NIDENT) != EI_NIDENT) {
> diff --git a/hw/pci-host/xen_igd_pt.c b/hw/pci-host/xen_igd_pt.c
> index 5dd17ef236..f6016f2cd5 100644
> --- a/hw/pci-host/xen_igd_pt.c
> +++ b/hw/pci-host/xen_igd_pt.c
> @@ -55,7 +55,7 @@ static void host_pci_config_read(int pos, int len, uint32_t *val, Error **errp)
>  
>      config_fd = open(path, O_RDWR);
>      if (config_fd < 0) {
> -        error_setg_errno(errp, errno, "Failed to open: %s", path);
> +        error_setg_file_open(errp, errno, path);
>          goto out;
>      }
>  
> diff --git a/monitor/hmp-cmds-target.c b/monitor/hmp-cmds-target.c
> index e982061146..ad4ed2167d 100644
> --- a/monitor/hmp-cmds-target.c
> +++ b/monitor/hmp-cmds-target.c
> @@ -331,7 +331,7 @@ static uint64_t vtop(void *ptr, Error **errp)
>  
>      fd = open("/proc/self/pagemap", O_RDONLY);
>      if (fd == -1) {
> -        error_setg_errno(errp, errno, "Cannot open /proc/self/pagemap");
> +        error_setg_file_open(errp, errno, "/proc/self/pagemap");
>          return -1;
>      }
>  
> diff --git a/net/dump.c b/net/dump.c
> index 581234b775..0c39f09892 100644
> --- a/net/dump.c
> +++ b/net/dump.c
> @@ -111,7 +111,7 @@ static int net_dump_state_init(DumpState *s, const char *filename,
>  
>      fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY | O_BINARY, 0644);
>      if (fd < 0) {
> -        error_setg_errno(errp, errno, "net dump: can't open %s", filename);
> +        error_setg_file_open(errp, errno, filename);
>          return -1;
>      }
>  
> diff --git a/net/tap-bsd.c b/net/tap-bsd.c
> index bbf84d1828..3fd300d46f 100644
> --- a/net/tap-bsd.c
> +++ b/net/tap-bsd.c
> @@ -68,7 +68,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>          }
>      }
>      if (fd < 0) {
> -        error_setg_errno(errp, errno, "could not open %s", dname);
> +        error_setg_file_open(errp, errno, dname);
>          return -1;
>      }
>  
> @@ -118,7 +118,7 @@ static int tap_open_clone(char *ifname, int ifname_size, Error **errp)
>  
>      fd = RETRY_ON_EINTR(open(PATH_NET_TAP, O_RDWR));
>      if (fd < 0) {
> -        error_setg_errno(errp, errno, "could not open %s", PATH_NET_TAP);
> +        error_setg_file_open(errp, errno, PATH_NET_TAP);
>          return -1;
>      }
>  
> @@ -166,7 +166,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>          snprintf(dname, sizeof dname, "/dev/%s", ifname);
>          fd = RETRY_ON_EINTR(open(dname, O_RDWR));
>          if (fd < 0 && errno != ENOENT) {
> -            error_setg_errno(errp, errno, "could not open %s", dname);
> +            error_setg_file_open(errp, errno, dname);
>              return -1;
>          }
>      }
> diff --git a/net/tap-linux.c b/net/tap-linux.c
> index 2a90b58467..909c4f1fcf 100644
> --- a/net/tap-linux.c
> +++ b/net/tap-linux.c
> @@ -57,7 +57,7 @@ int tap_open(char *ifname, int ifname_size, int *vnet_hdr,
>      if (fd < 0) {
>          fd = RETRY_ON_EINTR(open(PATH_NET_TUN, O_RDWR));
>          if (fd < 0) {
> -            error_setg_errno(errp, errno, "could not open %s", PATH_NET_TUN);
> +            error_setg_file_open(errp, errno, PATH_NET_TUN);
>              return -1;
>          }
>      }
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index fd2dada013..8660ecd9e4 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -891,8 +891,7 @@ static SevCapability *sev_get_capabilities(Error **errp)
>  
>      fd = open(sev_device, O_RDWR);
>      if (fd < 0) {
> -        error_setg_errno(errp, errno, "SEV: Failed to open %s",
> -                         sev_device);
> +        error_setg_file_open(errp, errno, sev_device);
>          g_free(sev_device);
>          return NULL;
>      }
> @@ -1819,8 +1818,7 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>      devname = object_property_get_str(OBJECT(sev_common), "sev-device", NULL);
>      sev_common->sev_fd = open(devname, O_RDWR);
>      if (sev_common->sev_fd < 0) {
> -        error_setg(errp, "%s: Failed to open %s '%s'", __func__,
> -                   devname, strerror(errno));
> +        error_setg_file_open(errp, errno, devname);
>          g_free(devname);
>          return -1;
>      }
> diff --git a/ui/ui-qmp-cmds.c b/ui/ui-qmp-cmds.c
> index 74fa6c6ec5..d927121676 100644
> --- a/ui/ui-qmp-cmds.c
> +++ b/ui/ui-qmp-cmds.c
> @@ -371,8 +371,7 @@ qmp_screendump(const char *filename, const char *device,
>  
>      fd = qemu_open_old(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0666);
>      if (fd == -1) {
> -        error_setg(errp, "failed to open file '%s': %s", filename,
> -                   strerror(errno));
> +        error_setg_file_open(errp, errno, filename);
>          return;
>      }
>  
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index fdff042ab4..8b1b2e2f05 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -309,7 +309,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
>      s->container = open("/dev/vfio/vfio", O_RDWR);
>  
>      if (s->container == -1) {
> -        error_setg_errno(errp, errno, "Failed to open /dev/vfio/vfio");
> +        error_setg_file_open(errp, errno, "/dev/vfio/vfio");
>          return -errno;
>      }
>      if (ioctl(s->container, VFIO_GET_API_VERSION) != VFIO_API_VERSION) {
> @@ -333,8 +333,7 @@ static int qemu_vfio_init_pci(QEMUVFIOState *s, const char *device,
>  
>      s->group = open(group_file, O_RDWR);
>      if (s->group == -1) {
> -        error_setg_errno(errp, errno, "Failed to open VFIO group file: %s",
> -                         group_file);
> +        error_setg_file_open(errp, errno, group_file);
>          g_free(group_file);
>          ret = -errno;
>          goto fail_container;
> -- 
> 2.49.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 11/14] error: Use error_setg_errno() to improve error messages

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> A few error messages show numeric errno codes.  Use error_setg_errno()
> to show human-readable text instead.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

...

> diff --git a/migration/rdma.c b/migration/rdma.c
> index 337b415889..ef4885ef5f 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2349,8 +2349,7 @@ static int qemu_get_cm_event_timeout(RDMAContext *rdma,
>          error_setg(errp, "RDMA ERROR: poll cm event timeout");
>          return -1;
>      } else if (ret < 0) {
> -        error_setg(errp, "RDMA ERROR: failed to poll cm event, errno=%i",
> -                   errno);
> +        error_setg_errno(errp, "RDMA ERROR: failed to poll cm event");

Hasn't that lost the errno ?

Dave

-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 12/14] error: Use error_setg_errno() for simplicity and consistency

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Use error_setg_errno() instead of passing the value of strerror() or
> g_strerror() to error_setg().
> 
> The separator between the error message proper and the value of
> strerror() changes from " : ", "", " - ", "- " to ": " in places.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> @@ -792,9 +792,9 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>                             VFU_REGION_FLAG_RW | VFU_REGION_FLAG_ALWAYS_CB,
>                             NULL, 0, -1, 0);
>      if (ret < 0) {
> -        error_setg(errp,
> -                   "vfu: Failed to setup config space handlers for %s- %s",
> -                   o->device, strerror(errno));
> +        error_setg_errno(errp,
> +                         "vfu: Failed to setup config space handlers for %s",
> +                         o->device);

missing errno.

>          goto fail;
>      }
>  
> @@ -822,8 +822,8 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>  
>      ret = vfu_realize_ctx(o->vfu_ctx);
>      if (ret < 0) {
> -        error_setg(errp, "vfu: Failed to realize device %s- %s",
> -                   o->device, strerror(errno));
> +        error_setg_errno(errp, "vfu: Failed to realize device %s",
> +                         o->device);

missing errno.

Dave

>          goto fail;
>      }
>  
> diff --git a/hw/sensor/emc141x.c b/hw/sensor/emc141x.c
> index 7b2ce383a1..a51fc44395 100644
> --- a/hw/sensor/emc141x.c
> +++ b/hw/sensor/emc141x.c
> @@ -59,7 +59,7 @@ static void emc141x_get_temperature(Object *obj, Visitor *v, const char *name,
>      unsigned tempid;
>  
>      if (sscanf(name, "temperature%u", &tempid) != 1) {
> -        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
> +        error_setg_errno(errp, errno, "error reading %s", name);
>          return;
>      }
>  
> @@ -86,7 +86,7 @@ static void emc141x_set_temperature(Object *obj, Visitor *v, const char *name,
>      }
>  
>      if (sscanf(name, "temperature%u", &tempid) != 1) {
> -        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
> +        error_setg_errno(errp, errno, "error reading %s", name);
>          return;
>      }
>  
> diff --git a/hw/sensor/tmp421.c b/hw/sensor/tmp421.c
> index 3421c44086..127edd0ba5 100644
> --- a/hw/sensor/tmp421.c
> +++ b/hw/sensor/tmp421.c
> @@ -117,7 +117,7 @@ static void tmp421_get_temperature(Object *obj, Visitor *v, const char *name,
>      int tempid;
>  
>      if (sscanf(name, "temperature%d", &tempid) != 1) {
> -        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
> +        error_setg_errno(errp, errno, "error reading %s", name);
>          return;
>      }
>  
> @@ -154,7 +154,7 @@ static void tmp421_set_temperature(Object *obj, Visitor *v, const char *name,
>      }
>  
>      if (sscanf(name, "temperature%d", &tempid) != 1) {
> -        error_setg(errp, "error reading %s: %s", name, g_strerror(errno));
> +        error_setg_errno(errp, errno, "error reading %s", name);
>          return;
>      }
>  
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 7558b2ad83..b228f9eb85 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -1281,8 +1281,8 @@ static int save_opt_one(void *opaque,
>                  break;
>              }
>              if (ret < 0) {
> -                error_setg(errp, "Unable to read from %s: %s",
> -                           value, strerror(errno));
> +                error_setg_errno(errp, errno, "Unable to read from %s",
> +                                 value);
>                  qemu_close(fd);
>                  return -1;
>              }
> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> index 4a7b970976..f97d576171 100644
> --- a/hw/virtio/vdpa-dev.c
> +++ b/hw/virtio/vdpa-dev.c
> @@ -41,8 +41,8 @@ vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp)
>      uint32_t val = (uint32_t)-1;
>  
>      if (ioctl(fd, cmd, &val) < 0) {
> -        error_setg(errp, "vhost-vdpa-device: cmd 0x%lx failed: %s",
> -                   cmd, strerror(errno));
> +        error_setg_errno(errp, errno, "vhost-vdpa-device: cmd 0x%lx failed",
> +                         cmd);
>      }
>  
>      return val;
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 3f98dcb6fd..5454372ac6 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -582,7 +582,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
>  
>      ufd = uffd_open(O_CLOEXEC);
>      if (ufd == -1) {
> -        error_setg(errp, "Userfaultfd not available: %s", strerror(errno));
> +        error_setg_errno(errp, errno, "Userfaultfd not available");
>          goto out;
>      }
>  
> @@ -620,7 +620,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
>       * it was enabled.
>       */
>      if (munlockall()) {
> -        error_setg(errp, "munlockall() failed: %s", strerror(errno));
> +        error_setg_errno(errp, errno, "munlockall() failed");
>          goto out;
>      }
>  
> @@ -632,7 +632,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
>      testarea = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE |
>                                      MAP_ANONYMOUS, -1, 0);
>      if (testarea == MAP_FAILED) {
> -        error_setg(errp, "Failed to map test area: %s", strerror(errno));
> +        error_setg_errno(errp, errno, "Failed to map test area");
>          goto out;
>      }
>      g_assert(QEMU_PTR_IS_ALIGNED(testarea, pagesize));
> @@ -642,14 +642,14 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis, Error **errp)
>      reg_struct.mode = UFFDIO_REGISTER_MODE_MISSING;
>  
>      if (ioctl(ufd, UFFDIO_REGISTER, &reg_struct)) {
> -        error_setg(errp, "UFFDIO_REGISTER failed: %s", strerror(errno));
> +        error_setg_errno(errp, errno, "UFFDIO_REGISTER failed");
>          goto out;
>      }
>  
>      range_struct.start = (uintptr_t)testarea;
>      range_struct.len = pagesize;
>      if (ioctl(ufd, UFFDIO_UNREGISTER, &range_struct)) {
> -        error_setg(errp, "UFFDIO_UNREGISTER failed: %s", strerror(errno));
> +        error_setg_errno(errp, errno, "UFFDIO_UNREGISTER failed");
>          goto out;
>      }
>  
> diff --git a/migration/rdma.c b/migration/rdma.c
> index ef4885ef5f..9e301cf917 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -2349,7 +2349,7 @@ static int qemu_get_cm_event_timeout(RDMAContext *rdma,
>          error_setg(errp, "RDMA ERROR: poll cm event timeout");
>          return -1;
>      } else if (ret < 0) {
> -        error_setg_errno(errp, "RDMA ERROR: failed to poll cm event");
> +        error_setg_errno(errp, errno, "RDMA ERROR: failed to poll cm event");
>          return -1;
>      } else if (poll_fd.revents & POLLIN) {
>          if (rdma_get_cm_event(rdma->channel, cm_event) < 0) {
> diff --git a/net/slirp.c b/net/slirp.c
> index 5996fec512..04925f3318 100644
> --- a/net/slirp.c
> +++ b/net/slirp.c
> @@ -1020,8 +1020,9 @@ static int slirp_smb(SlirpState* s, const char *exported_dir,
>      }
>  
>      if (access(exported_dir, R_OK | X_OK)) {
> -        error_setg(errp, "Error accessing shared directory '%s': %s",
> -                   exported_dir, strerror(errno));
> +        error_setg_errno(errp, errno,
> +                         "Error accessing shared directory '%s'",
> +                         exported_dir);
>          return -1;
>      }
>  
> diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
> index 246171d323..661972e34e 100644
> --- a/qga/commands-posix-ssh.c
> +++ b/qga/commands-posix-ssh.c
> @@ -61,20 +61,22 @@ mkdir_for_user(const char *path, const struct passwd *p,
>                 mode_t mode, Error **errp)
>  {
>      if (g_mkdir(path, mode) == -1) {
> -        error_setg(errp, "failed to create directory '%s': %s",
> -                   path, g_strerror(errno));
> +        error_setg_errno(errp, errno, "failed to create directory '%s'",
> +                         path);
>          return false;
>      }
>  
>      if (chown(path, p->pw_uid, p->pw_gid) == -1) {
> -        error_setg(errp, "failed to set ownership of directory '%s': %s",
> -                   path, g_strerror(errno));
> +        error_setg_errno(errp, errno,
> +                         "failed to set ownership of directory '%s'",
> +                         path);
>          return false;
>      }
>  
>      if (chmod(path, mode) == -1) {
> -        error_setg(errp, "failed to set permissions of directory '%s': %s",
> -                   path, g_strerror(errno));
> +        error_setg_errno(errp, errno,
> +                         "failed to set permissions of directory '%s'",
> +                         path);
>          return false;
>      }
>  
> @@ -95,14 +97,15 @@ write_authkeys(const char *path, const GStrv keys,
>      }
>  
>      if (chown(path, p->pw_uid, p->pw_gid) == -1) {
> -        error_setg(errp, "failed to set ownership of directory '%s': %s",
> -                   path, g_strerror(errno));
> +        error_setg_errno(errp, errno,
> +                         "failed to set ownership of directory '%s'",
> +                         path);
>          return false;
>      }
>  
>      if (chmod(path, 0600) == -1) {
> -        error_setg(errp, "failed to set permissions of '%s': %s",
> -                   path, g_strerror(errno));
> +        error_setg_errno(errp, errno, "failed to set permissions of '%s'",
> +                         path);
>          return false;
>      }
>  
> diff --git a/system/vl.c b/system/vl.c
> index 5091fe52d9..2ef5b4b3b2 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -619,7 +619,7 @@ static int parse_add_fd(void *opaque, QemuOpts *opts, Error **errp)
>      }
>  #endif
>      if (dupfd == -1) {
> -        error_setg(errp, "error duplicating fd: %s", strerror(errno));
> +        error_setg_errno(errp, errno, "error duplicating fd");
>          return -1;
>      }
>  
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 43124bf1c7..3501b5d546 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2699,9 +2699,8 @@ int kvmppc_get_htab_fd(bool write, uint64_t index, Error **errp)
>  
>      ret = kvm_vm_ioctl(kvm_state, KVM_PPC_GET_HTAB_FD, &s);
>      if (ret < 0) {
> -        error_setg(errp, "Unable to open fd for %s HPT %s KVM: %s",
> -                   write ? "writing" : "reading", write ? "to" : "from",
> -                   strerror(errno));
> +        error_setg_errno(errp, errno, "Unable to open fd for %s HPT %s KVM",
> +                   write ? "writing" : "reading", write ? "to" : "from");
>          return -errno;
>      }
>  
> -- 
> 2.49.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 03/14] tap-solaris: Use error_setg_file_open() for better error messages

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Error messages change from
> 
>     Can't open /dev/ip (actually /dev/udp)
>     Can't open /dev/tap
>     Can't open /dev/tap (2)
> 
> to
> 
>     Could not open '/dev/udp': REASON
>     Could not open '/dev/tap': REASON
> 
> where REASON is the value of strerror(errno).

I guess the new macro has a __LINE__ so the (2) is redundant.

> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> ---
>  net/tap-solaris.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tap-solaris.c b/net/tap-solaris.c
> index 75397e6c54..faf7922ea8 100644
> --- a/net/tap-solaris.c
> +++ b/net/tap-solaris.c
> @@ -87,13 +87,13 @@ static int tap_alloc(char *dev, size_t dev_size, Error **errp)
>  
>      ip_fd = RETRY_ON_EINTR(open("/dev/udp", O_RDWR, 0));
>      if (ip_fd < 0) {
> -        error_setg(errp, "Can't open /dev/ip (actually /dev/udp)");
> +        error_setg_file_open(errp, errno, "/dev/udp");
>          return -1;
>      }
>  
>      tap_fd = RETRY_ON_EINTR(open("/dev/tap", O_RDWR, 0));
>      if (tap_fd < 0) {
> -        error_setg(errp, "Can't open /dev/tap");
> +        error_setg_file_open(errp, errno, "/dev/tap");
>          return -1;
>      }
>  
> @@ -107,7 +107,7 @@ static int tap_alloc(char *dev, size_t dev_size, Error **errp)
>  
>      if_fd = RETRY_ON_EINTR(open("/dev/tap", O_RDWR, 0));
>      if (if_fd < 0) {
> -        error_setg(errp, "Can't open /dev/tap (2)");
> +        error_setg_file_open(errp, errno, "/dev/tap");
>          return -1;
>      }
>      if(ioctl(if_fd, I_PUSH, "ip") < 0){
> -- 
> 2.49.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 02/14] hw/usb: Use error_setg_file_open() for a better error message

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> The error message changes from
> 
>     open FILENAME failed
> 
> to
> 
>     Could not open 'FILENAME': REASON
> 
> where REASON is the value of strerror(errno).
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/usb/bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index 8dd2ce415e..47d42ca3c1 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -262,7 +262,7 @@ static void usb_qdev_realize(DeviceState *qdev, Error **errp)
>          int fd = qemu_open_old(dev->pcap_filename,
>                                 O_CREAT | O_WRONLY | O_TRUNC | O_BINARY, 0666);
>          if (fd < 0) {
> -            error_setg(errp, "open %s failed", dev->pcap_filename);
> +            error_setg_file_open(errp, errno, dev->pcap_filename);

Wouldn't it be easier to flip it to use qemu_open() ?

Dave

>              usb_qdev_unrealize(qdev);
>              return;
>          }
> -- 
> 2.49.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 04/14] qga: Use error_setg_file_open() for better error messages

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Error messages change from
> 
>     open("FNAME"): REASON
> 
> to
> 
>     Could not open 'FNAME': REASON
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> ---
>  qga/commands-linux.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 4a09ddc760..5cf76ca2d9 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -1502,14 +1502,15 @@ static void transfer_vcpu(GuestLogicalProcessor *vcpu, bool sys2vcpu,
>  
>      dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>      if (dirfd == -1) {
> -        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +        error_setg_file_open(errp, errno, dirpath);
>          return;
>      }
>  
>      fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
>      if (fd == -1) {
>          if (errno != ENOENT) {
> -            error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
> +            error_setg_errno(errp, errno, "could not open %s/%s",
> +                             dirpath, fn);
>          } else if (sys2vcpu) {
>              vcpu->online = true;
>              vcpu->can_offline = false;
> @@ -1711,7 +1712,7 @@ static void transfer_memory_block(GuestMemoryBlock *mem_blk, bool sys2memblk,
>      dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>      if (dirfd == -1) {
>          if (sys2memblk) {
> -            error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +            error_setg_file_open(errp, errno, dirpath);
>          } else {
>              if (errno == ENOENT) {
>                  result->response = GUEST_MEMORY_BLOCK_RESPONSE_TYPE_NOT_FOUND;
> @@ -1936,7 +1937,7 @@ static GuestDiskStatsInfoList *guest_get_diskstats(Error **errp)
>  
>      fp = fopen(diskstats, "r");
>      if (fp  == NULL) {
> -        error_setg_errno(errp, errno, "open(\"%s\")", diskstats);
> +        error_setg_file_open(errp, errno, diskstats);
>          return NULL;
>      }
>  
> @@ -2047,7 +2048,7 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
>  
>      fp = fopen(cpustats, "r");
>      if (fp  == NULL) {
> -        error_setg_errno(errp, errno, "open(\"%s\")", cpustats);
> +        error_setg_file_open(errp, errno, cpustats);
>          return NULL;
>      }
>  
> -- 
> 2.49.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 05/14] hw/scsi: Use error_setg_file_open() for a better error message

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> The error message changes from
> 
>     vhost-scsi: open vhost char device failed: REASON
> 
> to
> 
>     Could not open '/dev/vhost-scsi': REASON
> 
> I think the exact file name is more useful to know than the file's
> purpose.
> 
> We could put back the "vhost-scsi: " prefix with error_prepend().  Not
> worth the bother.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

> ---
>  hw/scsi/vhost-scsi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index cdf405b0f8..239138c931 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -245,8 +245,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
>      } else {
>          vhostfd = open("/dev/vhost-scsi", O_RDWR);
>          if (vhostfd < 0) {
> -            error_setg(errp, "vhost-scsi: open vhost char device failed: %s",
> -                       strerror(errno));
> +            error_setg_file_open(errp, errno, "/dev/vhost-scsi");
>              return;
>          }
>      }
> -- 
> 2.49.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 02/14] hw/usb: Use error_setg_file_open() for a better error message

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> The error message changes from
>> 
>>     open FILENAME failed
>> 
>> to
>> 
>>     Could not open 'FILENAME': REASON
>> 
>> where REASON is the value of strerror(errno).
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/usb/bus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
>> index 8dd2ce415e..47d42ca3c1 100644
>> --- a/hw/usb/bus.c
>> +++ b/hw/usb/bus.c
>> @@ -262,7 +262,7 @@ static void usb_qdev_realize(DeviceState *qdev, Error **errp)
>>          int fd = qemu_open_old(dev->pcap_filename,
>>                                 O_CREAT | O_WRONLY | O_TRUNC | O_BINARY, 0666);
>>          if (fd < 0) {
>> -            error_setg(errp, "open %s failed", dev->pcap_filename);
>> +            error_setg_file_open(errp, errno, dev->pcap_filename);
>
> Wouldn't it be easier to flip it to use qemu_open() ?

Mechanical change; I missed the obvious :)

I'll give it a try, along with the call in ui/ui-qmp-cmd.c [PATCH 09].
Thanks!

>
> Dave
>
>>              usb_qdev_unrealize(qdev);
>>              return;
>>          }
>> -- 
>> 2.49.0
>> 

Re: [PATCH 03/14] tap-solaris: Use error_setg_file_open() for better error messages

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Error messages change from
>> 
>>     Can't open /dev/ip (actually /dev/udp)
>>     Can't open /dev/tap
>>     Can't open /dev/tap (2)
>> 
>> to
>> 
>>     Could not open '/dev/udp': REASON
>>     Could not open '/dev/tap': REASON
>> 
>> where REASON is the value of strerror(errno).
>
> I guess the new macro has a __LINE__ so the (2) is redundant.

It does capture __FILE__, __LINE__, and __func__, but they're only
printed for &error_abort.

How likely is it that the first open of /dev/tap succeeds, and the
second fails?

Do users users then need to know that the second failed?  If yes, then
" (2)" is a terrible way to tell them.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

Thanks!

Re: [PATCH 09/14] error: Use error_setg_file_open() for simplicity and consistency

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Replace
>> 
>>     error_setg_errno(errp, errno, MSG, FNAME);
>> 
>> by
>> 
>>     error_setg_file_open(errp, errno, FNAME);
>> 
>> where MSG is "Could not open '%s'" or similar.
>> 
>> Also replace equivalent uses of error_setg().
>> 
>> A few messages lose prefixes ("net dump: ", "SEV: ", __func__ ": ").
>> We could put them back with error_prepend().  Not worth the bother.
>
> Yeh, I guess you could just do it with another macro using
> the same internal function just with string concatenation.

I'm no fan of such prefixes.  A sign of developers not caring enough to
craft a good error message for *users*.  *Especially* in the case of
__func__.

The error messages changes in question are:

    net dump: can't open DUMP-FILE: REASON
    Could not open 'DUMP-FILE': REASON

    SEV: Failed to open SEV-DEVICE: REASON
    Could not open 'SEV-DEVICE': REASON

    sev_common_kvm_init: Failed to open SEV_DEVICE 'REASON'
    Could not open 'SEV-DEVICE': REASON

I think these are all improvements, and the loss of the prefix is fine.

>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

Thanks!

Re: [PATCH 11/14] error: Use error_setg_errno() to improve error messages

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> A few error messages show numeric errno codes.  Use error_setg_errno()
>> to show human-readable text instead.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> ...
>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 337b415889..ef4885ef5f 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -2349,8 +2349,7 @@ static int qemu_get_cm_event_timeout(RDMAContext *rdma,
>>          error_setg(errp, "RDMA ERROR: poll cm event timeout");
>>          return -1;
>>      } else if (ret < 0) {
>> -        error_setg(errp, "RDMA ERROR: failed to poll cm event, errno=%i",
>> -                   errno);
>> +        error_setg_errno(errp, "RDMA ERROR: failed to poll cm event");
>
> Hasn't that lost the errno ?

Yes.  My build tree must have lost the ability to compile this file.  I
need to fix that.

Thanks!

Re: [PATCH 12/14] error: Use error_setg_errno() for simplicity and consistency

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Use error_setg_errno() instead of passing the value of strerror() or
>> g_strerror() to error_setg().
>> 
>> The separator between the error message proper and the value of
>> strerror() changes from " : ", "", " - ", "- " to ": " in places.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> @@ -792,9 +792,9 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>>                             VFU_REGION_FLAG_RW | VFU_REGION_FLAG_ALWAYS_CB,
>>                             NULL, 0, -1, 0);
>>      if (ret < 0) {
>> -        error_setg(errp,
>> -                   "vfu: Failed to setup config space handlers for %s- %s",
>> -                   o->device, strerror(errno));
>> +        error_setg_errno(errp,
>> +                         "vfu: Failed to setup config space handlers for %s",
>> +                         o->device);
>
> missing errno.

Yes.

>>          goto fail;
>>      }
>>  
>> @@ -822,8 +822,8 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>>  
>>      ret = vfu_realize_ctx(o->vfu_ctx);
>>      if (ret < 0) {
>> -        error_setg(errp, "vfu: Failed to realize device %s- %s",
>> -                   o->device, strerror(errno));
>> +        error_setg_errno(errp, "vfu: Failed to realize device %s",
>> +                         o->device);
>
> missing errno.

Yes.  Another file my build tree doesn't compile anymore.  Will fix,
thanks!

[...]

Re: [PATCH 11/14] error: Use error_setg_errno() to improve error messages

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> A few error messages show numeric errno codes.  Use error_setg_errno()
>>> to show human-readable text instead.
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> ...
>>
>>> diff --git a/migration/rdma.c b/migration/rdma.c
>>> index 337b415889..ef4885ef5f 100644
>>> --- a/migration/rdma.c
>>> +++ b/migration/rdma.c
>>> @@ -2349,8 +2349,7 @@ static int qemu_get_cm_event_timeout(RDMAContext *rdma,
>>>          error_setg(errp, "RDMA ERROR: poll cm event timeout");
>>>          return -1;
>>>      } else if (ret < 0) {
>>> -        error_setg(errp, "RDMA ERROR: failed to poll cm event, errno=%i",
>>> -                   errno);
>>> +        error_setg_errno(errp, "RDMA ERROR: failed to poll cm event");
>>
>> Hasn't that lost the errno ?
>
> Yes.  My build tree must have lost the ability to compile this file.  I
> need to fix that.

Actually a patch splitting accident.  Fixed.

[...]

Re: [PATCH 04/14] qga: Use error_setg_file_open() for better error messages

[Kostiantyn Kostiuk]

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

Reviewed-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>

On Thu, Nov 20, 2025 at 9:13 PM Markus Armbruster <armbru@redhat.com> wrote:

> Error messages change from
>
>     open("FNAME"): REASON
>
> to
>
>     Could not open 'FNAME': REASON
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qga/commands-linux.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/qga/commands-linux.c b/qga/commands-linux.c
> index 4a09ddc760..5cf76ca2d9 100644
> --- a/qga/commands-linux.c
> +++ b/qga/commands-linux.c
> @@ -1502,14 +1502,15 @@ static void transfer_vcpu(GuestLogicalProcessor
> *vcpu, bool sys2vcpu,
>
>      dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>      if (dirfd == -1) {
> -        error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +        error_setg_file_open(errp, errno, dirpath);
>          return;
>      }
>
>      fd = openat(dirfd, fn, sys2vcpu ? O_RDONLY : O_RDWR);
>      if (fd == -1) {
>          if (errno != ENOENT) {
> -            error_setg_errno(errp, errno, "open(\"%s/%s\")", dirpath, fn);
> +            error_setg_errno(errp, errno, "could not open %s/%s",
> +                             dirpath, fn);
>          } else if (sys2vcpu) {
>              vcpu->online = true;
>              vcpu->can_offline = false;
> @@ -1711,7 +1712,7 @@ static void transfer_memory_block(GuestMemoryBlock
> *mem_blk, bool sys2memblk,
>      dirfd = open(dirpath, O_RDONLY | O_DIRECTORY);
>      if (dirfd == -1) {
>          if (sys2memblk) {
> -            error_setg_errno(errp, errno, "open(\"%s\")", dirpath);
> +            error_setg_file_open(errp, errno, dirpath);
>          } else {
>              if (errno == ENOENT) {
>                  result->response =
> GUEST_MEMORY_BLOCK_RESPONSE_TYPE_NOT_FOUND;
> @@ -1936,7 +1937,7 @@ static GuestDiskStatsInfoList
> *guest_get_diskstats(Error **errp)
>
>      fp = fopen(diskstats, "r");
>      if (fp  == NULL) {
> -        error_setg_errno(errp, errno, "open(\"%s\")", diskstats);
> +        error_setg_file_open(errp, errno, diskstats);
>          return NULL;
>      }
>
> @@ -2047,7 +2048,7 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error
> **errp)
>
>      fp = fopen(cpustats, "r");
>      if (fp  == NULL) {
> -        error_setg_errno(errp, errno, "open(\"%s\")", cpustats);
> +        error_setg_file_open(errp, errno, cpustats);
>          return NULL;
>      }
>
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 3379 bytes --]

Re: [PATCH 12/14] error: Use error_setg_errno() for simplicity and consistency

[Markus Armbruster]

Cc: trimmed

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> Use error_setg_errno() instead of passing the value of strerror() or
>>> g_strerror() to error_setg().
>>> 
>>> The separator between the error message proper and the value of
>>> strerror() changes from " : ", "", " - ", "- " to ": " in places.
>>> 
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>>> @@ -792,9 +792,9 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>>>                             VFU_REGION_FLAG_RW | VFU_REGION_FLAG_ALWAYS_CB,
>>>                             NULL, 0, -1, 0);
>>>      if (ret < 0) {
>>> -        error_setg(errp,
>>> -                   "vfu: Failed to setup config space handlers for %s- %s",
>>> -                   o->device, strerror(errno));
>>> +        error_setg_errno(errp,
>>> +                         "vfu: Failed to setup config space handlers for %s",
>>> +                         o->device);
>>
>> missing errno.
>
> Yes.
>
>>>          goto fail;
>>>      }
>>>  
>>> @@ -822,8 +822,8 @@ static void vfu_object_init_ctx(VfuObject *o, Error **errp)
>>>  
>>>      ret = vfu_realize_ctx(o->vfu_ctx);
>>>      if (ret < 0) {
>>> -        error_setg(errp, "vfu: Failed to realize device %s- %s",
>>> -                   o->device, strerror(errno));
>>> +        error_setg_errno(errp, "vfu: Failed to realize device %s",
>>> +                         o->device);
>>
>> missing errno.
>
> Yes.  Another file my build tree doesn't compile anymore.  Will fix,
> thanks!
>
> [...]

To include it in the build, I need to pass --enable-vfio-user-server to
configure, then install the libaries configure asked for.

Build then fails for me:

    FAILED: subprojects/libvfio-user/test/unit_tests.p/unit-tests.c.o 
    cc -m64 -Isubprojects/libvfio-user/test/unit_tests.p -Isubprojects/libvfio-user/test -I../subprojects/libvfio-user/test -Isubprojects/libvfio-user/include -I../subprojects/libvfio-user/include -Isubprojects/libvfio-user/lib -I../subprojects/libvfio-user/lib -I/usr/include/json-c -fdiagnostics-color=auto -Wall -Winvalid-pch -Wextra -Werror -std=gnu99 -O2 -g -DSTAP_SDT_V2 -mcx16 -msse2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIE -DUNIT_TEST -DWITH_TRAN_PIPE -D_GNU_SOURCE -Werror -DDEBUG -Wno-missing-field-initializers -Wmissing-declarations -Wwrite-strings -MD -MQ subprojects/libvfio-user/test/unit_tests.p/unit-tests.c.o -MF subprojects/libvfio-user/test/unit_tests.p/unit-tests.c.o.d -o subprojects/libvfio-user/test/unit_tests.p/unit-tests.c.o -c ../subprojects/libvfio-user/test/unit-tests.c
    ../subprojects/libvfio-user/test/unit-tests.c: In function ‘test_device_is_stopped_and_copying’:
    ../subprojects/libvfio-user/test/unit-tests.c:585:23: error: storing the address of local variable ‘migration’ in ‘vfu_ctx.migration’ [-Werror=dangling-pointer=]
      585 |     vfu_ctx.migration = &migration;
          |     ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
    ../subprojects/libvfio-user/test/unit-tests.c:584:22: note: ‘migration’ declared here
      584 |     struct migration migration;
          |                      ^~~~~~~~~
    ../subprojects/libvfio-user/test/unit-tests.c:62:18: note: ‘vfu_ctx’ declared here
       62 | static vfu_ctx_t vfu_ctx;
          |                  ^~~~~~~
    cc1: all warnings being treated as errors

and

    FAILED: subprojects/libvfio-user/lib/libvfio-user.so.0.0.1.p/libvfio-user.c.o 
    cc -m64 -Isubprojects/libvfio-user/lib/libvfio-user.so.0.0.1.p -Isubprojects/libvfio-user/lib -I../subprojects/libvfio-user/lib -Isubprojects/libvfio-user/include -I../subprojects/libvfio-user/include -I/usr/include/json-c -fvisibility=hidden -fdiagnostics-color=auto -Wall -Winvalid-pch -Wextra -Werror -std=gnu99 -O2 -g -DSTAP_SDT_V2 -mcx16 -msse2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -ftrivial-auto-var-init=zero -fzero-call-used-regs=used-gpr -fPIC -D_GNU_SOURCE -Werror -DDEBUG -Wno-missing-field-initializers -Wmissing-declarations -Wwrite-strings -MD -MQ subprojects/libvfio-user/lib/libvfio-user.so.0.0.1.p/libvfio-user.c.o -MF subprojects/libvfio-user/lib/libvfio-user.so.0.0.1.p/libvfio-user.c.o.d -o subprojects/libvfio-user/lib/libvfio-user.so.0.0.1.p/libvfio-user.c.o -c ../subprojects/libvfio-user/lib/libvfio-user.c
    ../subprojects/libvfio-user/lib/libvfio-user.c: In function ‘handle_device_get_region_io_fds’:
    ../subprojects/libvfio-user/lib/libvfio-user.c:617:38: error: ‘calloc’ sizes specified with ‘sizeof’ in the earlier argument and not in the later argument [-Werror=calloc-transposed-args]
      617 |         msg->out.fds = calloc(sizeof(int), max_sent_sub_regions);
          |                                      ^~~
    ../subprojects/libvfio-user/lib/libvfio-user.c:617:38: note: earlier argument should specify number of elements, later size of each element
    cc1: all warnings being treated as errors

Re: [PATCH 13/14] qga/commands-win32: Use error_setg_win32() for better error messages

[Kostiantyn Kostiuk]

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

Reviewed-by: Kostiantyn Kostiuk <kkostiuk@redhat.com>

On Thu, Nov 20, 2025 at 9:14 PM Markus Armbruster <armbru@redhat.com> wrote:

> We include numeric GetLastError() codes in error messages in a few
> places, like this:
>
>     error_setg(errp, "GRIPE: %d", (int)GetLastError());
>
> Show text instead, like this:
>
>     error_setg_win32(errp, GetLastError(), "GRIPE");
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qga/commands-win32.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index acc2c11589..0fd0c966e4 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -1798,8 +1798,8 @@ void qmp_guest_set_time(bool has_time, int64_t
> time_ns, Error **errp)
>      tf.dwHighDateTime = (DWORD) (time >> 32);
>
>      if (!FileTimeToSystemTime(&tf, &ts)) {
> -        error_setg(errp, "Failed to convert system time %d",
> -                   (int)GetLastError());
> +        error_setg_win32(errp, GetLastError(),
> +                         "Failed to convert system time");
>          return;
>      }
>
> @@ -1810,7 +1810,8 @@ void qmp_guest_set_time(bool has_time, int64_t
> time_ns, Error **errp)
>      }
>
>      if (!SetSystemTime(&ts)) {
> -        error_setg(errp, "Failed to set time to guest: %d",
> (int)GetLastError());
> +        error_setg_win32(errp, GetLastError(),
> +                         "Failed to set time to guest");
>          return;
>      }
>  }
> @@ -1834,13 +1835,12 @@ GuestLogicalProcessorList
> *qmp_guest_get_vcpus(Error **errp)
>          (length > sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION))) {
>          ptr = pslpi = g_malloc0(length);
>          if (GetLogicalProcessorInformation(pslpi, &length) == FALSE) {
> -            error_setg(&local_err, "Failed to get processor information:
> %d",
> -                       (int)GetLastError());
> +            error_setg_win32(&local_err, GetLastError(),
> +                             "Failed to get processor information");
>          }
>      } else {
> -        error_setg(&local_err,
> -                   "Failed to get processor information buffer length:
> %d",
> -                   (int)GetLastError());
> +        error_setg_win32(&local_err, GetLastError(),
> +                         "Failed to get processor information buffer
> length");
>      }
>
>      while ((local_err == NULL) && (length > 0)) {
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 3403 bytes --]

Re: [PATCH 09/14] error: Use error_setg_file_open() for simplicity and consistency

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
> 
> > * Markus Armbruster (armbru@redhat.com) wrote:
> >> Replace
> >> 
> >>     error_setg_errno(errp, errno, MSG, FNAME);
> >> 
> >> by
> >> 
> >>     error_setg_file_open(errp, errno, FNAME);
> >> 
> >> where MSG is "Could not open '%s'" or similar.
> >> 
> >> Also replace equivalent uses of error_setg().
> >> 
> >> A few messages lose prefixes ("net dump: ", "SEV: ", __func__ ": ").
> >> We could put them back with error_prepend().  Not worth the bother.
> >
> > Yeh, I guess you could just do it with another macro using
> > the same internal function just with string concatenation.
> 
> I'm no fan of such prefixes.  A sign of developers not caring enough to
> craft a good error message for *users*.  *Especially* in the case of
> __func__.
> 
> The error messages changes in question are:
> 
>     net dump: can't open DUMP-FILE: REASON
>     Could not open 'DUMP-FILE': REASON
> 
>     SEV: Failed to open SEV-DEVICE: REASON
>     Could not open 'SEV-DEVICE': REASON
> 
>     sev_common_kvm_init: Failed to open SEV_DEVICE 'REASON'
>     Could not open 'SEV-DEVICE': REASON
> 
> I think these are all improvements, and the loss of the prefix is fine.

Yeh, although I find the error messages aren't just for users;
they're often for the first dev to see it to guess which other
dev to pass the problem to, so a hint about where it's coming
from can be useful.

Dave

> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
> 
> Thanks!
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

Re: [PATCH 09/14] error: Use error_setg_file_open() for simplicity and consistency

[Markus Armbruster]

"Dr. David Alan Gilbert" <dave@treblig.org> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>> 
>> > * Markus Armbruster (armbru@redhat.com) wrote:
>> >> Replace
>> >> 
>> >>     error_setg_errno(errp, errno, MSG, FNAME);
>> >> 
>> >> by
>> >> 
>> >>     error_setg_file_open(errp, errno, FNAME);
>> >> 
>> >> where MSG is "Could not open '%s'" or similar.
>> >> 
>> >> Also replace equivalent uses of error_setg().
>> >> 
>> >> A few messages lose prefixes ("net dump: ", "SEV: ", __func__ ": ").
>> >> We could put them back with error_prepend().  Not worth the bother.
>> >
>> > Yeh, I guess you could just do it with another macro using
>> > the same internal function just with string concatenation.
>> 
>> I'm no fan of such prefixes.  A sign of developers not caring enough to
>> craft a good error message for *users*.  *Especially* in the case of
>> __func__.
>> 
>> The error messages changes in question are:
>> 
>>     net dump: can't open DUMP-FILE: REASON
>>     Could not open 'DUMP-FILE': REASON
>> 
>>     SEV: Failed to open SEV-DEVICE: REASON
>>     Could not open 'SEV-DEVICE': REASON
>> 
>>     sev_common_kvm_init: Failed to open SEV_DEVICE 'REASON'
>>     Could not open 'SEV-DEVICE': REASON
>> 
>> I think these are all improvements, and the loss of the prefix is fine.
>
> Yeh, although I find the error messages aren't just for users;
> they're often for the first dev to see it to guess which other
> dev to pass the problem to, so a hint about where it's coming
> from can be useful.

I agree!  But I think an error message must be make sense to users
*first* and help developers second, and once they make sense to users,
they're often good enough for developers.

The common failures I see happen when developers remain caught in the
developer's perspective, and write something that makes sense to *them*.
Strawman form:

    prefix: failed op[: reason]

where "prefix" is a subsystem tag, or even __func__, and "reason" is
strerror() or similar.

To users, this tends to read as

    gobbledygook: techbabble[: reason]

When we care to replace "failed op" (developer's perspective) by
something that actually makes sense to users, "prefix" often becomes
redundant.

The error messages shown above aren't bad to begin with.  "failed to
open FILE", where FILE is something the user specified, should make
sense to the user.  It should also be good enough for developers even
without a prefix: connecting trouble with the DUMP-FILE to dump /
trouble with the SEV-DEVICE to SEV should be straightforward.

[...]

Re: [PATCH 09/14] error: Use error_setg_file_open() for simplicity and consistency

[BALATON Zoltan]

On Sat, 22 Nov 2025, Markus Armbruster wrote:
> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>>>
>>>> * Markus Armbruster (armbru@redhat.com) wrote:
>>>>> Replace
>>>>>
>>>>>     error_setg_errno(errp, errno, MSG, FNAME);
>>>>>
>>>>> by
>>>>>
>>>>>     error_setg_file_open(errp, errno, FNAME);
>>>>>
>>>>> where MSG is "Could not open '%s'" or similar.
>>>>>
>>>>> Also replace equivalent uses of error_setg().
>>>>>
>>>>> A few messages lose prefixes ("net dump: ", "SEV: ", __func__ ": ").
>>>>> We could put them back with error_prepend().  Not worth the bother.
>>>>
>>>> Yeh, I guess you could just do it with another macro using
>>>> the same internal function just with string concatenation.
>>>
>>> I'm no fan of such prefixes.  A sign of developers not caring enough to
>>> craft a good error message for *users*.  *Especially* in the case of
>>> __func__.
>>>
>>> The error messages changes in question are:
>>>
>>>     net dump: can't open DUMP-FILE: REASON
>>>     Could not open 'DUMP-FILE': REASON
>>>
>>>     SEV: Failed to open SEV-DEVICE: REASON
>>>     Could not open 'SEV-DEVICE': REASON
>>>
>>>     sev_common_kvm_init: Failed to open SEV_DEVICE 'REASON'
>>>     Could not open 'SEV-DEVICE': REASON
>>>
>>> I think these are all improvements, and the loss of the prefix is fine.
>>
>> Yeh, although I find the error messages aren't just for users;
>> they're often for the first dev to see it to guess which other
>> dev to pass the problem to, so a hint about where it's coming
>> from can be useful.
>
> I agree!  But I think an error message must be make sense to users
> *first* and help developers second, and once they make sense to users,
> they're often good enough for developers.
>
> The common failures I see happen when developers remain caught in the
> developer's perspective, and write something that makes sense to *them*.
> Strawman form:
>
>    prefix: failed op[: reason]
>
> where "prefix" is a subsystem tag, or even __func__, and "reason" is
> strerror() or similar.
>
> To users, this tends to read as
>
>    gobbledygook: techbabble[: reason]
>
> When we care to replace "failed op" (developer's perspective) by
> something that actually makes sense to users, "prefix" often becomes
> redundant.
>
> The error messages shown above aren't bad to begin with.  "failed to
> open FILE", where FILE is something the user specified, should make
> sense to the user.  It should also be good enough for developers even
> without a prefix: connecting trouble with the DUMP-FILE to dump /
> trouble with the SEV-DEVICE to SEV should be straightforward.
>
> [...]

I think that

net dump: can't open random-filename: because of some error

shows better where to look for the problem than just

Could not open 'random-filename': because of some error

as the latter does not tell where the file name comes from or what is it. 
It could be added by a management application or added by the users 
randomly without really knowing what they are doing so repeating the 
option or part in the message that the error comes from can help to find 
out where to correct it. Otherwise it might be difficult to guess what 
random-filename is related to if it's not named something you'd expect.

Regards,
BALATON Zoltan

Re: [PATCH 09/14] error: Use error_setg_file_open() for simplicity and consistency

[Dr. David Alan Gilbert]

* BALATON Zoltan (balaton@eik.bme.hu) wrote:
> On Sat, 22 Nov 2025, Markus Armbruster wrote:
> > "Dr. David Alan Gilbert" <dave@treblig.org> writes:
> > 
> > > * Markus Armbruster (armbru@redhat.com) wrote:
> > > > "Dr. David Alan Gilbert" <dave@treblig.org> writes:
> > > > 
> > > > > * Markus Armbruster (armbru@redhat.com) wrote:
> > > > > > Replace
> > > > > > 
> > > > > >     error_setg_errno(errp, errno, MSG, FNAME);
> > > > > > 
> > > > > > by
> > > > > > 
> > > > > >     error_setg_file_open(errp, errno, FNAME);
> > > > > > 
> > > > > > where MSG is "Could not open '%s'" or similar.
> > > > > > 
> > > > > > Also replace equivalent uses of error_setg().
> > > > > > 
> > > > > > A few messages lose prefixes ("net dump: ", "SEV: ", __func__ ": ").
> > > > > > We could put them back with error_prepend().  Not worth the bother.
> > > > > 
> > > > > Yeh, I guess you could just do it with another macro using
> > > > > the same internal function just with string concatenation.
> > > > 
> > > > I'm no fan of such prefixes.  A sign of developers not caring enough to
> > > > craft a good error message for *users*.  *Especially* in the case of
> > > > __func__.
> > > > 
> > > > The error messages changes in question are:
> > > > 
> > > >     net dump: can't open DUMP-FILE: REASON
> > > >     Could not open 'DUMP-FILE': REASON
> > > > 
> > > >     SEV: Failed to open SEV-DEVICE: REASON
> > > >     Could not open 'SEV-DEVICE': REASON
> > > > 
> > > >     sev_common_kvm_init: Failed to open SEV_DEVICE 'REASON'
> > > >     Could not open 'SEV-DEVICE': REASON
> > > > 
> > > > I think these are all improvements, and the loss of the prefix is fine.
> > > 
> > > Yeh, although I find the error messages aren't just for users;
> > > they're often for the first dev to see it to guess which other
> > > dev to pass the problem to, so a hint about where it's coming
> > > from can be useful.
> > 
> > I agree!  But I think an error message must be make sense to users
> > *first* and help developers second, and once they make sense to users,
> > they're often good enough for developers.
> > 
> > The common failures I see happen when developers remain caught in the
> > developer's perspective, and write something that makes sense to *them*.
> > Strawman form:
> > 
> >    prefix: failed op[: reason]
> > 
> > where "prefix" is a subsystem tag, or even __func__, and "reason" is
> > strerror() or similar.
> > 
> > To users, this tends to read as
> > 
> >    gobbledygook: techbabble[: reason]
> > 
> > When we care to replace "failed op" (developer's perspective) by
> > something that actually makes sense to users, "prefix" often becomes
> > redundant.
> > 
> > The error messages shown above aren't bad to begin with.  "failed to
> > open FILE", where FILE is something the user specified, should make
> > sense to the user.  It should also be good enough for developers even
> > without a prefix: connecting trouble with the DUMP-FILE to dump /
> > trouble with the SEV-DEVICE to SEV should be straightforward.
> > 
> > [...]
> 
> I think that
> 
> net dump: can't open random-filename: because of some error
> 
> shows better where to look for the problem than just
> 
> Could not open 'random-filename': because of some error
> 
> as the latter does not tell where the file name comes from or what is it. It
> could be added by a management application or added by the users randomly
> without really knowing what they are doing so repeating the option or part
> in the message that the error comes from can help to find out where to
> correct it. Otherwise it might be difficult to guess what random-filename is
> related to if it's not named something you'd expect.

Yeh agreed.  It very much depends if you think of a 'user' as the person
who typed a qemu command line, or pressed a button on a GUI that triggered
15 levels of abstraction that eventually ran a qemu.

Or for the support person who has a customer saying 'help I've got this error',
and now needs to route it to the network person rather than something else.

Dave

> Regards,
> BALATON Zoltan
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/