Re: [PATCH kvmtool 5/6] util: Allow die_perror() to take a variable list of argument

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Will,

On Sun, May 17, 2026 at 09:44:30AM +0100, Will Deacon wrote:
> On Mon, Mar 23, 2026 at 03:02:20PM +0000, Alexandru Elisei wrote:
> > Make die_perror() more similar to die() by allowing the user to specify
> > a format string and arguments.
> > 
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  include/kvm/util.h |  2 +-
> >  util/util.c        | 17 +++++++++++++++--
> >  2 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/kvm/util.h b/include/kvm/util.h
> > index ac8515f8c46b..a6dba1147d68 100644
> > --- a/include/kvm/util.h
> > +++ b/include/kvm/util.h
> > @@ -41,7 +41,7 @@
> >  #define MAP_ANON_NORESERVE (MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE)
> >  
> >  extern void die(const char *err, ...) NORETURN __attribute__((format (printf, 1, 2)));
> > -extern void die_perror(const char *s) NORETURN;
> > +extern void die_perror(const char *fmt, ...) NORETURN __attribute__((format (printf, 1, 2)));
> >  extern void pr_err(const char *err, ...) __attribute__((format (printf, 1, 2)));
> >  extern void pr_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
> >  extern void pr_info(const char *err, ...) __attribute__((format (printf, 1, 2)));
> > diff --git a/util/util.c b/util/util.c
> > index fc732200f2dc..0f116e0ac459 100644
> > --- a/util/util.c
> > +++ b/util/util.c
> > @@ -1,6 +1,7 @@
> >  /*
> >   * Taken from perf which in turn take it from GIT
> >   */
> > +#include <stdio.h>
> >  
> >  #include "kvm/util.h"
> >  
> > @@ -98,11 +99,23 @@ void __pr_debug(const char *debug, ...)
> >  	va_end(params);
> >  }
> >  
> > -void die_perror(const char *s)
> > +void die_perror(const char *fmt, ...)
> >  {
> > +	char buf[1024];
> > +	va_list params;
> >  	int e = errno;
> > +	int ret;
> > +
> > +	va_start(params, fmt);
> > +	ret = vsnprintf(buf, sizeof(buf), fmt, params);
> > +	if (ret < 0) {
> > +		e = errno;
> > +		strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
> 
> That looks really confusing to me as we're effectively hiding the real
> error that caused us to get here just because we couldn't pretty-print
> the error string! In the unlikely case that vsnprintf() fails, I think
> we should preserve the original errno and either print a string to say
> that the formatting failed _or_ pretty-print the original error with
> strerror().

You have a good point.

I'm not exactly sure what you mean by pretty-printing the original error.
Do you mean that we should print the message passed to die_perror()? I'm
don't know how I can do that reliably. I could use vprintf(), but if
vsnprintf() failed, wouldn't that make it likely that vprintf() will also
fail?

How about this:

diff --git a/util/util.c b/util/util.c
index 0f116e0ac459..4e3704f781e8 100644
--- a/util/util.c
+++ b/util/util.c
@@ -109,8 +109,9 @@ void die_perror(const char *fmt, ...)
        va_start(params, fmt);
        ret = vsnprintf(buf, sizeof(buf), fmt, params);
        if (ret < 0) {
-               e = errno;
-               strncpy(buf, "vsnprintf", ARRAY_SIZE(buf) - 1);
+               fprintf(stderr, "vsnprintf failed with error %d (%s)\n",
+                       errno, strerror(errno));
+               strncpy(buf, "Error", ARRAY_SIZE(buf) - 1);
        }
        va_end(params);

but we're still losing the original message.

Thanks,
Alex