Re: [Qemu-devel] [PATCH v2 04/11] monitor: Create MonitorHMP with readline state

[Peter Xu <peterx@redhat.com>]

On Wed, Jun 12, 2019 at 11:07:01AM +0200, Markus Armbruster wrote:

[...]

> > +struct MonitorHMP {
> > +    Monitor common;
> > +    /*
> > +     * State used only in the thread "owning" the monitor.
> > +     * If @use_io_thread, this is @mon_iothread.
> > +     * Else, it's the main thread.
> > +     * These members can be safely accessed without locks.
> > +     */
> > +    ReadLineState *rs;
> > +};
> > +
> 
> Hmm.
> 
> The monitor I/O thread code makes an effort not to restrict I/O thread
> use to QMP, even though we only use it there.  Whether the code would
> actually work for HMP as well we don't know.
> 
> Readline was similar until your PATCH 02: the code made an effort not to
> restrict it to HMP, even though we only use it there.  Whether the code
> would actually work for QMP as well we don't know.
> 
> Should we stop pretending and hard-code "I/O thread only for QMP"?
> 
> If yes, the comment above gets simplified by the patch that hard-codes
> "I/O thread only for QMP".
> 
> If no, we should perhaps point out that we currently don't use an I/O
> thread with HMP.  The comment above seems like a good place for that.

Yes I agree on that if we're refactoring the comment then we can make
it more explicit here.  For my own preference, I would prefer the
latter one, even we can have a bigger comment above MonitorHMP
mentioning that it's only used in main thread so no lock is needed for
all the HMP only structs (until someone wants to hammer on HMP again).

Thanks,

-- 
Peter Xu