Re: [Qemu-devel] [PATCH 1/7] monitor: Add command_completion callback to mon_cmd_t.

[Luiz Capitulino]

On Thu, 27 Mar 2014 22:55:46 +0100
Hani Benhabiles <address@hidden> wrote:

> On Thu, Mar 27, 2014 at 04:21:51PM -0400, Luiz Capitulino wrote:
> > 
> > I think this patch has to be split into at least three patches. One for
> > the drive_del change (which is unrelated with the other stuff), one
> > converting device_add & device_del and the third one converting
> > object_add and object_del.
> > 
> > Also, please, take the simplest conversion for the first patch, which
> > introduces the callback.
> > 
> 
> Ok, I will split them. Just felt that the number of patches was growing-up too
> fast.

That's not a problem.

> > One more comment below.
> > 
> > > 
> > > Signed-off-by: Hani Benhabiles <address@hidden>
> > > Suggested-by: Luiz Capitulino <address@hidden>
> > > ---
> > >  hmp-commands.hx |  6 +++++-
> > >  hmp.h           |  4 ++++
> > >  monitor.c       | 65 
> > > +++++++++++++++++++++++++++++++++++----------------------
> > >  3 files changed, 49 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > > index f3fc514..4c4d261 100644
> > > --- a/hmp-commands.hx
> > > +++ b/hmp-commands.hx
> > > @@ -176,7 +176,7 @@ ETEXI
> > >  
> > >      {
> > >          .name       = "drive_del",
> > > -        .args_type  = "id:s",
> > > +        .args_type  = "id:B",
> > >          .params     = "device",
> > >          .help       = "remove host block device",
> > >          .user_print = monitor_user_noop,
> > > @@ -658,6 +658,7 @@ ETEXI
> > >          .help       = "add device, like -device on the command line",
> > >          .user_print = monitor_user_noop,
> > >          .mhandler.cmd_new = do_device_add,
> > > +        .command_completion = device_add_completion,
> > >      },
> > >  
> > >  STEXI
> > > @@ -673,6 +674,7 @@ ETEXI
> > >          .params     = "device",
> > >          .help       = "remove device",
> > >          .mhandler.cmd = hmp_device_del,
> > > +        .command_completion = device_del_completion,
> > >      },
> > >  
> > >  STEXI
> > > @@ -1254,6 +1256,7 @@ ETEXI
> > >          .params     = "[qom-type=]type,id=str[,prop=value][,...]",
> > >          .help       = "create QOM object",
> > >          .mhandler.cmd = hmp_object_add,
> > > +        .command_completion = object_add_completion,
> > >      },
> > >  
> > >  STEXI
> > > @@ -1268,6 +1271,7 @@ ETEXI
> > >          .params     = "id",
> > >          .help       = "destroy QOM object",
> > >          .mhandler.cmd = hmp_object_del,
> > > +        .command_completion = object_del_completion,
> > >      },
> > >  
> > >  STEXI
> > > diff --git a/hmp.h b/hmp.h
> > > index ed58f0e..558658f 100644
> > > --- a/hmp.h
> > > +++ b/hmp.h
> > > @@ -92,5 +92,9 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict);
> > >  void hmp_cpu_add(Monitor *mon, const QDict *qdict);
> > >  void hmp_object_add(Monitor *mon, const QDict *qdict);
> > >  void hmp_object_del(Monitor *mon, const QDict *qdict);
> > > +void device_add_completion(Monitor *mon, int nb_args, const char *str);
> > > +void device_del_completion(Monitor *mon, int nb_args, const char *str);
> > > +void object_add_completion(Monitor *mon, int nb_args, const char *str);
> > > +void object_del_completion(Monitor *mon, int nb_args, const char *str);
> > >  
> > >  #endif
> > > diff --git a/monitor.c b/monitor.c
> > > index 342e83b..2c8528c 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -137,6 +137,7 @@ typedef struct mon_cmd_t {
> > >       * used, and mhandler of 1st level plays the role of help function.
> > >       */
> > >      struct mon_cmd_t *sub_table;
> > > +    void (*command_completion)(Monitor *mon, int nb_args, const char 
> > > *str);
> > 
> > Why does command_completion need 'mon'? It seems to me that a ReadLineState
> > suffices.
> 
> No particular reason right now. I just tried to keep the prototype general
> enough in order to not have to make a mass conversion in case some need shows 
> up
> in the future. Should I just use ReadLineState in the next series version ?

IMO, yes. If, in the future, it turns out we need a Monitor object we can
just update the callback signature.