[Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Mark Wu]

Now we have several qemu-ga commands not returning response on success.
It has been documented in qga/qapi-schema.json already. This patch exposes
the 'success-response' flag by extending 'guest-info' command. With this
change, the clients can handle the command response more flexibly.

Changes:
	v2: add the notation 'since 1.7' to the option 'success-response'
        (per Eric Blake's comments)

Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
---
 include/qapi/qmp/dispatch.h |  1 +
 qapi/qmp-registry.c         | 13 +++++++++++++
 qga/commands.c              |  2 ++
 qga/qapi-schema.json        |  5 ++++-
 4 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1ce11f5..f08c1dd 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.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_has_success_response(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 28bbbe8..f86c3a5 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -79,6 +79,19 @@ bool qmp_command_is_enabled(const char *name)
     return false;
 }
 
+bool qmp_command_has_success_response(const char *name)
+{
+    QmpCommand *cmd;
+
+    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
+        if (strcmp(cmd->name, name) == 0) {
+            return cmd->options != QCO_NO_SUCCESS_RESP;
+        }
+    }
+
+    return false;
+}
+
 char **qmp_get_command_list(void)
 {
     QmpCommand *cmd;
diff --git a/qga/commands.c b/qga/commands.c
index 528b082..0df910b 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
         cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
         cmd_info->name = g_strdup(*cmd_list);
         cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
+        cmd_info->success_response =
+            qmp_command_has_success_response(cmd_info->name);
 
         cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList));
         cmd_info_list->value = cmd_info;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 7155b7a..9c6a58f 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -141,10 +141,13 @@
 #
 # @enabled: whether command is currently enabled by guest admin
 #
+# @success-response: whether command returns a response on success
+#                    and only available since 1.7
+#
 # Since 1.1.0
 ##
 { 'type': 'GuestAgentCommandInfo',
-  'data': { 'name': 'str', 'enabled': 'bool' } }
+  'data': { 'name': 'str', 'enabled': 'bool', 'success-response': 'bool' } }
 
 ##
 # @GuestAgentInfo
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Luiz Capitulino]

On Sun, 22 Sep 2013 14:50:54 +0800
Mark Wu <wudxw@linux.vnet.ibm.com> wrote:

