Re: [Qemu-devel] [PATCH 1/6] qapi: add support for command options

[Luiz Capitulino]

On Mon, 7 May 2012 11:05:36 -0500
Michael Roth <address@hidden> wrote:

> On Fri, May 04, 2012 at 05:20:17PM -0300, Luiz Capitulino wrote:
> > Options allow for changes in commands behavior. This commit introduces
> > the QCO_NO_SUCCESS_RESP option, which causes a command to not emit a
> > success response.
> > 
> > This is needed by commands such as qemu-ga's guest-shutdown, which
> > may not be able to complete before the VM vanishes. In this case, it's
> > useful and simpler not to bother sending a success response.
> > 
> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >  qapi/qmp-core.h          |   10 +++++++++-
> >  qapi/qmp-dispatch.c      |    8 ++++++--
> >  qapi/qmp-registry.c      |    4 +++-
> >  scripts/qapi-commands.py |   14 ++++++++++++--
> >  4 files changed, 30 insertions(+), 6 deletions(-)
> > 
> > diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h
> > index 431ddbb..b0f64ba 100644
> > --- a/qapi/qmp-core.h
> > +++ b/qapi/qmp-core.h
> > @@ -25,16 +25,24 @@ typedef enum QmpCommandType
> >      QCT_NORMAL,
> >  } QmpCommandType;
> > 
> > +typedef enum QmpCommandOptions
> > +{
> > +    QCO_NO_OPTIONS = 0x0,
> > +    QCO_NO_SUCCESS_RESP = 0x1,
> > +} QmpCommandOptions;
> > +
> >  typedef struct QmpCommand
> >  {
> >      const char *name;
> >      QmpCommandType type;
> >      QmpCommandFunc *fn;
> > +    QmpCommandOptions options;
> >      QTAILQ_ENTRY(QmpCommand) node;
> >      bool enabled;
> >  } QmpCommand;
> > 
> > -void qmp_register_command(const char *name, QmpCommandFunc *fn);
> > +void qmp_register_command(const char *name, QmpCommandFunc *fn,
> > +                          QmpCommandOptions options);
> >  QmpCommand *qmp_find_command(const char *name);
> >  QObject *qmp_dispatch(QObject *request);
> >  void qmp_disable_command(const char *name);
> > diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> > index 43f640a..122c1a2 100644
> > --- a/qapi/qmp-dispatch.c
> > +++ b/qapi/qmp-dispatch.c
> > @@ -94,8 +94,12 @@ static QObject *do_qmp_dispatch(QObject *request, Error 
> > **errp)
> >      switch (cmd->type) {
> >      case QCT_NORMAL:
> >          cmd->fn(args, &ret, errp);
> > -        if (!error_is_set(errp) && ret == NULL) {
> > -            ret = QOBJECT(qdict_new());
> > +        if (!error_is_set(errp)) {
> > +            if (cmd->options & QCO_NO_SUCCESS_RESP) {
> > +                g_assert(!ret);
> > +            } else if (!ret) {
> > +                ret = QOBJECT(qdict_new());
> > +            }
> >          }
> >          break;
> >      }
> > diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c
> > index 43d5cde..5414613 100644
> > --- a/qapi/qmp-registry.c
> > +++ b/qapi/qmp-registry.c
> > @@ -17,7 +17,8 @@
> >  static QTAILQ_HEAD(QmpCommandList, QmpCommand) qmp_commands =
> >      QTAILQ_HEAD_INITIALIZER(qmp_commands);
> > 
> > -void qmp_register_command(const char *name, QmpCommandFunc *fn)
> > +void qmp_register_command(const char *name, QmpCommandFunc *fn,
> > +                          QmpCommandOptions options)
> >  {
> >      QmpCommand *cmd = g_malloc0(sizeof(*cmd));
> > 
> > @@ -25,6 +26,7 @@ void qmp_register_command(const char *name, 
> > QmpCommandFunc *fn)
> >      cmd->type = QCT_NORMAL;
> >      cmd->fn = fn;
> >      cmd->enabled = true;
> > +    cmd->options = options;
> >      QTAILQ_INSERT_TAIL(&qmp_commands, cmd, node);
> >  }
> > 
> > diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> > index 0b4f0a0..e746333 100644
> > --- a/scripts/qapi-commands.py
> > +++ b/scripts/qapi-commands.py
> > @@ -291,14 +291,24 @@ out:
> > 
> >      return ret
> > 
> > +def option_is_enabled(opt, val, cmd):
> > +    if opt in cmd and cmd[opt] == val:
> > +        return True
> > +    return False
> > +
> >  def gen_registry(commands):
> >      registry=""
> >      push_indent()
> >      for cmd in commands:
> > +        options = 'QCO_NO_OPTIONS'
> > +        if option_is_enabled('success-response', 'no', cmd):
> > +            options = 'QCO_NO_SUCCESS_RESP'
> > +
> 
> Hate to hold things up for a nit, but the naming of option_is_enabled()
> is just plain wrong here. option_is_enabled() is returning true for a
> case where we're actually checking for an option being disabled. I'm
> guessing it looks this way because we're trying to determine if the
> internal QCO_NO_SUCCESS_RESP option is enabled, but option_is_enabled()
> only has knowledge of the QAPI directive so I think that's backwards.

Agreed.

> option_value_matches() would indicate the purpose better, or
> option_is_disabled() and then moving the "no"/"false"/"disabled"
> matching into it.

I like option_value_matches(), will address this and the other comments and
resend.

Btw, I expect this series and my next one (which makes guest-shutdown and
guest-suspend-* synchronous) to go through your tree. Also note that they're
1.1 material.