[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/6] qapi: add support for command options
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 1/6] qapi: add support for command options |
Date: |
Tue, 8 May 2012 10:11:47 -0300 |
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.
- [Qemu-devel] [PATCH 0/6]: qemu-ga: no success response for certain commands, Luiz Capitulino, 2012/05/04
- [Qemu-devel] [PATCH 2/6] qemu-ga: don't warn on no command return, Luiz Capitulino, 2012/05/04
- [Qemu-devel] [PATCH 1/6] qapi: add support for command options, Luiz Capitulino, 2012/05/04
- [Qemu-devel] [PATCH 5/6] qemu-ga: guest-suspend-ram: don't emit a success response, Luiz Capitulino, 2012/05/04
- [Qemu-devel] [PATCH 3/6] qemu-ga: guest-shutdown: don't emit a success response, Luiz Capitulino, 2012/05/04
- [Qemu-devel] [PATCH 4/6] qemu-ga: guest-suspend-disk: don't emit a success response, Luiz Capitulino, 2012/05/04
- [Qemu-devel] [PATCH 6/6] qemu-ga: guest-suspend-hybrid: don't emit a success response, Luiz Capitulino, 2012/05/04