> Now we have several qemu-ga commands not returning response on success.
> It has been documented in qga/qapi-schema.json already. This patch exposes
> the 'success-response' flag by extending 'guest-info' command. With this
> change, the clients can handle the command response more flexibly.
> 
> Changes:
> 	v2: add the notation 'since 1.7' to the option 'success-response'
>         (per Eric Blake's comments)
> 
> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  include/qapi/qmp/dispatch.h |  1 +
>  qapi/qmp-registry.c         | 13 +++++++++++++
>  qga/commands.c              |  2 ++
>  qga/qapi-schema.json        |  5 ++++-
>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1ce11f5..f08c1dd 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.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_has_success_response(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 28bbbe8..f86c3a5 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -79,6 +79,19 @@ bool qmp_command_is_enabled(const char *name)
>      return false;
>  }
>  
> +bool qmp_command_has_success_response(const char *name)
> +{
> +    QmpCommand *cmd;
> +
> +    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> +        if (strcmp(cmd->name, name) == 0) {
> +            return cmd->options != QCO_NO_SUCCESS_RESP;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  char **qmp_get_command_list(void)
>  {
>      QmpCommand *cmd;
> diff --git a/qga/commands.c b/qga/commands.c
> index 528b082..0df910b 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
>          cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
>          cmd_info->name = g_strdup(*cmd_list);
>          cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
> +        cmd_info->success_response =
> +            qmp_command_has_success_response(cmd_info->name);
>  
>          cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList));
>          cmd_info_list->value = cmd_info;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 7155b7a..9c6a58f 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -141,10 +141,13 @@
>  #
>  # @enabled: whether command is currently enabled by guest admin
>  #
> +# @success-response: whether command returns a response on success
> +#                    and only available since 1.7
> +#
>  # Since 1.1.0
>  ##
>  { 'type': 'GuestAgentCommandInfo',
> -  'data': { 'name': 'str', 'enabled': 'bool' } }
> +  'data': { 'name': 'str', 'enabled': 'bool', 'success-response': 'bool' } }
>  
>  ##
>  # @GuestAgentInfo

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

On 09/22/2013 12:50 AM, Mark Wu wrote:
> Now we have several qemu-ga commands not returning response on success.
> It has been documented in qga/qapi-schema.json already. This patch exposes
> the 'success-response' flag by extending 'guest-info' command. With this
> change, the clients can handle the command response more flexibly.
> 
> Changes:
> 	v2: add the notation 'since 1.7' to the option 'success-response'
>         (per Eric Blake's comments)

Typically, the patch changelog should be placed...

> 
> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
> ---

...after the --- separator (see
http://wiki.qemu.org/Contribute/SubmitAPatch for more hints).


> +++ b/qga/qapi-schema.json
> @@ -141,10 +141,13 @@
>  #
>  # @enabled: whether command is currently enabled by guest admin
>  #
> +# @success-response: whether command returns a response on success
> +#                    and only available since 1.7

More typically, this is written:

@success-response: whether command returns a response on
                   success (since 1.7)

but as we don't parse the documentation, it doesn't hurt as written.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Michael Roth]

Quoting Mark Wu (2013-09-22 01:50:54)
> Now we have several qemu-ga commands not returning response on success.
> It has been documented in qga/qapi-schema.json already. This patch exposes
> the 'success-response' flag by extending 'guest-info' command. With this
> change, the clients can handle the command response more flexibly.
> 
> Changes:
>         v2: add the notation 'since 1.7' to the option 'success-response'
>         (per Eric Blake's comments)
> 
> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Eric, do we have your reviewed-by other than the changes you mentioned? If so I
can fix those up in my tree.

> ---
>  include/qapi/qmp/dispatch.h |  1 +
>  qapi/qmp-registry.c         | 13 +++++++++++++
>  qga/commands.c              |  2 ++
>  qga/qapi-schema.json        |  5 ++++-
>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1ce11f5..f08c1dd 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.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_has_success_response(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 28bbbe8..f86c3a5 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -79,6 +79,19 @@ bool qmp_command_is_enabled(const char *name)
>      return false;
>  }
> 
> +bool qmp_command_has_success_response(const char *name)
> +{
> +    QmpCommand *cmd;
> +
> +    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> +        if (strcmp(cmd->name, name) == 0) {
> +            return cmd->options != QCO_NO_SUCCESS_RESP;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  char **qmp_get_command_list(void)
>  {
>      QmpCommand *cmd;
> diff --git a/qga/commands.c b/qga/commands.c
> index 528b082..0df910b 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
>          cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
>          cmd_info->name = g_strdup(*cmd_list);
>          cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
> +        cmd_info->success_response =
> +            qmp_command_has_success_response(cmd_info->name);
> 
>          cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList));
>          cmd_info_list->value = cmd_info;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 7155b7a..9c6a58f 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -141,10 +141,13 @@
>  #
>  # @enabled: whether command is currently enabled by guest admin
>  #
> +# @success-response: whether command returns a response on success
> +#                    and only available since 1.7
> +#
>  # Since 1.1.0
>  ##
>  { 'type': 'GuestAgentCommandInfo',
> -  'data': { 'name': 'str', 'enabled': 'bool' } }
> +  'data': { 'name': 'str', 'enabled': 'bool', 'success-response': 'bool' } }
> 
>  ##
>  # @GuestAgentInfo
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

On 09/24/2013 01:00 PM, Michael Roth wrote:
> Quoting Mark Wu (2013-09-22 01:50:54)
>> Now we have several qemu-ga commands not returning response on success.
>> It has been documented in qga/qapi-schema.json already. This patch exposes
>> the 'success-response' flag by extending 'guest-info' command. With this
>> change, the clients can handle the command response more flexibly.
>>
>> Changes:
>>         v2: add the notation 'since 1.7' to the option 'success-response'
>>         (per Eric Blake's comments)
>>
>> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
> 
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Eric, do we have your reviewed-by other than the changes you mentioned? If so I
> can fix those up in my tree.

Aha - force me to do a FULL review, rather than just an interface
review.  I found more issues, so this probably deserves a v2:

>> +bool qmp_command_has_success_response(const char *name)
>> +{
>> +    QmpCommand *cmd;
>> +
>> +    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
>> +        if (strcmp(cmd->name, name) == 0) {
>> +            return cmd->options != QCO_NO_SUCCESS_RESP;

cmd->options is a bitmask - it is feasible that we may add more QCO_NO_*
flags in the future, at which point inequality is NOT correct.  Rather,
you want:

return !(cmd->options & QCO_NO_SUCCESS_RESP);

>> +++ b/qga/commands.c
>> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
>>          cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
>>          cmd_info->name = g_strdup(*cmd_list);
>>          cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
>> +        cmd_info->success_response =
>> +            qmp_command_has_success_response(cmd_info->name);

This feels wasteful.  Why are we doing an O(n) lookup for BOTH
qmp_command_is_enabled AND qmp_command_has_success_response, in an O(n)
loop over command names?  That's O(n^2) in the number of commands.
Better would be getting a list of QmpCommand* instead of a list of
char*, and looking directly in each object, for O(n) computation of the
results.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Michael Roth]

Quoting Eric Blake (2013-09-24 14:13:08)
> On 09/24/2013 01:00 PM, Michael Roth wrote:
> > Quoting Mark Wu (2013-09-22 01:50:54)
> >> Now we have several qemu-ga commands not returning response on success.
> >> It has been documented in qga/qapi-schema.json already. This patch exposes
> >> the 'success-response' flag by extending 'guest-info' command. With this
> >> change, the clients can handle the command response more flexibly.
> >>
> >> Changes:
> >>         v2: add the notation 'since 1.7' to the option 'success-response'
> >>         (per Eric Blake's comments)
> >>
> >> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
> > 
> > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > 
> > Eric, do we have your reviewed-by other than the changes you mentioned? If so I
> > can fix those up in my tree.
> 
> Aha - force me to do a FULL review, rather than just an interface
> review.  I found more issues, so this probably deserves a v2:

Haha, you saw through my plan! :)

> 
> >> +bool qmp_command_has_success_response(const char *name)
> >> +{
> >> +    QmpCommand *cmd;
> >> +
> >> +    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> >> +        if (strcmp(cmd->name, name) == 0) {
> >> +            return cmd->options != QCO_NO_SUCCESS_RESP;
> 
> cmd->options is a bitmask - it is feasible that we may add more QCO_NO_*
> flags in the future, at which point inequality is NOT correct.  Rather,
> you want:
> 
> return !(cmd->options & QCO_NO_SUCCESS_RESP);
> 
> >> +++ b/qga/commands.c
> >> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
> >>          cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
> >>          cmd_info->name = g_strdup(*cmd_list);
> >>          cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
> >> +        cmd_info->success_response =
> >> +            qmp_command_has_success_response(cmd_info->name);
> 
> This feels wasteful.  Why are we doing an O(n) lookup for BOTH
> qmp_command_is_enabled AND qmp_command_has_success_response, in an O(n)
> loop over command names?  That's O(n^2) in the number of commands.
> Better would be getting a list of QmpCommand* instead of a list of
> char*, and looking directly in each object, for O(n) computation of the
> results.

Agreed, modifying qmp_get_command_list to return a list of QmpCommand
would be nicer. Rather than looking directly at the fields though I
think we should just fix up qmp_command_is_enabled() and friends to
take a QmpCommand arg instead of a char*. We already have
qmp_find_command to map char*->QmpCommand to support any cases where
we rely on cmd names.

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Luiz Capitulino]

On Tue, 24 Sep 2013 16:07:29 -0500
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> > >> +bool qmp_command_has_success_response(const char *name)
> > >> +{
> > >> +    QmpCommand *cmd;
> > >> +
> > >> +    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> > >> +        if (strcmp(cmd->name, name) == 0) {
> > >> +            return cmd->options != QCO_NO_SUCCESS_RESP;
> > 
> > cmd->options is a bitmask - it is feasible that we may add more QCO_NO_*
> > flags in the future, at which point inequality is NOT correct.  Rather,
> > you want:
> > 
> > return !(cmd->options & QCO_NO_SUCCESS_RESP);

Good catch! IIRC I added cmd->options myself and didn't catch this...

> > >> +++ b/qga/commands.c
> > >> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
> > >>          cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
> > >>          cmd_info->name = g_strdup(*cmd_list);
> > >>          cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
> > >> +        cmd_info->success_response =
> > >> +            qmp_command_has_success_response(cmd_info->name);
> > 
> > This feels wasteful.  Why are we doing an O(n) lookup for BOTH
> > qmp_command_is_enabled AND qmp_command_has_success_response, in an O(n)
> > loop over command names?  That's O(n^2) in the number of commands.
> > Better would be getting a list of QmpCommand* instead of a list of
> > char*, and looking directly in each object, for O(n) computation of the
> > results.
> 
> Agreed, modifying qmp_get_command_list to return a list of QmpCommand
> would be nicer. Rather than looking directly at the fields though I
> think we should just fix up qmp_command_is_enabled() and friends to
> take a QmpCommand arg instead of a char*. We already have
> qmp_find_command to map char*->QmpCommand to support any cases where
> we rely on cmd names.

I agree and I thought the same thing when I reviewed the patch, but
I didn't mind as Mark is just using what's already there.

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Mark Wu]

Hi guys,

Thanks a lot for all your insightful comments.  I will post a patch to 
resolve the O(n2) problem according to Michael's comments soon and 
rebase the 'success-response' on it.

Mark.

On 09/25/2013 08:34 AM, Luiz Capitulino wrote:
> On Tue, 24 Sep 2013 16:07:29 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>
>>>>> +bool qmp_command_has_success_response(const char *name)
>>>>> +{
>>>>> +    QmpCommand *cmd;
>>>>> +
>>>>> +    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
>>>>> +        if (strcmp(cmd->name, name) == 0) {
>>>>> +            return cmd->options != QCO_NO_SUCCESS_RESP;
>>> cmd->options is a bitmask - it is feasible that we may add more QCO_NO_*
>>> flags in the future, at which point inequality is NOT correct.  Rather,
>>> you want:
>>>
>>> return !(cmd->options & QCO_NO_SUCCESS_RESP);
> Good catch! IIRC I added cmd->options myself and didn't catch this...
>
>>>>> +++ b/qga/commands.c
>>>>> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
>>>>>           cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
>>>>>           cmd_info->name = g_strdup(*cmd_list);
>>>>>           cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
>>>>> +        cmd_info->success_response =
>>>>> +            qmp_command_has_success_response(cmd_info->name);
>>> This feels wasteful.  Why are we doing an O(n) lookup for BOTH
>>> qmp_command_is_enabled AND qmp_command_has_success_response, in an O(n)
>>> loop over command names?  That's O(n^2) in the number of commands.
>>> Better would be getting a list of QmpCommand* instead of a list of
>>> char*, and looking directly in each object, for O(n) computation of the
>>> results.
>> Agreed, modifying qmp_get_command_list to return a list of QmpCommand
>> would be nicer. Rather than looking directly at the fields though I
>> think we should just fix up qmp_command_is_enabled() and friends to
>> take a QmpCommand arg instead of a char*. We already have
>> qmp_find_command to map char*->QmpCommand to support any cases where
>> we rely on cmd names.
> I agree and I thought the same thing when I reviewed the patch, but
> I didn't mind as Mark is just using what's already there.
>

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

On 09/18/2013 01:23 AM, Mark Wu wrote:
> Now we have several qemu-ga commands not returning response on success.
> It has been documented in qga/qapi-schema.json already. This patch exposes
> the 'success-response' flag by extending 'guest-info' command. With this
> change, the clients can handle the command response more flexibly.
> 
> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
> ---
>  include/qapi/qmp/dispatch.h |  1 +
>  qapi/qmp-registry.c         | 13 +++++++++++++
>  qga/commands.c              |  2 ++
>  qga/qapi-schema.json        |  4 +++-
>  4 files changed, 19 insertions(+), 1 deletion(-)
> 

> +++ b/qga/qapi-schema.json
> @@ -141,10 +141,12 @@
>  #
>  # @enabled: whether command is currently enabled by guest admin
>  #
> +# @success-response: whether command returns a response on success

Please add the notation (since 1.7) to this option, to document that it
was not present in 1.1.0.

Other than that, looks okay to me.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

Re: [Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Luiz Capitulino]

On Wed, 18 Sep 2013 15:23:58 +0800
Mark Wu <wudxw@linux.vnet.ibm.com> wrote:

> Now we have several qemu-ga commands not returning response on success.
> It has been documented in qga/qapi-schema.json already. This patch exposes
> the 'success-response' flag by extending 'guest-info' command. With this
> change, the clients can handle the command response more flexibly.
> 
> Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>

Looks good:

Reviewed-by: Luiz Capitulino <lcapitulino@redhat.com>

> ---
>  include/qapi/qmp/dispatch.h |  1 +
>  qapi/qmp-registry.c         | 13 +++++++++++++
>  qga/commands.c              |  2 ++
>  qga/qapi-schema.json        |  4 +++-
>  4 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
> index 1ce11f5..f08c1dd 100644
> --- a/include/qapi/qmp/dispatch.h
> +++ b/include/qapi/qmp/dispatch.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_has_success_response(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 28bbbe8..f86c3a5 100644
> --- a/qapi/qmp-registry.c
> +++ b/qapi/qmp-registry.c
> @@ -79,6 +79,19 @@ bool qmp_command_is_enabled(const char *name)
>      return false;
>  }
>  
> +bool qmp_command_has_success_response(const char *name)
> +{
> +    QmpCommand *cmd;
> +
> +    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
> +        if (strcmp(cmd->name, name) == 0) {
> +            return cmd->options != QCO_NO_SUCCESS_RESP;
> +        }
> +    }
> +
> +    return false;
> +}
> +
>  char **qmp_get_command_list(void)
>  {
>      QmpCommand *cmd;
> diff --git a/qga/commands.c b/qga/commands.c
> index 528b082..0df910b 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
>          cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
>          cmd_info->name = g_strdup(*cmd_list);
>          cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
> +        cmd_info->success_response =
> +            qmp_command_has_success_response(cmd_info->name);
>  
>          cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList));
>          cmd_info_list->value = cmd_info;
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index 7155b7a..6caf019 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -141,10 +141,12 @@
>  #
>  # @enabled: whether command is currently enabled by guest admin
>  #
> +# @success-response: whether command returns a response on success
> +#
>  # Since 1.1.0
>  ##
>  { 'type': 'GuestAgentCommandInfo',
> -  'data': { 'name': 'str', 'enabled': 'bool' } }
> +  'data': { 'name': 'str', 'enabled': 'bool', 'success-response': 'bool' } }
>  
>  ##
>  # @GuestAgentInfo

[Qemu-devel] [PATCH] Extend qemu-ga's 'guest-info' command to expose flag 'success-response'

[Mark Wu]

Now we have several qemu-ga commands not returning response on success.
It has been documented in qga/qapi-schema.json already. This patch exposes
the 'success-response' flag by extending 'guest-info' command. With this
change, the clients can handle the command response more flexibly.

Signed-off-by: Mark Wu <wudxw@linux.vnet.ibm.com>
---
 include/qapi/qmp/dispatch.h |  1 +
 qapi/qmp-registry.c         | 13 +++++++++++++
 qga/commands.c              |  2 ++
 qga/qapi-schema.json        |  4 +++-
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 1ce11f5..f08c1dd 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.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_has_success_response(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 28bbbe8..f86c3a5 100644
--- a/qapi/qmp-registry.c
+++ b/qapi/qmp-registry.c
@@ -79,6 +79,19 @@ bool qmp_command_is_enabled(const char *name)
     return false;
 }
 
+bool qmp_command_has_success_response(const char *name)
+{
+    QmpCommand *cmd;
+
+    QTAILQ_FOREACH(cmd, &qmp_commands, node) {
+        if (strcmp(cmd->name, name) == 0) {
+            return cmd->options != QCO_NO_SUCCESS_RESP;
+        }
+    }
+
+    return false;
+}
+
 char **qmp_get_command_list(void)
 {
     QmpCommand *cmd;
diff --git a/qga/commands.c b/qga/commands.c
index 528b082..0df910b 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -63,6 +63,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
         cmd_info = g_malloc0(sizeof(GuestAgentCommandInfo));
         cmd_info->name = g_strdup(*cmd_list);
         cmd_info->enabled = qmp_command_is_enabled(cmd_info->name);
+        cmd_info->success_response =
+            qmp_command_has_success_response(cmd_info->name);
 
         cmd_info_list = g_malloc0(sizeof(GuestAgentCommandInfoList));
         cmd_info_list->value = cmd_info;
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 7155b7a..6caf019 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -141,10 +141,12 @@
 #
 # @enabled: whether command is currently enabled by guest admin
 #
+# @success-response: whether command returns a response on success
+#
 # Since 1.1.0
 ##
 { 'type': 'GuestAgentCommandInfo',
-  'data': { 'name': 'str', 'enabled': 'bool' } }
+  'data': { 'name': 'str', 'enabled': 'bool', 'success-response': 'bool' } }
 
 ##
 # @GuestAgentInfo
-- 
1.8.3.1