| Message ID | e86c4ea1f6eb2c2ca9062a58a9bc13232148fd88.1345552661.git.mprivozn@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 21 Aug 2012 14:39:35 +0200 Michal Privoznik <mprivozn@redhat.com> wrote: > This is a useful hint for users (or libvirt) whether a command returns > anything success and hence wait for reply or not. Missing signed-off-by (I think you can add it as a reply to this patch). Patch looks good, it's fixing an important omission from my no-success-response support: Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com> Tested-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > diff to v2: > -fixed typo and field description > > diff to v1: > -changed condition in qmp_command_reports_success per Paolo's advice > > qapi-schema-guest.json | 6 +++++- > qapi/qmp-core.h | 1 + > qapi/qmp-registry.c | 12 ++++++++++++ > qga/commands.c | 1 + > 4 files changed, 19 insertions(+), 1 deletions(-) > > diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json > index d955cf1..60872e5 100644 > --- a/qapi-schema-guest.json > +++ b/qapi-schema-guest.json > @@ -91,10 +91,14 @@ > # > # @enabled: whether command is currently enabled by guest admin > # > +# @success-response: true unless the command does not respond on success > +# See 'guest-shutdown' command for more detailed info. > +# > # Since 1.1.0 > ## > { 'type': 'GuestAgentCommandInfo', > - 'data': { 'name': 'str', 'enabled': 'bool' } } > + 'data': { 'name': 'str', 'enabled': 'bool', > + 'success-response': 'bool'} } > > ## > # @GuestAgentInfo > diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h > index 00446cf..271c3f2 100644 > --- a/qapi/qmp-core.h > +++ b/qapi/qmp-core.h > @@ -48,6 +48,7 @@ QObject *qmp_dispatch(QObject *request); > void qmp_disable_command(const char *name); > void qmp_enable_command(const char *name); > bool qmp_command_is_enabled(const char *name); > +bool qmp_command_reports_success(const char *name); > char **qmp_get_command_list(void); > QObject *qmp_build_error_object(Error *errp); > > diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c > index 5414613..3f7303c 100644 > --- a/qapi/qmp-registry.c > +++ b/qapi/qmp-registry.c > @@ -77,6 +77,18 @@ bool qmp_command_is_enabled(const char *name) > return false; > } > > +bool qmp_command_reports_success(const char *name) { > + QmpCommand *cmd; > + > + QTAILQ_FOREACH(cmd, &qmp_commands, node) { > + if (strcmp(cmd->name, name) == 0) { > + return (cmd->options & QCO_NO_SUCCESS_RESP) == 0; > + } > + } > + > + return true; > +} > + > char **qmp_get_command_list(void) > { > QmpCommand *cmd; > diff --git a/qga/commands.c b/qga/commands.c > index 46b0b08..9811221 100644 > --- a/qga/commands.c > +++ b/qga/commands.c > @@ -63,6 +63,7 @@ struct GuestAgentInfo *qmp_guest_info(Error **err) > cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo)); > cmd_info->name = strdup(*cmd_list); > cmd_info->enabled = qmp_command_is_enabled(cmd_info->name); > + cmd_info->success_response = qmp_command_reports_success(cmd_info->name); > > cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList)); > cmd_info_list->value = cmd_info;
On 21.08.2012 14:39, Michal Privoznik wrote: > This is a useful hint for users (or libvirt) whether a command returns > anything success and hence wait for reply or not. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> > --- > diff to v2: > -fixed typo and field description > > diff to v1: > -changed condition in qmp_command_reports_success per Paolo's advice > > qapi-schema-guest.json | 6 +++++- > qapi/qmp-core.h | 1 + > qapi/qmp-registry.c | 12 ++++++++++++ > qga/commands.c | 1 + > 4 files changed, 19 insertions(+), 1 deletions(-) > > diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json > index d955cf1..60872e5 100644 > --- a/qapi-schema-guest.json > +++ b/qapi-schema-guest.json > @@ -91,10 +91,14 @@ > # > # @enabled: whether command is currently enabled by guest admin > # > +# @success-response: true unless the command does not respond on success > +# See 'guest-shutdown' command for more detailed info. > +# > # Since 1.1.0 > ## > { 'type': 'GuestAgentCommandInfo', > - 'data': { 'name': 'str', 'enabled': 'bool' } } > + 'data': { 'name': 'str', 'enabled': 'bool', > + 'success-response': 'bool'} } > > ## > # @GuestAgentInfo > diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h > index 00446cf..271c3f2 100644 > --- a/qapi/qmp-core.h > +++ b/qapi/qmp-core.h > @@ -48,6 +48,7 @@ QObject *qmp_dispatch(QObject *request); > void qmp_disable_command(const char *name); > void qmp_enable_command(const char *name); > bool qmp_command_is_enabled(const char *name); > +bool qmp_command_reports_success(const char *name); > char **qmp_get_command_list(void); > QObject *qmp_build_error_object(Error *errp); > > diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c > index 5414613..3f7303c 100644 > --- a/qapi/qmp-registry.c > +++ b/qapi/qmp-registry.c > @@ -77,6 +77,18 @@ bool qmp_command_is_enabled(const char *name) > return false; > } > > +bool qmp_command_reports_success(const char *name) { > + QmpCommand *cmd; > + > + QTAILQ_FOREACH(cmd, &qmp_commands, node) { > + if (strcmp(cmd->name, name) == 0) { > + return (cmd->options & QCO_NO_SUCCESS_RESP) == 0; > + } > + } > + > + return true; > +} > + > char **qmp_get_command_list(void) > { > QmpCommand *cmd; > diff --git a/qga/commands.c b/qga/commands.c > index 46b0b08..9811221 100644 > --- a/qga/commands.c > +++ b/qga/commands.c > @@ -63,6 +63,7 @@ struct GuestAgentInfo *qmp_guest_info(Error **err) > cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo)); > cmd_info->name = strdup(*cmd_list); > cmd_info->enabled = qmp_command_is_enabled(cmd_info->name); > + cmd_info->success_response = qmp_command_reports_success(cmd_info->name); > > cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList)); > cmd_info_list->value = cmd_info; >
On Tue, Aug 21, 2012 at 02:39:35PM +0200, Michal Privoznik wrote: > This is a useful hint for users (or libvirt) whether a command returns > anything success and hence wait for reply or not. I'm not necessarilly against this patch, but I think we should clarify it's purpose. Commands that don't return success should still be "waited" on, since they may still return an error. You can only stop waiting once you recieve an error, a client-side timeout, or some "external" indicator in the case of things like shutdown and suspend. For guest-suspend*, this has always been the case, so there's no reason to probe for their behavior. For guest-shutdown, the semantics did change, but were considered backward-compatible in that the new behavior would trigger a client-side timeout for older clients, which was always a required aspect of the protocol (since it's a connectionless protocol, possibly malicious agent, etc), and the success response was always documented as async and do-nothing. So we only need to worry about how new clients deal with guest-shutdown, Specifically, whether or not we will get a success response from the agent. But since the success response was always meaningless, why couldn't we just throw it away and continue waiting for a shutdown event from QMP, or querying query-status for runstate change? Would this solve the ambiguity? If so, we can get just update the guest-shutdown documentation to detail how to handle "success" responses from older agents (ignore them, basically). I think this is a nicer way to handle newer clients than adding more things to probe for. > --- > diff to v2: > -fixed typo and field description > > diff to v1: > -changed condition in qmp_command_reports_success per Paolo's advice > > qapi-schema-guest.json | 6 +++++- > qapi/qmp-core.h | 1 + > qapi/qmp-registry.c | 12 ++++++++++++ > qga/commands.c | 1 + > 4 files changed, 19 insertions(+), 1 deletions(-) > > diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json > index d955cf1..60872e5 100644 > --- a/qapi-schema-guest.json > +++ b/qapi-schema-guest.json > @@ -91,10 +91,14 @@ > # > # @enabled: whether command is currently enabled by guest admin > # > +# @success-response: true unless the command does not respond on success > +# See 'guest-shutdown' command for more detailed info. > +# > # Since 1.1.0 > ## > { 'type': 'GuestAgentCommandInfo', > - 'data': { 'name': 'str', 'enabled': 'bool' } } > + 'data': { 'name': 'str', 'enabled': 'bool', > + 'success-response': 'bool'} } > > ## > # @GuestAgentInfo > diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h > index 00446cf..271c3f2 100644 > --- a/qapi/qmp-core.h > +++ b/qapi/qmp-core.h > @@ -48,6 +48,7 @@ QObject *qmp_dispatch(QObject *request); > void qmp_disable_command(const char *name); > void qmp_enable_command(const char *name); > bool qmp_command_is_enabled(const char *name); > +bool qmp_command_reports_success(const char *name); > char **qmp_get_command_list(void); > QObject *qmp_build_error_object(Error *errp); > > diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c > index 5414613..3f7303c 100644 > --- a/qapi/qmp-registry.c > +++ b/qapi/qmp-registry.c > @@ -77,6 +77,18 @@ bool qmp_command_is_enabled(const char *name) > return false; > } > > +bool qmp_command_reports_success(const char *name) { > + QmpCommand *cmd; > + > + QTAILQ_FOREACH(cmd, &qmp_commands, node) { > + if (strcmp(cmd->name, name) == 0) { > + return (cmd->options & QCO_NO_SUCCESS_RESP) == 0; > + } > + } > + > + return true; > +} > + > char **qmp_get_command_list(void) > { > QmpCommand *cmd; > diff --git a/qga/commands.c b/qga/commands.c > index 46b0b08..9811221 100644 > --- a/qga/commands.c > +++ b/qga/commands.c > @@ -63,6 +63,7 @@ struct GuestAgentInfo *qmp_guest_info(Error **err) > cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo)); > cmd_info->name = strdup(*cmd_list); > cmd_info->enabled = qmp_command_is_enabled(cmd_info->name); > + cmd_info->success_response = qmp_command_reports_success(cmd_info->name); > > cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList)); > cmd_info_list->value = cmd_info; > -- > 1.7.8.6 > >
diff to v2: -fixed typo and field description diff to v1: -changed condition in qmp_command_reports_success per Paolo's advice qapi-schema-guest.json | 6 +++++- qapi/qmp-core.h | 1 + qapi/qmp-registry.c | 12 ++++++++++++ qga/commands.c | 1 + 4 files changed, 19 insertions(+), 1 deletions(-) diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json index d955cf1..60872e5 100644 --- a/qapi-schema-guest.json +++ b/qapi-schema-guest.json @@ -91,10 +91,14 @@ # # @enabled: whether command is currently enabled by guest admin # +# @success-response: true unless the command does not respond on success +# See 'guest-shutdown' command for more detailed info. +# # Since 1.1.0 ## { 'type': 'GuestAgentCommandInfo', - 'data': { 'name': 'str', 'enabled': 'bool' } } + 'data': { 'name': 'str', 'enabled': 'bool', + 'success-response': 'bool'} } ## # @GuestAgentInfo diff --git a/qapi/qmp-core.h b/qapi/qmp-core.h index 00446cf..271c3f2 100644 --- a/qapi/qmp-core.h +++ b/qapi/qmp-core.h @@ -48,6 +48,7 @@ QObject *qmp_dispatch(QObject *request); void qmp_disable_command(const char *name); void qmp_enable_command(const char *name); bool qmp_command_is_enabled(const char *name); +bool qmp_command_reports_success(const char *name); char **qmp_get_command_list(void); QObject *qmp_build_error_object(Error *errp); diff --git a/qapi/qmp-registry.c b/qapi/qmp-registry.c index 5414613..3f7303c 100644 --- a/qapi/qmp-registry.c +++ b/qapi/qmp-registry.c @@ -77,6 +77,18 @@ bool qmp_command_is_enabled(const char *name) return false; } +bool qmp_command_reports_success(const char *name) { + QmpCommand *cmd; + + QTAILQ_FOREACH(cmd, &qmp_commands, node) { + if (strcmp(cmd->name, name) == 0) { + return (cmd->options & QCO_NO_SUCCESS_RESP) == 0; + } + } + + return true; +} + char **qmp_get_command_list(void) { QmpCommand *cmd; diff --git a/qga/commands.c b/qga/commands.c index 46b0b08..9811221 100644 --- a/qga/commands.c +++ b/qga/commands.c @@ -63,6 +63,7 @@ struct GuestAgentInfo *qmp_guest_info(Error **err) cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo)); cmd_info->name = strdup(*cmd_list); cmd_info->enabled = qmp_command_is_enabled(cmd_info->name); + cmd_info->success_response = qmp_command_reports_success(cmd_info->name); cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList)); cmd_info_list->value = cmd_info;