* Markus Armbruster (arm...@redhat.com) wrote: > Kevin Wolf <kw...@redhat.com> writes: > > > Am 12.06.2019 um 11:07 hat Markus Armbruster geschrieben: > >> Cc: Peter for a monitor I/O thread question. > >> > >> Kevin Wolf <kw...@redhat.com> writes: > >> > >> > The ReadLineState in Monitor is only used for HMP monitors. Create > >> > MonitorHMP and move it there. > >> > > >> > Signed-off-by: Kevin Wolf <kw...@redhat.com> > >> > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > > > >> > @@ -218,6 +210,17 @@ struct Monitor { > >> > int mux_out; > >> > }; > >> > > >> > +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. > >> > >> Perhaps restricting readline to HMP should be a separate patch before > >> PATCH 02. > > > > Yes, possibly iothreads could be restricted to QMP. It doesn't help me > > in splitting the monitor in any way, though, so I don't see it within > > the scope of this series. > > That's okay. > > Would you mind pointing out we don't actually use an I/O thread with HMP > in the comment? > > > Keeping readline around for QMP, on the other hand, would probably have > > been harder than making the restriction. > > > > As for splitting patch 2, I don't think that reorganising a patch that > > already does its job and already received review is the most productive > > thing we could do, but if you insist on a separate patch, I can do that. > > No, I don't insist. > > >> > @@ -748,12 +754,13 @@ char *qmp_human_monitor_command(const char > >> > *command_line, bool has_cpu_index, > >> > int64_t cpu_index, Error **errp) > >> > { > >> > char *output = NULL; > >> > - Monitor *old_mon, hmp; > >> > + Monitor *old_mon; > >> > + MonitorHMP hmp = {}; > >> > >> Any particular reason for adding the initializer? > > > > Yes: > > > >> > > >> > - monitor_data_init(&hmp, 0, true, false); > >> > + monitor_data_init(&hmp.common, 0, true, false); > > > > monitor_data_init() does a memset(), but only on hmp.common, so the > > fields outside of hmp.common would remain uniniitialised. Specifically, > > hmp.rs wouldn't be initialised to NULL and attempting to free it in the > > end would crash. > > I see. > > Drop the superfluous memset() in monitor_data_init() then. > > >> > old_mon = cur_mon; > >> > - cur_mon = &hmp; > >> > + cur_mon = &hmp.common; > >> > > >> > if (has_cpu_index) { > >> > int ret = monitor_set_cpu(cpu_index); > > > >> > @@ -1341,16 +1348,19 @@ static void hmp_info_sync_profile(Monitor *mon, > >> > const QDict *qdict) > >> > > >> > static void hmp_info_history(Monitor *mon, const QDict *qdict) > >> > { > >> > + MonitorHMP *hmp_mon = container_of(mon, MonitorHMP, common); > >> > >> Unchecked conversion. Tolerable, I think, since HMP command handlers > >> generally don't get invoked manually, unlike QMP command handlers. > > > > I would like to see all HMP command handlers take MonitorHMP* instead of > > Monitor*, but that would be a big ugly patch touching everything that > > isn't really needed for the goal of this series, so I didn't include it. > > I consider the MonitorHMP job incomplete without it. But it's Dave's > turf.
I'd rather see stuff move forward and then fix that later when someone has the time. Dave > > If you consider it valuable to get rid of this container_of(), that's > > probably the follow-up you could do. > > My recent qemu-common.h pull request temporarily cooled my enthusiasm > for big, ugly patches touching everything... > > >> > @@ -4460,6 +4474,7 @@ static void monitor_qmp_event(void *opaque, int > >> > event) > >> > static void monitor_event(void *opaque, int event) > >> > { > >> > Monitor *mon = opaque; > >> > + MonitorHMP *hmp_mon = container_of(cur_mon, MonitorHMP, common); > >> > >> Any particular reason for changing from @opaque to @cur_mon? > > > > Probably a copy & paste error, thanks for catching it! I'll fix it. > > > >> > @@ -4662,11 +4679,11 @@ static void monitor_init_qmp(Chardev *chr, int > >> > flags) > >> > > >> > static void monitor_init_hmp(Chardev *chr, int flags) > >> > { > >> > - Monitor *mon = g_malloc(sizeof(*mon)); > >> > + MonitorHMP *mon = g_malloc0(sizeof(*mon)); > >> > >> Any particular reason for changing to g_malloc0()? > >> > >> You hid the same change for monitor_init_qmp() in PATCH 03, where I > >> missed it until now. > > > > As above, initialising the fields outside mon->common. > > > > Kevin -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK