On Mon, 7 May 2012 11:05:36 -0500 Michael Roth <mdr...@linux.vnet.ibm.com> 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 <lcapitul...@redhat.com> > > --- > > 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.