From: Markus Armbruster <armbru@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>,
"Tao Su" <tao1.su@linux.intel.com>,
qemu-devel@nongnu.org, pbonzini@redhat.com, xiaoyao.li@intel.com,
alex.bennee@linaro.org, armbru@redhat.com, philmd@linaro.org
Subject: Re: [PATCH v2] target/i386: Revert monitor_puts() in do_inject_x86_mce()
Date: Wed, 20 Mar 2024 15:34:43 +0100 [thread overview]
Message-ID: <87y1adm0os.fsf@pond.sub.org> (raw)
In-Reply-To: <CAFEAcA8t8C8QCeQS-fxvRKNST80g+84Ggh4B_cZAc6vzHL-wmA@mail.gmail.com> (Peter Maydell's message of "Wed, 20 Mar 2024 14:04:42 +0000")
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.
next prev parent reply other threads:[~2024-03-20 14:35 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-20 8:36 [PATCH v2] target/i386: Revert monitor_puts() in do_inject_x86_mce() Tao Su
2024-03-20 8:47 ` Paolo Bonzini
2024-03-20 13:02 ` Daniel P. Berrangé
2024-03-20 14:04 ` Peter Maydell
2024-03-20 14:34 ` Markus Armbruster [this message]
2024-03-20 15:32 ` Daniel P. Berrangé
2024-03-21 17:16 ` Michael Tokarev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y1adm0os.fsf@pond.sub.org \
--to=armbru@redhat.com \
--cc=alex.bennee@linaro.org \
--cc=berrange@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=tao1.su@linux.intel.com \
--cc=xiaoyao.li@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).