Re: [Qemu-devel] [PATCH 11/15] monitor error: Make printf()-like functions return a value

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > printf() & friends return the number of characters written on success,
> > negative value on error.
> >
> > monitor_printf(), monitor_vfprintf(), monitor_vprintf(),
> > error_printf(), error_printf_unless_qmp(), error_vprintf(), and
> > error_vprintf_unless_qmp() return void.  Some of them carry a TODO
> > comment asking for int instead.
> >
> > Improve them to return int like printf() does.
> >
> > This makes our use of monitor_printf() as fprintf_function slightly
> > less dirty: the function cast no longer adds a return value that isn't
> > there.  It still changes a parameter's pointer type.  That will be
> > addressed in a future commit.
> >
> > monitor_vfprintf() always returns zero.  Improve it to return the
> > proper value.
> >
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> [...]
> > diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> > index 99c6406668..537298209d 100644
> > --- a/stubs/error-printf.c
> > +++ b/stubs/error-printf.c
> > @@ -2,19 +2,19 @@
> >  #include "qemu-common.h"
> >  #include "qemu/error-report.h"
> >  
> > -void error_vprintf(const char *fmt, va_list ap)
> > +int error_vprintf(const char *fmt, va_list ap)
> >  {
> >      if (g_test_initialized() && !g_test_subprocess() &&
> >          getenv("QTEST_SILENT_ERRORS")) {
> >          char *msg = g_strdup_vprintf(fmt, ap);
> >          g_test_message("%s", msg);
> >          g_free(msg);
> > -    } else {
> > -        vfprintf(stderr, fmt, ap);
> > +        return strlen(msg);
> 
> Use after free.  Obvious fixup appended.
> 
> >      }
> > +    return vfprintf(stderr, fmt, ap);
> >  }
> >  
> > -void error_vprintf_unless_qmp(const char *fmt, va_list ap)
> > +int error_vprintf_unless_qmp(const char *fmt, va_list ap)
> >  {
> > -    error_vprintf(fmt, ap);
> > +    return error_vprintf(fmt, ap);
> >  }
> [...]
> 
> diff --git a/stubs/error-printf.c b/stubs/error-printf.c
> index 537298209d..1f9d3b3714 100644
> --- a/stubs/error-printf.c
> +++ b/stubs/error-printf.c
> @@ -4,12 +4,15 @@
>  
>  int error_vprintf(const char *fmt, va_list ap)
>  {
> +    int ret;
> +
>      if (g_test_initialized() && !g_test_subprocess() &&
>          getenv("QTEST_SILENT_ERRORS")) {
>          char *msg = g_strdup_vprintf(fmt, ap);
>          g_test_message("%s", msg);
> +        ret = strlen(msg);
>          g_free(msg);
> -        return strlen(msg);
> +        return ret;
>      }
>      return vfprintf(stderr, fmt, ap);
>  }

Yeh, with the fixup:

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK