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.

Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Tao Su <tao1.su@linux.intel.com>
---
v1 -> v2:
- Instead revert the broken part of commit bf0c50d4aa85
- Add Markus's Reviewed-by

v1:
- https://lore.kernel.org/all/20240320052118.520378-1-tao1.su@linux.intel.com/
---
 target/i386/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/i386/helper.c b/target/i386/helper.c
index 2070dd0dda..23ccb23a5b 100644
--- a/target/i386/helper.c
+++ b/target/i386/helper.c
@@ -430,7 +430,7 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
         if (need_reset) {
             emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
                                       recursive);
-            monitor_puts(params->mon, msg);
+            monitor_printf(params->mon, "%s", msg);
             qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
             qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
             return;

base-commit: c62d54d0a8067ffb3d5b909276f7296d7df33fa7
-- 
2.34.1

20.03.2024 11:36, Tao Su :
> 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.
> 
> Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Tao Su <tao1.su@linux.intel.com>

Smells like a -stable material, is it not?

Thanks,

/mjt

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.

> 
> Fixes: bf0c50d4aa85 (monitor: expose monitor_puts to rest of code)
> Reviwed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Tao Su <tao1.su@linux.intel.com>
> ---
> v1 -> v2:
> - Instead revert the broken part of commit bf0c50d4aa85
> - Add Markus's Reviewed-by
> 
> v1:
> - https://lore.kernel.org/all/20240320052118.520378-1-tao1.su@linux.intel.com/
> ---
>  target/i386/helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/helper.c b/target/i386/helper.c
> index 2070dd0dda..23ccb23a5b 100644
> --- a/target/i386/helper.c
> +++ b/target/i386/helper.c
> @@ -430,7 +430,7 @@ static void do_inject_x86_mce(CPUState *cs, run_on_cpu_data data)
>          if (need_reset) {
>              emit_guest_memory_failure(MEMORY_FAILURE_ACTION_RESET, ar,
>                                        recursive);
> -            monitor_puts(params->mon, msg);
> +            monitor_printf(params->mon, "%s", msg);
>              qemu_log_mask(CPU_LOG_RESET, "%s\n", msg);
>              qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>              return;
> 
> base-commit: c62d54d0a8067ffb3d5b909276f7296d7df33fa7
> -- 
> 2.34.1
> 
> 

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 :|

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()...

-- PMM

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.

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 :|

Queued, thanks.

Paolo