Re: [PATCH 1/9] monitor: make error_vprintf_unless_qmp() static

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Thu, Jul 7, 2022 at 4:25 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Not needed outside monitor.c. Remove the needless stub.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  include/monitor/monitor.h | 1 -
>> >  monitor/monitor.c         | 3 ++-
>> >  stubs/error-printf.c      | 5 -----
>> >  3 files changed, 2 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> > index a4b40e8391db..44653e195b45 100644
>> > --- a/include/monitor/monitor.h
>> > +++ b/include/monitor/monitor.h
>> > @@ -56,7 +56,6 @@ void monitor_register_hmp(const char *name, bool info,
>> >  void monitor_register_hmp_info_hrt(const char *name,
>> >                                     HumanReadableText *(*handler)(Error **errp));
>> >
>> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap) G_GNUC_PRINTF(1, 0);
>> >  int error_printf_unless_qmp(const char *fmt, ...) G_GNUC_PRINTF(1, 2);
>> >
>> >  #endif /* MONITOR_H */
>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 86949024f643..ba4c1716a48a 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -273,7 +273,8 @@ int error_vprintf(const char *fmt, va_list ap)
>> >      return vfprintf(stderr, fmt, ap);
>> >  }
>> >
>> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
>> > +G_GNUC_PRINTF(1, 0)
>> > +static int error_vprintf_unless_qmp(const char *fmt, va_list ap)
>> >  {
>> >      Monitor *cur_mon = monitor_cur();
>> >
>> > diff --git a/stubs/error-printf.c b/stubs/error-printf.c
>> > index 0e326d801059..1afa0f62ca26 100644
>> > --- a/stubs/error-printf.c
>> > +++ b/stubs/error-printf.c
>> > @@ -16,8 +16,3 @@ int error_vprintf(const char *fmt, va_list ap)
>> >      }
>> >      return vfprintf(stderr, fmt, ap);
>> >  }
>> > -
>> > -int error_vprintf_unless_qmp(const char *fmt, va_list ap)
>> > -{
>> > -    return error_vprintf(fmt, ap);
>> > -}
>>
>> When I write a printf-like utility function, I habitually throw in a
>> vprintf-like function.
>>
>> Any particular reason for hiding this one?  To avoid misunderstandings:
>> I'm fine with hiding it if it's causing you trouble.
>
> I don't think I had an issue with it, only that I wrote tests for the
> error-report.h API, and didn't see the need to cover a function that isn't
> used outside the unit.

I'd keep it and not worry about missing tests; the tests of
error_printf_unless_qmp() exercise it fine.

>> Except I think we'd better delete than hide then: inline into
>> error_printf_unless_qmp().  Makes sense?
>
> It can't be easily inlined because of the surrounding va_start/va_end

Easily enough, I think:

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 86949024f6..201a672ac6 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -273,27 +273,22 @@ int error_vprintf(const char *fmt, va_list ap)
     return vfprintf(stderr, fmt, ap);
 }
 
-int error_vprintf_unless_qmp(const char *fmt, va_list ap)
-{
-    Monitor *cur_mon = monitor_cur();
-
-    if (!cur_mon) {
-        return vfprintf(stderr, fmt, ap);
-    }
-    if (!monitor_cur_is_qmp()) {
-        return monitor_vprintf(cur_mon, fmt, ap);
-    }
-    return -1;
-}
-
 int error_printf_unless_qmp(const char *fmt, ...)
 {
+    Monitor *cur_mon = monitor_cur();
     va_list ap;
     int ret;
 
     va_start(ap, fmt);
-    ret = error_vprintf_unless_qmp(fmt, ap);
+    if (!cur_mon) {
+        ret = vfprintf(stderr, fmt, ap);
+    } else if (!monitor_cur_is_qmp()) {
+        ret = monitor_vprintf(cur_mon, fmt, ap);
+    } else {
+        ret = -1;
+    }
     va_end(ap);
+
     return ret;
 }