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

[Markus Armbruster]

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.