Re: [PATCH v2] target/i386: Revert monitor_puts() in do_inject_x86_mce()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Mar 20, 2024 at 03:34:43PM +0100, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On Wed, 20 Mar 2024 at 13:03, Daniel P. Berrangé <berrange@redhat.com> wrote:
> >>
> >> On Wed, Mar 20, 2024 at 04:36:40PM +0800, Tao Su wrote:
> >> > monitor_puts() doesn't check the monitor pointer, but do_inject_x86_mce()
> >> > may have a parameter with NULL monitor pointer. Revert monitor_puts() in
> >> > do_inject_x86_mce() to fix, then the fact that we send the same message to
> >> > monitor and log is again more obvious.
> >>
> >> Yikes, why do we have such a horrible trap-door in our
> >> monitor output APIs.
> >>
> >> Isn't the right fix here to make 'monitor_puts' check for
> >> NULL & be a no-op, in the same way 'monitor_printf' does,
> >> so the APIs have consistent behaviour.
> >
> > The other difference between monitor_puts(mon, s) and
> > monitor_printf(mon, "%s", s)
> > is that the latter will return an error if the monitor is QMP, whereas
> > the former will go ahead and print the message anyway. That one is
> > awkward to resolve, because the mechanism the QMP monitor uses to
> > print the JSON in qmp_send_response() is to call monitor_puts()...
> 
> We need a low-level function to send to a monitor, be it HMP or QMP:
> monitor_puts().
> 
> We need a high-level function to format JSON and send it to QMP:
> qmp_send_response().
> 
> We need a high-level functions to format text and send it to HMP:
> monitor_printf(), ...
> 
> Trouble is the first and the last one are deceptively named.  The names
> suggest monitor_printf() is to monitor_puts() what printf() is to
> puts().  Not true.
> 
> Naming the functions that expect an HMP monitor hmp_FOO() would make
> more sense.  Renaming them now would be quite some churn, though.

How about a simpler alternative.

 * Rename monitor_puts to monitor_puts_internal and it is
   in monitor-internal.h

 * Change low level users (whcih are all inside monitor/)
   to use monitor_puts_internal

 * Introduce a new monitors_puts, which is to monitor_printf
   what puts() is to printf()

Most (all?) usage outside monitor/ appears to be HMP only.

eg this patch:

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 965f5d5450..8e5d0cc71c 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -40,7 +40,6 @@ void monitor_flush(Monitor *mon);
 int monitor_set_cpu(Monitor *mon, int cpu_index);
 int monitor_get_cpu_index(Monitor *mon);
 
-int monitor_puts_locked(Monitor *mon, const char *str);
 void monitor_flush_locked(Monitor *mon);
 
 void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, uint64_t size, Error **errp);
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index 252de85681..972e7f96d9 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -188,4 +188,7 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const char *name);
 void handle_hmp_command(MonitorHMP *mon, const char *cmdline);
 int hmp_compare_cmd(const char *name, const char *list);
 
+int monitor_puts_internal(Monitor *mon, const char *str);
+int monitor_puts_locked(Monitor *mon, const char *str);
+
 #endif
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 01ede1babd..c0ec5bc03e 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -206,12 +206,25 @@ int monitor_puts_locked(Monitor *mon, const char *str)
     return i;
 }
 
-int monitor_puts(Monitor *mon, const char *str)
+int monitor_puts_internal(Monitor *mon, const char *str)
 {
     QEMU_LOCK_GUARD(&mon->mon_lock);
     return monitor_puts_locked(mon, str);
 }
 
+int monitor_puts(Monitor *mon, const char *str)
+{
+    if (!mon) {
+        return -1;
+    }
+
+    if (monitor_is_qmp(mon)) {
+        return -1;
+    }
+
+    return monitor_puts_internal(mon, str);
+}
+
 int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
 {
     char *buf;
@@ -226,7 +239,7 @@ int monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     }
 
     buf = g_strdup_vprintf(fmt, ap);
-    n = monitor_puts(mon, buf);
+    n = monitor_puts_internal(mon, buf);
     g_free(buf);
     return n;
 }
diff --git a/monitor/qmp.c b/monitor/qmp.c
index a239945e8d..4d848ee91c 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -139,7 +139,7 @@ void qmp_send_response(MonitorQMP *mon, const QDict *rsp)
     trace_monitor_qmp_respond(mon, json->str);
 
     g_string_append_c(json, '\n');
-    monitor_puts(&mon->common, json->str);
+    monitor_puts_internal(&mon->common, json->str);
 
     g_string_free(json, true);
 }


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|