On Wed, Mar 20, 2024 at 03:34:43PM +0100, Markus Armbruster wrote:
> Peter Maydell <peter.mayd...@linaro.org> writes:
>
> > On Wed, 20 Mar 2024 at 13:03, Daniel P. Berrangé <berra...@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 :|
