From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35813) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3rRJ-0007Ar-R4 for qemu-devel@nongnu.org; Tue, 21 Aug 2012 12:36:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T3rRI-0006Z4-3F for qemu-devel@nongnu.org; Tue, 21 Aug 2012 12:36:25 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:48687) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T3rRH-0006Yk-SE for qemu-devel@nongnu.org; Tue, 21 Aug 2012 12:36:24 -0400 Received: by obbta14 with SMTP id ta14so10949824obb.4 for ; Tue, 21 Aug 2012 09:36:23 -0700 (PDT) Sender: fluxion Date: Tue, 21 Aug 2012 11:30:06 -0500 From: Michael Roth Message-ID: <20120821163006.GB6071@illuin> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH for-1.2 v3] qemu-ga: report success-response field in guest-info List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michal Privoznik Cc: qemu-devel@nongnu.org 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 > >