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

* 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   |_______/

"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!

* 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   |_______/

"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.

[...]

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

* 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   |_______/