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

[Markus Armbruster]

Kevin Wolf <address@hidden> writes:

> Am 12.06.2019 um 15:17 hat Markus Armbruster geschrieben:
>> Kevin Wolf <address@hidden> writes:
>> 
>> > Move HMP infrastructure from monitor/misc.c to monitor/hmp.c. This is
>> > code that can be shared for all targets, so compile it only once.
>> >
>> > The amount of function and particularly extern variables in
>> > monitor_int.h is probably a bit larger than it needs to be, but this way
>> > no non-trivial code modifications are needed. The interfaces between HMP
>> > and the monitor core can be cleaned up later.
>> >
>> > Signed-off-by: Kevin Wolf <address@hidden>
>> > Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>> > ---
>> >  include/monitor/monitor.h |    1 +
>> >  monitor/monitor_int.h     |   31 +
>> >  monitor/hmp.c             | 1387 +++++++++++++++++++++++++++++++++++++
>> >  monitor/misc.c            | 1338 +----------------------------------
>> >  monitor/Makefile.objs     |    2 +-
>> >  monitor/trace-events      |    4 +-
>> >  6 files changed, 1429 insertions(+), 1334 deletions(-)
>> >  create mode 100644 monitor/hmp.c
>> >
>> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
>> > index 7bbab05320..8547529e49 100644
>> > --- a/include/monitor/monitor.h
>> > +++ b/include/monitor/monitor.h
>> > @@ -22,6 +22,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);
>> > +void monitor_init_hmp(Chardev *chr, int flags);
>> >  void monitor_cleanup(void);
>> >  
>> >  int monitor_suspend(Monitor *mon);
>> > diff --git a/monitor/monitor_int.h b/monitor/monitor_int.h
>> > index 4aabee54e1..88eaed9c5c 100644
>> > --- a/monitor/monitor_int.h
>> > +++ b/monitor/monitor_int.h
>> > @@ -27,6 +27,7 @@
>> >  
>> >  #include "qemu-common.h"
>> >  #include "monitor/monitor.h"
>> > +#include "qemu/cutils.h"
>> >  
>> >  #include "qapi/qmp/qdict.h"
>> >  #include "qapi/qmp/json-parser.h"
>> > @@ -154,6 +155,29 @@ static inline bool monitor_is_qmp(const Monitor *mon)
>> >      return (mon->flags & MONITOR_USE_CONTROL);
>> >  }
>> >  
>> > +/**
>> > + * Is @name in the '|' separated list of names @list?
>> > + */
>> > +static inline int compare_cmd(const char *name, const char *list)
>> > +{
>> > +    const char *p, *pstart;
>> > +    int len;
>> > +    len = strlen(name);
>> > +    p = list;
>> > +    for (;;) {
>> > +        pstart = p;
>> > +        p = qemu_strchrnul(p, '|');
>> > +        if ((p - pstart) == len && !memcmp(pstart, name, len)) {
>> > +            return 1;
>> > +        }
>> > +        if (*p == '\0') {
>> > +            break;
>> > +        }
>> > +        p++;
>> > +    }
>> > +    return 0;
>> > +}
>> > +
>> 
>> What's the justification for inline?
>
> It seemed small enough, but maybe it isn't (it has also grown by two
> lines after fixing the coding style). I can leave it in misc.c and just
> make it public.

Yes, please.

> I'd just need a more specific name than compare_cmd() to make it public.

Arguably a good idea even for static inlines in an internal header :)

>> >  typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
>> >  extern IOThread *mon_iothread;
>> >  extern QEMUBH *qmp_dispatcher_bh;
>> > @@ -162,6 +186,8 @@ extern QemuMutex monitor_lock;
>> >  extern MonitorList mon_list;
>> >  extern int mon_refcount;
>> >  
>> > +extern mon_cmd_t mon_cmds[];
>> > +
>> 
>> Any particular reason for not moving this one to hmp.c, along with
>> info_cmds?  Question, not demand :)
>
> Yes, it's not part of the core infrastructure, but contains commands
> specific to the system emulator. If a tool were to use HMP, it would
> have to provide its own command tables.
>
> If we ever create a monitor/hmp-sysemu.c or something like it, this
> would be a good place for the tables.

I'm very much in favor of splitting up monitor/misc.c further.  It's
okay to leave that for later, of course.