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

[Markus Armbruster]

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.

> 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