Re: [Qemu-devel] [PATCH 09/18] QMP: Introduce command dispatch table

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Wed, 29 Sep 2010 13:31:13 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > Also update QMP functions to use it. The table is generated
>> > from the qemu-monitor-qmp.hx file.
>> >
>> >>From now on, QMP and HMP have different command dispatch
>> > tables.
>> >
>> > Signed-off-by: Luiz Capitulino <address@hidden>
>> > ---
>> >  Makefile.target |    7 +++++--
>> >  monitor.c       |   11 +++++++++--
>> >  2 files changed, 14 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/Makefile.target b/Makefile.target
>> > index a4e80b1..7fab81b 100644
>> > --- a/Makefile.target
>> > +++ b/Makefile.target
>> > @@ -299,7 +299,7 @@ obj-alpha-y = alpha_palcode.o
>> >  
>> >  main.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>> >  
>> > -monitor.o: qemu-monitor.h
>> > +monitor.o: qemu-monitor.h qemu-monitor-qmp.h
>> >  
>> >  $(obj-y) $(obj-$(TARGET_BASE_ARCH)-y): $(GENERATED_HEADERS)
>> >  
>> > @@ -323,10 +323,13 @@ gdbstub-xml.c: $(TARGET_XML_FILES) 
>> > $(SRC_PATH)/feature_to_c.sh
>> >  qemu-monitor.h: $(SRC_PATH)/qemu-monitor.hx
>> >    $(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   
>> > $(TARGET_DIR)$@")
>> >  
>> > +qemu-monitor-qmp.h: $(SRC_PATH)/qemu-monitor-qmp.hx
>> > +  $(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   
>> > $(TARGET_DIR)$@")
>> > +
>> >  clean:
>> >    rm -f *.o *.a *~ $(PROGS) nwfpe/*.o fpu/*.o
>> >    rm -f *.d */*.d tcg/*.o ide/*.o
>> > -  rm -f qemu-monitor.h gdbstub-xml.c
>> > +  rm -f qemu-monitor.h qmp-monitor-qmp.h gdbstub-xml.c
>> 
>> Typo: qmp-monitor-qmp.h instead of qemu-monitor-qmp.h.
>> 
>> I'm not thrilled about qemu-monitor-qmp; because if I expand the
>> abbreviation, I get "QEMU monitor QEMU Monitor Protocol".
>> 
>> What about just qmp?
>
> I think qmp-commands is better, but then I'll also change qemu-monitor.hx to
> hmp-commands.hx.
>
> Objections?

Fine with me.

>> [...]
>> >  install: all
>> >  ifneq ($(PROGS),)
>> > diff --git a/monitor.c b/monitor.c
>> > index 7b699db..fe1a29a 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -189,6 +189,8 @@ static QLIST_HEAD(mon_list, Monitor) mon_list;
>> >  static const mon_cmd_t mon_cmds[];
>> >  static const mon_cmd_t info_cmds[];
>> >  
>> > +static const mon_cmd_t qmp_cmds[];
>> > +
>> >  Monitor *cur_mon;
>> >  Monitor *default_mon;
>> >  
>> > @@ -742,7 +744,7 @@ static void do_info_commands(Monitor *mon, QObject 
>> > **ret_data)
>> >  
>> >      cmd_list = qlist_new();
>> >  
>> > -    for (cmd = mon_cmds; cmd->name != NULL; cmd++) {
>> > +    for (cmd = qmp_cmds; cmd->name != NULL; cmd++) {
>> >          if (monitor_handler_ported(cmd) && !monitor_cmd_user_only(cmd) &&
>> >              !compare_cmd(cmd->name, "info")) {
>> >              qlist_append_obj(cmd_list, get_cmd_dict(cmd->name));
>> 
>> We need to switch here, because "info commands" is about QMP commands,
>> not about HMP commands.  Curious, isn't it?
>
> Drop the compare_cmd() and change the function's name, you mean?

No.  The reason for this change wasn't obvious to me, so I wrote it
down, in case it's not obvious to others, either.  I didn't mean to
suggest you change anything here.

[...]