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

[Kevin Wolf <kwolf@redhat.com>]

Am 12.06.2019 um 11:07 hat Markus Armbruster geschrieben:
> Cc: Peter for a monitor I/O thread question.
> 
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > The ReadLineState in Monitor is only used for HMP monitors. Create
> > MonitorHMP and move it there.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@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.

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.

> > @@ -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.

> >      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.

If you consider it valuable to get rid of this container_of(), that's
probably the follow-up you could do.

> > @@ -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