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

[Kevin Wolf <kwolf@redhat.com>]

Am 12.06.2019 um 16:08 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@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 <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.
> 
> That's okay.
> 
> Would you mind pointing out we don't actually use an I/O thread with HMP
> in the comment?

I do mind in a way (git doesn't really cope well with changing things in
the first patches of this series while the later patches move them to
different files - renaming mon_cmds resulted in a big mess and now I'm
kind of fed up with this kind of merge conflicts), but I'll do it
anyway. As long as it's only one line, it shouldn't be that hard to make
sure that it still exists at the end of the series...

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

Hm, yes, all callers already initialise the memory now, so that can be
done.

Kevin