Re: [Qemu-devel] [PATCH for-1.2 v3] qemu-ga: report success-response field in guest-info

[Michael Roth]

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
> 
>