Re: [Qemu-devel] [PATCH v2 09/11] monitor: Split out monitor/qmp.c

[Kevin Wolf]

Am 13.06.2019 um 07:38 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 12.06.2019 um 15:11 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <address@hidden> writes:
> >> > --- a/include/monitor/monitor.h
> >> > +++ b/include/monitor/monitor.h
> >> > @@ -21,6 +21,7 @@ bool monitor_cur_is_qmp(void);
> >> >  
> >> >  void monitor_init_globals(void);
> >> >  void monitor_init(Chardev *chr, int flags);
> >> > +void monitor_init_qmp(Chardev *chr, int flags);
> >> 
> >> Why does this one go to the non-internal header?
> >
> > Most callers already know whether they want QMP or HMP, so they can just
> > directly create the right thing instead of going through the
> > monitor_init() wrapper.
> >
> > If you prefer, I can move it to the internal header, though. It's not
> > called externally yet.
> 
> As is, monitor_init_qmp() and monitor_init_hmp() are awkward interfaces:
> what if you pass MONITOR_USE_CONTROL to monitor_init_hmp()?
> 
> I can see just one call passing flags that aren't compile-time
> constant.  I think a better interface would be
> 
>     monitor_init_hmp(Chardev *chr);
>     monitor_init_qmp(Chardev *chr, bool pretty);
> 
> replacing monitor_init() entirely.  This is my first preference.
> 
> My (somewhat distant) second is hiding the awkward interfaces in the
> internal header for now, and clean them up later.
> 
> Your choice.

I'm doing both, as in move it to the internal header in the code motion
patches, but add some patches at the end of the series to clean it up.

Kevin