Message ID | 1336162822-13161-2-git-send-email-lcapitulino@redhat.com |
---|---|
State | New |
Headers | show |
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 <lcapitulino@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. option_value_matches() would indicate the purpose better, or option_is_disabled() and then moving the "no"/"false"/"disabled" matching into it. Patch looks good otherwise. Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com> > registry += mcgen(''' > -qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s); > +qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s, %(opts)s); > ''', > - name=cmd['command'], c_name=c_fun(cmd['command'])) > + name=cmd['command'], c_name=c_fun(cmd['command']), > + opts=options) > pop_indent() > ret = mcgen(''' > static void qmp_init_marshal(void) > -- > 1.7.9.2.384.g4a92a >
On Mon, 7 May 2012 11:05:36 -0500 Michael Roth <mdroth@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 <lcapitulino@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.
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' + registry += mcgen(''' -qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s); +qmp_register_command("%(name)s", qmp_marshal_input_%(c_name)s, %(opts)s); ''', - name=cmd['command'], c_name=c_fun(cmd['command'])) + name=cmd['command'], c_name=c_fun(cmd['command']), + opts=options) pop_indent() ret = mcgen(''' static void qmp_init_marshal(void)
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 <lcapitulino@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(-)