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

[Dr. David Alan Gilbert]

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