[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 04/11] monitor: Create MonitorHM
From: |
Peter Xu |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 04/11] monitor: Create MonitorHMP with readline state |
Date: |
Wed, 12 Jun 2019 17:54:14 +0800 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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
[Qemu-block] [PATCH v2 01/11] monitor: Remove unused password prompting fields, Kevin Wolf, 2019/06/11
[Qemu-block] [PATCH v2 03/11] monitor: Make MonitorQMP a child class of Monitor, Kevin Wolf, 2019/06/11