Patchwork [v2] qemu-ga: add guest-set-support-level command

login
register
mail settings
Submitter Michael Roth
Date Jan. 11, 2012, 11:56 p.m.
Message ID <1326326165-23201-1-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/135572/
State New
Headers show

Comments

Michael Roth - Jan. 11, 2012, 11:56 p.m.
Recently commands where introduced on the mailing that involved adding
commands to the guest agent that could potentially break older versions
of QEMU. While it's okay to expect that qemu-ga can be updated to support
newer host features, it's unrealistic to require a host to be updated to
support qemu-ga features, or to expect that qemu-ga should be downgraded
in these circumstances, especially considering that a large mix of
guests/agents may be running on a particular host.

To address this, we introduce here a mechanism to set qemu-ga's
feature-set to one that is known to be compatible with
the host/QEMU running the guest. As new commands/options are added, we
can maintain backward-compatibility by adding conditional checks to filter
out host-incompatible functionality based on the host-specified support
level (generally analogous to the host QEMU version) set by the
client.

The current default/minimum support level supports all versions of QEMU
that have had qemu-ga in-tree (0.15.0, 1.0.0) and so should be
backward-compatible with existing hosts/clients.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qapi-schema-guest.json     |   31 ++++++++++++++++++++++++++++-
 qemu-ga.c                  |   46 ++++++++++++++++++++++++++++++++++++++++++++
 qga/guest-agent-commands.c |   13 ++++++++++++
 qga/guest-agent-core.h     |   11 ++++++++++
 4 files changed, 100 insertions(+), 1 deletions(-)
Luiz Capitulino - Jan. 12, 2012, 6:57 p.m.
On Wed, 11 Jan 2012 17:56:05 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> Recently commands where introduced on the mailing that involved adding
> commands to the guest agent that could potentially break older versions
> of QEMU. While it's okay to expect that qemu-ga can be updated to support
> newer host features, it's unrealistic to require a host to be updated to
> support qemu-ga features, or to expect that qemu-ga should be downgraded
> in these circumstances, especially considering that a large mix of
> guests/agents may be running on a particular host.
> 
> To address this, we introduce here a mechanism to set qemu-ga's
> feature-set to one that is known to be compatible with
> the host/QEMU running the guest. As new commands/options are added, we
> can maintain backward-compatibility by adding conditional checks to filter
> out host-incompatible functionality based on the host-specified support
> level (generally analogous to the host QEMU version) set by the
> client.
> 
> The current default/minimum support level supports all versions of QEMU
> that have had qemu-ga in-tree (0.15.0, 1.0.0) and so should be
> backward-compatible with existing hosts/clients.

The approach looks fine to me. I have a few review comments below.

> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qapi-schema-guest.json     |   31 ++++++++++++++++++++++++++++-
>  qemu-ga.c                  |   46 ++++++++++++++++++++++++++++++++++++++++++++
>  qga/guest-agent-commands.c |   13 ++++++++++++
>  qga/guest-agent-core.h     |   11 ++++++++++
>  4 files changed, 100 insertions(+), 1 deletions(-)
> 
> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> index 5f8a18d..32bc041 100644
> --- a/qapi-schema-guest.json
> +++ b/qapi-schema-guest.json
> @@ -43,15 +43,44 @@
>  #
>  # Since: 0.15.0
>  ##
> +{ 'type': 'GuestAgentSupportLevel',
> +  'data': { 'major': 'int', 'minor': 'int', 'micro': 'int' } }

This and GuestAgentCommandInfo are missing good documentation. Looks like
we don't document types as we do in qapi-schema.json. I think we should.

>  { 'type': 'GuestAgentCommandInfo',
>    'data': { 'name': 'str', 'enabled': 'bool' } }
>  { 'type': 'GuestAgentInfo',
>    'data': { 'version': 'str',
> -            'supported_commands': ['GuestAgentCommandInfo'] } }
> +            'supported_commands': ['GuestAgentCommandInfo']
> +            'support_level': 'GuestAgentSupportLevel' } }
>  { 'command': 'guest-info',
>    'returns': 'GuestAgentInfo' }

For example, something important that's is not totally clear to me just by
reading the command definition is if 'support_level' refers to the support
level that can be changed by a client.

>  
>  ##
> +# @guest-set-support-level:
> +#
> +# Set guest agent feature-set to one that is compatible with/supported by
> +# the host.
> +#
> +# Certain commands/options may have dependencies on host
> +# version/support-level, such as specific QEMU features (such
> +# dependencies will be noted in this schema). By default we assume 1.0.0,
> +# which is backward-compatible with QEMU 0.15.0/1.0, and should be compatible
> +# with later versions of QEMU as well. To enable newer guest agent features,
> +# this command must be issued to raise the support-level to one corresponding
> +# to supported host QEMU/KVM/etc capabilities.
> +#
> +# The currently set support level is obtainable via the guest-info command.
> +#
> +# @level: Desired host support-level, generally <= host QEMU version
> +# level. Note that undefined behavior may occur if a support-level is
> +# provided that exceeds the capabilities of the version of QEMU currently
> +# running the guest.

It's also better to note that if @level < 1.0.0 then the support level will
be set to 1.0.0.

> +#
> +# Returns: Nothing on success
> +#
> +{ 'command': 'guest-set-support-level',
> +  'data':    { 'major': 'int', 'minor': 'int', '*micro': 'int' } }
> +
> +##
>  # @guest-shutdown:
>  #
>  # Initiate guest-activated shutdown. Note: this is an asynchronous
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 200bb15..6840233 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -28,6 +28,7 @@
>  #include "qerror.h"
>  #include "error_int.h"
>  #include "qapi/qmp-core.h"
> +#include "qga-qapi-types.h"
>  
>  #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
>  #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid"
> @@ -102,6 +103,45 @@ static void usage(const char *cmd)
>  
>  static void conn_channel_close(GAState *s);
>  
> +static GuestAgentSupportLevel ga_support_level;
> +
> +static int ga_cmp_support_levels(GuestAgentSupportLevel a,
> +                                 GuestAgentSupportLevel b)
> +{
> +    if (a.major == b.major) {
> +        if (a.minor == b.minor) {
> +            return a.micro - b.micro;
> +        }
> +        return a.minor - b.minor;
> +    }
> +    return a.major - b.major;
> +}
> +
> +bool ga_has_support_level(int major, int minor, int micro)
> +{
> +    GuestAgentSupportLevel level = { major, minor, micro };
> +    return ga_cmp_support_levels(level, ga_support_level) >= 0;
> +}
> +
> +void ga_set_support_level(GuestAgentSupportLevel level)
> +{
> +    GuestAgentSupportLevel min_support_level = {
> +        QGA_SUPPORT_LEVEL_MAJOR_MIN,
> +        QGA_SUPPORT_LEVEL_MINOR_MIN,
> +        QGA_SUPPORT_LEVEL_MICRO_MIN
> +    };

Can be const.

> +    if (ga_cmp_support_levels(level, min_support_level) <= 0) {
> +        ga_support_level = min_support_level;
> +    } else {
> +        ga_support_level = level;
> +    }
> +}
> +
> +GuestAgentSupportLevel ga_get_support_level(void)
> +{
> +    return ga_support_level;
> +}
> +
>  static const char *ga_log_level_str(GLogLevelFlags level)
>  {
>      switch (level & G_LOG_LEVEL_MASK) {
> @@ -569,6 +609,11 @@ int main(int argc, char **argv)
>      GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
>      FILE *log_file = stderr;
>      GAState *s;
> +    GuestAgentSupportLevel default_support_level = {
> +        QGA_SUPPORT_LEVEL_MAJOR_DEFAULT,
> +        QGA_SUPPORT_LEVEL_MINOR_DEFAULT,
> +        QGA_SUPPORT_LEVEL_MICRO_DEFAULT
> +    };
>  
>      module_call_init(MODULE_INIT_QAPI);
>  
> @@ -642,6 +687,7 @@ int main(int argc, char **argv)
>          become_daemon(pidfile);
>      }
>  
> +    ga_set_support_level(default_support_level);

You could avoid that call, default_support_level and all the _DEFAULT
macros by initializing ga_support_level statically (using the _MIN macros).

>      s = g_malloc0(sizeof(GAState));
>      s->conn_channel = NULL;
>      s->path = path;
> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> index a09c8ca..656dde8 100644
> --- a/qga/guest-agent-commands.c
> +++ b/qga/guest-agent-commands.c
> @@ -62,6 +62,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
>      char **cmd_list_head, **cmd_list;
>  
>      info->version = g_strdup(QGA_VERSION);
> +    info->support_level = g_malloc0(sizeof(GuestAgentSupportLevel));
> +    *info->support_level = ga_get_support_level();
>  
>      cmd_list_head = cmd_list = qmp_get_command_list();
>      if (*cmd_list_head == NULL) {
> @@ -87,6 +89,17 @@ out:
>      return info;
>  }
>  
> +void qmp_guest_set_support_level(int64_t major, int64_t minor, bool has_micro,
> +                                 int64_t micro, Error **errp)
> +{
> +    GuestAgentSupportLevel level = {
> +        major,
> +        minor,
> +        has_micro ? micro : 0
> +    };
> +    ga_set_support_level(level);
> +}
> +
>  void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>  {
>      int ret;
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index e42b91d..7327e73 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -12,8 +12,16 @@
>   */
>  #include "qapi/qmp-core.h"
>  #include "qemu-common.h"
> +#include "qga-qapi-types.h"
>  
>  #define QGA_VERSION "1.0"
> +#define QGA_SUPPORT_LEVEL_MAJOR_DEFAULT 1
> +#define QGA_SUPPORT_LEVEL_MINOR_DEFAULT 0
> +#define QGA_SUPPORT_LEVEL_MICRO_DEFAULT 0
> +/* lowest possible support level */
> +#define QGA_SUPPORT_LEVEL_MAJOR_MIN 1
> +#define QGA_SUPPORT_LEVEL_MINOR_MIN 0
> +#define QGA_SUPPORT_LEVEL_MICRO_MIN 0
>  #define QGA_READ_COUNT_DEFAULT 4 << 10
>  
>  typedef struct GAState GAState;
> @@ -29,3 +37,6 @@ GACommandState *ga_command_state_new(void);
>  bool ga_logging_enabled(GAState *s);
>  void ga_disable_logging(GAState *s);
>  void ga_enable_logging(GAState *s);
> +bool ga_has_support_level(int major, int minor, int micro);
> +void ga_set_support_level(GuestAgentSupportLevel level);
> +GuestAgentSupportLevel ga_get_support_level(void);
Michael Roth - Jan. 12, 2012, 11:32 p.m.
On 01/12/2012 12:57 PM, Luiz Capitulino wrote:
> On Wed, 11 Jan 2012 17:56:05 -0600
> Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>
>> Recently commands where introduced on the mailing that involved adding
>> commands to the guest agent that could potentially break older versions
>> of QEMU. While it's okay to expect that qemu-ga can be updated to support
>> newer host features, it's unrealistic to require a host to be updated to
>> support qemu-ga features, or to expect that qemu-ga should be downgraded
>> in these circumstances, especially considering that a large mix of
>> guests/agents may be running on a particular host.
>>
>> To address this, we introduce here a mechanism to set qemu-ga's
>> feature-set to one that is known to be compatible with
>> the host/QEMU running the guest. As new commands/options are added, we
>> can maintain backward-compatibility by adding conditional checks to filter
>> out host-incompatible functionality based on the host-specified support
>> level (generally analogous to the host QEMU version) set by the
>> client.
>>
>> The current default/minimum support level supports all versions of QEMU
>> that have had qemu-ga in-tree (0.15.0, 1.0.0) and so should be
>> backward-compatible with existing hosts/clients.
>
> The approach looks fine to me. I have a few review comments below.
>
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   qapi-schema-guest.json     |   31 ++++++++++++++++++++++++++++-
>>   qemu-ga.c                  |   46 ++++++++++++++++++++++++++++++++++++++++++++
>>   qga/guest-agent-commands.c |   13 ++++++++++++
>>   qga/guest-agent-core.h     |   11 ++++++++++
>>   4 files changed, 100 insertions(+), 1 deletions(-)
>>
>> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
>> index 5f8a18d..32bc041 100644
>> --- a/qapi-schema-guest.json
>> +++ b/qapi-schema-guest.json
>> @@ -43,15 +43,44 @@
>>   #
>>   # Since: 0.15.0
>>   ##
>> +{ 'type': 'GuestAgentSupportLevel',
>> +  'data': { 'major': 'int', 'minor': 'int', 'micro': 'int' } }
>
> This and GuestAgentCommandInfo are missing good documentation. Looks like
> we don't document types as we do in qapi-schema.json. I think we should.
>

Agreed, I'll precede this with a patch to add the documentation for 
existing types, and update this patch accordingly.

>>   { 'type': 'GuestAgentCommandInfo',
>>     'data': { 'name': 'str', 'enabled': 'bool' } }
>>   { 'type': 'GuestAgentInfo',
>>     'data': { 'version': 'str',
>> -            'supported_commands': ['GuestAgentCommandInfo'] } }
>> +            'supported_commands': ['GuestAgentCommandInfo']
>> +            'support_level': 'GuestAgentSupportLevel' } }
>>   { 'command': 'guest-info',
>>     'returns': 'GuestAgentInfo' }
>
> For example, something important that's is not totally clear to me just by
> reading the command definition is if 'support_level' refers to the support
> level that can be changed by a client.
>
>>
>>   ##
>> +# @guest-set-support-level:
>> +#
>> +# Set guest agent feature-set to one that is compatible with/supported by
>> +# the host.
>> +#
>> +# Certain commands/options may have dependencies on host
>> +# version/support-level, such as specific QEMU features (such
>> +# dependencies will be noted in this schema). By default we assume 1.0.0,
>> +# which is backward-compatible with QEMU 0.15.0/1.0, and should be compatible
>> +# with later versions of QEMU as well. To enable newer guest agent features,
>> +# this command must be issued to raise the support-level to one corresponding
>> +# to supported host QEMU/KVM/etc capabilities.
>> +#
>> +# The currently set support level is obtainable via the guest-info command.
>> +#
>> +# @level: Desired host support-level, generally<= host QEMU version
>> +# level. Note that undefined behavior may occur if a support-level is
>> +# provided that exceeds the capabilities of the version of QEMU currently
>> +# running the guest.
>
> It's also better to note that if @level<  1.0.0 then the support level will
> be set to 1.0.0.
>

I must've looked at this like 5 times and made a mental note to do that, 
but in the end it escaped me...

>> +#
>> +# Returns: Nothing on success
>> +#
>> +{ 'command': 'guest-set-support-level',
>> +  'data':    { 'major': 'int', 'minor': 'int', '*micro': 'int' } }
>> +
>> +##
>>   # @guest-shutdown:
>>   #
>>   # Initiate guest-activated shutdown. Note: this is an asynchronous
>> diff --git a/qemu-ga.c b/qemu-ga.c
>> index 200bb15..6840233 100644
>> --- a/qemu-ga.c
>> +++ b/qemu-ga.c
>> @@ -28,6 +28,7 @@
>>   #include "qerror.h"
>>   #include "error_int.h"
>>   #include "qapi/qmp-core.h"
>> +#include "qga-qapi-types.h"
>>
>>   #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
>>   #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid"
>> @@ -102,6 +103,45 @@ static void usage(const char *cmd)
>>
>>   static void conn_channel_close(GAState *s);
>>
>> +static GuestAgentSupportLevel ga_support_level;
>> +
>> +static int ga_cmp_support_levels(GuestAgentSupportLevel a,
>> +                                 GuestAgentSupportLevel b)
>> +{
>> +    if (a.major == b.major) {
>> +        if (a.minor == b.minor) {
>> +            return a.micro - b.micro;
>> +        }
>> +        return a.minor - b.minor;
>> +    }
>> +    return a.major - b.major;
>> +}
>> +
>> +bool ga_has_support_level(int major, int minor, int micro)
>> +{
>> +    GuestAgentSupportLevel level = { major, minor, micro };
>> +    return ga_cmp_support_levels(level, ga_support_level)>= 0;
>> +}
>> +
>> +void ga_set_support_level(GuestAgentSupportLevel level)
>> +{
>> +    GuestAgentSupportLevel min_support_level = {
>> +        QGA_SUPPORT_LEVEL_MAJOR_MIN,
>> +        QGA_SUPPORT_LEVEL_MINOR_MIN,
>> +        QGA_SUPPORT_LEVEL_MICRO_MIN
>> +    };
>
> Can be const.
>
>> +    if (ga_cmp_support_levels(level, min_support_level)<= 0) {
>> +        ga_support_level = min_support_level;
>> +    } else {
>> +        ga_support_level = level;
>> +    }
>> +}
>> +
>> +GuestAgentSupportLevel ga_get_support_level(void)
>> +{
>> +    return ga_support_level;
>> +}
>> +
>>   static const char *ga_log_level_str(GLogLevelFlags level)
>>   {
>>       switch (level&  G_LOG_LEVEL_MASK) {
>> @@ -569,6 +609,11 @@ int main(int argc, char **argv)
>>       GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
>>       FILE *log_file = stderr;
>>       GAState *s;
>> +    GuestAgentSupportLevel default_support_level = {
>> +        QGA_SUPPORT_LEVEL_MAJOR_DEFAULT,
>> +        QGA_SUPPORT_LEVEL_MINOR_DEFAULT,
>> +        QGA_SUPPORT_LEVEL_MICRO_DEFAULT
>> +    };
>>
>>       module_call_init(MODULE_INIT_QAPI);
>>
>> @@ -642,6 +687,7 @@ int main(int argc, char **argv)
>>           become_daemon(pidfile);
>>       }
>>
>> +    ga_set_support_level(default_support_level);
>
> You could avoid that call, default_support_level and all the _DEFAULT
> macros by initializing ga_support_level statically (using the _MIN macros).
>

The _MIN vs. _DEFAULT is there because we may some time in the future 
decide to bump up the default support level. We'd still allow fallback 
to _MIN, if needed, but it may begin to impede progress in the future. 
But yah, technically since *_MIN == *_DEFAULT I could get rid of them 
till we need them, I just wanted to make it clear in the code from the 
start. Same with making the call...ga_set_support_level() may at some 
point take list of capabilities or something...just wanted to make a 
central point where all the initialization happens. But I don't mind 
losing it if that's too much ugly-now-but-might-make-sense-later stuff.

>>       s = g_malloc0(sizeof(GAState));
>>       s->conn_channel = NULL;
>>       s->path = path;
>> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
>> index a09c8ca..656dde8 100644
>> --- a/qga/guest-agent-commands.c
>> +++ b/qga/guest-agent-commands.c
>> @@ -62,6 +62,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
>>       char **cmd_list_head, **cmd_list;
>>
>>       info->version = g_strdup(QGA_VERSION);
>> +    info->support_level = g_malloc0(sizeof(GuestAgentSupportLevel));
>> +    *info->support_level = ga_get_support_level();
>>
>>       cmd_list_head = cmd_list = qmp_get_command_list();
>>       if (*cmd_list_head == NULL) {
>> @@ -87,6 +89,17 @@ out:
>>       return info;
>>   }
>>
>> +void qmp_guest_set_support_level(int64_t major, int64_t minor, bool has_micro,
>> +                                 int64_t micro, Error **errp)
>> +{
>> +    GuestAgentSupportLevel level = {
>> +        major,
>> +        minor,
>> +        has_micro ? micro : 0
>> +    };
>> +    ga_set_support_level(level);
>> +}
>> +
>>   void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
>>   {
>>       int ret;
>> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
>> index e42b91d..7327e73 100644
>> --- a/qga/guest-agent-core.h
>> +++ b/qga/guest-agent-core.h
>> @@ -12,8 +12,16 @@
>>    */
>>   #include "qapi/qmp-core.h"
>>   #include "qemu-common.h"
>> +#include "qga-qapi-types.h"
>>
>>   #define QGA_VERSION "1.0"
>> +#define QGA_SUPPORT_LEVEL_MAJOR_DEFAULT 1
>> +#define QGA_SUPPORT_LEVEL_MINOR_DEFAULT 0
>> +#define QGA_SUPPORT_LEVEL_MICRO_DEFAULT 0
>> +/* lowest possible support level */
>> +#define QGA_SUPPORT_LEVEL_MAJOR_MIN 1
>> +#define QGA_SUPPORT_LEVEL_MINOR_MIN 0
>> +#define QGA_SUPPORT_LEVEL_MICRO_MIN 0
>>   #define QGA_READ_COUNT_DEFAULT 4<<  10
>>
>>   typedef struct GAState GAState;
>> @@ -29,3 +37,6 @@ GACommandState *ga_command_state_new(void);
>>   bool ga_logging_enabled(GAState *s);
>>   void ga_disable_logging(GAState *s);
>>   void ga_enable_logging(GAState *s);
>> +bool ga_has_support_level(int major, int minor, int micro);
>> +void ga_set_support_level(GuestAgentSupportLevel level);
>> +GuestAgentSupportLevel ga_get_support_level(void);
>
Luiz Capitulino - Jan. 13, 2012, 12:14 p.m.
On Thu, 12 Jan 2012 17:32:32 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On 01/12/2012 12:57 PM, Luiz Capitulino wrote:
> > On Wed, 11 Jan 2012 17:56:05 -0600
> > Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> >
> >> Recently commands where introduced on the mailing that involved adding
> >> commands to the guest agent that could potentially break older versions
> >> of QEMU. While it's okay to expect that qemu-ga can be updated to support
> >> newer host features, it's unrealistic to require a host to be updated to
> >> support qemu-ga features, or to expect that qemu-ga should be downgraded
> >> in these circumstances, especially considering that a large mix of
> >> guests/agents may be running on a particular host.
> >>
> >> To address this, we introduce here a mechanism to set qemu-ga's
> >> feature-set to one that is known to be compatible with
> >> the host/QEMU running the guest. As new commands/options are added, we
> >> can maintain backward-compatibility by adding conditional checks to filter
> >> out host-incompatible functionality based on the host-specified support
> >> level (generally analogous to the host QEMU version) set by the
> >> client.
> >>
> >> The current default/minimum support level supports all versions of QEMU
> >> that have had qemu-ga in-tree (0.15.0, 1.0.0) and so should be
> >> backward-compatible with existing hosts/clients.
> >
> > The approach looks fine to me. I have a few review comments below.
> >
> >>
> >> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> >> ---
> >>   qapi-schema-guest.json     |   31 ++++++++++++++++++++++++++++-
> >>   qemu-ga.c                  |   46 ++++++++++++++++++++++++++++++++++++++++++++
> >>   qga/guest-agent-commands.c |   13 ++++++++++++
> >>   qga/guest-agent-core.h     |   11 ++++++++++
> >>   4 files changed, 100 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
> >> index 5f8a18d..32bc041 100644
> >> --- a/qapi-schema-guest.json
> >> +++ b/qapi-schema-guest.json
> >> @@ -43,15 +43,44 @@
> >>   #
> >>   # Since: 0.15.0
> >>   ##
> >> +{ 'type': 'GuestAgentSupportLevel',
> >> +  'data': { 'major': 'int', 'minor': 'int', 'micro': 'int' } }
> >
> > This and GuestAgentCommandInfo are missing good documentation. Looks like
> > we don't document types as we do in qapi-schema.json. I think we should.
> >
> 
> Agreed, I'll precede this with a patch to add the documentation for 
> existing types, and update this patch accordingly.
> 
> >>   { 'type': 'GuestAgentCommandInfo',
> >>     'data': { 'name': 'str', 'enabled': 'bool' } }
> >>   { 'type': 'GuestAgentInfo',
> >>     'data': { 'version': 'str',
> >> -            'supported_commands': ['GuestAgentCommandInfo'] } }
> >> +            'supported_commands': ['GuestAgentCommandInfo']
> >> +            'support_level': 'GuestAgentSupportLevel' } }
> >>   { 'command': 'guest-info',
> >>     'returns': 'GuestAgentInfo' }
> >
> > For example, something important that's is not totally clear to me just by
> > reading the command definition is if 'support_level' refers to the support
> > level that can be changed by a client.
> >
> >>
> >>   ##
> >> +# @guest-set-support-level:
> >> +#
> >> +# Set guest agent feature-set to one that is compatible with/supported by
> >> +# the host.
> >> +#
> >> +# Certain commands/options may have dependencies on host
> >> +# version/support-level, such as specific QEMU features (such
> >> +# dependencies will be noted in this schema). By default we assume 1.0.0,
> >> +# which is backward-compatible with QEMU 0.15.0/1.0, and should be compatible
> >> +# with later versions of QEMU as well. To enable newer guest agent features,
> >> +# this command must be issued to raise the support-level to one corresponding
> >> +# to supported host QEMU/KVM/etc capabilities.
> >> +#
> >> +# The currently set support level is obtainable via the guest-info command.
> >> +#
> >> +# @level: Desired host support-level, generally<= host QEMU version
> >> +# level. Note that undefined behavior may occur if a support-level is
> >> +# provided that exceeds the capabilities of the version of QEMU currently
> >> +# running the guest.
> >
> > It's also better to note that if @level<  1.0.0 then the support level will
> > be set to 1.0.0.
> >
> 
> I must've looked at this like 5 times and made a mental note to do that, 
> but in the end it escaped me...

Hehe, happens. There's one more issue below.

> 
> >> +#
> >> +# Returns: Nothing on success
> >> +#
> >> +{ 'command': 'guest-set-support-level',
> >> +  'data':    { 'major': 'int', 'minor': 'int', '*micro': 'int' } }
> >> +
> >> +##
> >>   # @guest-shutdown:
> >>   #
> >>   # Initiate guest-activated shutdown. Note: this is an asynchronous
> >> diff --git a/qemu-ga.c b/qemu-ga.c
> >> index 200bb15..6840233 100644
> >> --- a/qemu-ga.c
> >> +++ b/qemu-ga.c
> >> @@ -28,6 +28,7 @@
> >>   #include "qerror.h"
> >>   #include "error_int.h"
> >>   #include "qapi/qmp-core.h"
> >> +#include "qga-qapi-types.h"
> >>
> >>   #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
> >>   #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid"
> >> @@ -102,6 +103,45 @@ static void usage(const char *cmd)
> >>
> >>   static void conn_channel_close(GAState *s);
> >>
> >> +static GuestAgentSupportLevel ga_support_level;
> >> +
> >> +static int ga_cmp_support_levels(GuestAgentSupportLevel a,
> >> +                                 GuestAgentSupportLevel b)
> >> +{
> >> +    if (a.major == b.major) {
> >> +        if (a.minor == b.minor) {
> >> +            return a.micro - b.micro;
> >> +        }
> >> +        return a.minor - b.minor;
> >> +    }
> >> +    return a.major - b.major;
> >> +}
> >> +
> >> +bool ga_has_support_level(int major, int minor, int micro)
> >> +{
> >> +    GuestAgentSupportLevel level = { major, minor, micro };
> >> +    return ga_cmp_support_levels(level, ga_support_level)>= 0;
> >> +}

This should be <= 0. This was a bit tricky to find because my test
matrix isn't small.

> >> +
> >> +void ga_set_support_level(GuestAgentSupportLevel level)
> >> +{
> >> +    GuestAgentSupportLevel min_support_level = {
> >> +        QGA_SUPPORT_LEVEL_MAJOR_MIN,
> >> +        QGA_SUPPORT_LEVEL_MINOR_MIN,
> >> +        QGA_SUPPORT_LEVEL_MICRO_MIN
> >> +    };
> >
> > Can be const.
> >
> >> +    if (ga_cmp_support_levels(level, min_support_level)<= 0) {
> >> +        ga_support_level = min_support_level;
> >> +    } else {
> >> +        ga_support_level = level;
> >> +    }
> >> +}
> >> +
> >> +GuestAgentSupportLevel ga_get_support_level(void)
> >> +{
> >> +    return ga_support_level;
> >> +}
> >> +
> >>   static const char *ga_log_level_str(GLogLevelFlags level)
> >>   {
> >>       switch (level&  G_LOG_LEVEL_MASK) {
> >> @@ -569,6 +609,11 @@ int main(int argc, char **argv)
> >>       GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
> >>       FILE *log_file = stderr;
> >>       GAState *s;
> >> +    GuestAgentSupportLevel default_support_level = {
> >> +        QGA_SUPPORT_LEVEL_MAJOR_DEFAULT,
> >> +        QGA_SUPPORT_LEVEL_MINOR_DEFAULT,
> >> +        QGA_SUPPORT_LEVEL_MICRO_DEFAULT
> >> +    };
> >>
> >>       module_call_init(MODULE_INIT_QAPI);
> >>
> >> @@ -642,6 +687,7 @@ int main(int argc, char **argv)
> >>           become_daemon(pidfile);
> >>       }
> >>
> >> +    ga_set_support_level(default_support_level);
> >
> > You could avoid that call, default_support_level and all the _DEFAULT
> > macros by initializing ga_support_level statically (using the _MIN macros).
> >
> 
> The _MIN vs. _DEFAULT is there because we may some time in the future 
> decide to bump up the default support level. We'd still allow fallback 
> to _MIN, if needed, but it may begin to impede progress in the future. 
> But yah, technically since *_MIN == *_DEFAULT I could get rid of them 
> till we need them, I just wanted to make it clear in the code from the 
> start. Same with making the call...ga_set_support_level() may at some 
> point take list of capabilities or something...just wanted to make a 
> central point where all the initialization happens. But I don't mind 
> losing it if that's too much ugly-now-but-might-make-sense-later stuff.
> 
> >>       s = g_malloc0(sizeof(GAState));
> >>       s->conn_channel = NULL;
> >>       s->path = path;
> >> diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
> >> index a09c8ca..656dde8 100644
> >> --- a/qga/guest-agent-commands.c
> >> +++ b/qga/guest-agent-commands.c
> >> @@ -62,6 +62,8 @@ struct GuestAgentInfo *qmp_guest_info(Error **err)
> >>       char **cmd_list_head, **cmd_list;
> >>
> >>       info->version = g_strdup(QGA_VERSION);
> >> +    info->support_level = g_malloc0(sizeof(GuestAgentSupportLevel));
> >> +    *info->support_level = ga_get_support_level();
> >>
> >>       cmd_list_head = cmd_list = qmp_get_command_list();
> >>       if (*cmd_list_head == NULL) {
> >> @@ -87,6 +89,17 @@ out:
> >>       return info;
> >>   }
> >>
> >> +void qmp_guest_set_support_level(int64_t major, int64_t minor, bool has_micro,
> >> +                                 int64_t micro, Error **errp)
> >> +{
> >> +    GuestAgentSupportLevel level = {
> >> +        major,
> >> +        minor,
> >> +        has_micro ? micro : 0
> >> +    };
> >> +    ga_set_support_level(level);
> >> +}
> >> +
> >>   void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
> >>   {
> >>       int ret;
> >> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> >> index e42b91d..7327e73 100644
> >> --- a/qga/guest-agent-core.h
> >> +++ b/qga/guest-agent-core.h
> >> @@ -12,8 +12,16 @@
> >>    */
> >>   #include "qapi/qmp-core.h"
> >>   #include "qemu-common.h"
> >> +#include "qga-qapi-types.h"
> >>
> >>   #define QGA_VERSION "1.0"
> >> +#define QGA_SUPPORT_LEVEL_MAJOR_DEFAULT 1
> >> +#define QGA_SUPPORT_LEVEL_MINOR_DEFAULT 0
> >> +#define QGA_SUPPORT_LEVEL_MICRO_DEFAULT 0
> >> +/* lowest possible support level */
> >> +#define QGA_SUPPORT_LEVEL_MAJOR_MIN 1
> >> +#define QGA_SUPPORT_LEVEL_MINOR_MIN 0
> >> +#define QGA_SUPPORT_LEVEL_MICRO_MIN 0
> >>   #define QGA_READ_COUNT_DEFAULT 4<<  10
> >>
> >>   typedef struct GAState GAState;
> >> @@ -29,3 +37,6 @@ GACommandState *ga_command_state_new(void);
> >>   bool ga_logging_enabled(GAState *s);
> >>   void ga_disable_logging(GAState *s);
> >>   void ga_enable_logging(GAState *s);
> >> +bool ga_has_support_level(int major, int minor, int micro);
> >> +void ga_set_support_level(GuestAgentSupportLevel level);
> >> +GuestAgentSupportLevel ga_get_support_level(void);
> >
>

Patch

diff --git a/qapi-schema-guest.json b/qapi-schema-guest.json
index 5f8a18d..32bc041 100644
--- a/qapi-schema-guest.json
+++ b/qapi-schema-guest.json
@@ -43,15 +43,44 @@ 
 #
 # Since: 0.15.0
 ##
+{ 'type': 'GuestAgentSupportLevel',
+  'data': { 'major': 'int', 'minor': 'int', 'micro': 'int' } }
 { 'type': 'GuestAgentCommandInfo',
   'data': { 'name': 'str', 'enabled': 'bool' } }
 { 'type': 'GuestAgentInfo',
   'data': { 'version': 'str',
-            'supported_commands': ['GuestAgentCommandInfo'] } }
+            'supported_commands': ['GuestAgentCommandInfo']
+            'support_level': 'GuestAgentSupportLevel' } }
 { 'command': 'guest-info',
   'returns': 'GuestAgentInfo' }
 
 ##
+# @guest-set-support-level:
+#
+# Set guest agent feature-set to one that is compatible with/supported by
+# the host.
+#
+# Certain commands/options may have dependencies on host
+# version/support-level, such as specific QEMU features (such
+# dependencies will be noted in this schema). By default we assume 1.0.0,
+# which is backward-compatible with QEMU 0.15.0/1.0, and should be compatible
+# with later versions of QEMU as well. To enable newer guest agent features,
+# this command must be issued to raise the support-level to one corresponding
+# to supported host QEMU/KVM/etc capabilities.
+#
+# The currently set support level is obtainable via the guest-info command.
+#
+# @level: Desired host support-level, generally <= host QEMU version
+# level. Note that undefined behavior may occur if a support-level is
+# provided that exceeds the capabilities of the version of QEMU currently
+# running the guest.
+#
+# Returns: Nothing on success
+#
+{ 'command': 'guest-set-support-level',
+  'data':    { 'major': 'int', 'minor': 'int', '*micro': 'int' } }
+
+##
 # @guest-shutdown:
 #
 # Initiate guest-activated shutdown. Note: this is an asynchronous
diff --git a/qemu-ga.c b/qemu-ga.c
index 200bb15..6840233 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -28,6 +28,7 @@ 
 #include "qerror.h"
 #include "error_int.h"
 #include "qapi/qmp-core.h"
+#include "qga-qapi-types.h"
 
 #define QGA_VIRTIO_PATH_DEFAULT "/dev/virtio-ports/org.qemu.guest_agent.0"
 #define QGA_PIDFILE_DEFAULT "/var/run/qemu-ga.pid"
@@ -102,6 +103,45 @@  static void usage(const char *cmd)
 
 static void conn_channel_close(GAState *s);
 
+static GuestAgentSupportLevel ga_support_level;
+
+static int ga_cmp_support_levels(GuestAgentSupportLevel a,
+                                 GuestAgentSupportLevel b)
+{
+    if (a.major == b.major) {
+        if (a.minor == b.minor) {
+            return a.micro - b.micro;
+        }
+        return a.minor - b.minor;
+    }
+    return a.major - b.major;
+}
+
+bool ga_has_support_level(int major, int minor, int micro)
+{
+    GuestAgentSupportLevel level = { major, minor, micro };
+    return ga_cmp_support_levels(level, ga_support_level) >= 0;
+}
+
+void ga_set_support_level(GuestAgentSupportLevel level)
+{
+    GuestAgentSupportLevel min_support_level = {
+        QGA_SUPPORT_LEVEL_MAJOR_MIN,
+        QGA_SUPPORT_LEVEL_MINOR_MIN,
+        QGA_SUPPORT_LEVEL_MICRO_MIN
+    };
+    if (ga_cmp_support_levels(level, min_support_level) <= 0) {
+        ga_support_level = min_support_level;
+    } else {
+        ga_support_level = level;
+    }
+}
+
+GuestAgentSupportLevel ga_get_support_level(void)
+{
+    return ga_support_level;
+}
+
 static const char *ga_log_level_str(GLogLevelFlags level)
 {
     switch (level & G_LOG_LEVEL_MASK) {
@@ -569,6 +609,11 @@  int main(int argc, char **argv)
     GLogLevelFlags log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
     FILE *log_file = stderr;
     GAState *s;
+    GuestAgentSupportLevel default_support_level = {
+        QGA_SUPPORT_LEVEL_MAJOR_DEFAULT,
+        QGA_SUPPORT_LEVEL_MINOR_DEFAULT,
+        QGA_SUPPORT_LEVEL_MICRO_DEFAULT
+    };
 
     module_call_init(MODULE_INIT_QAPI);
 
@@ -642,6 +687,7 @@  int main(int argc, char **argv)
         become_daemon(pidfile);
     }
 
+    ga_set_support_level(default_support_level);
     s = g_malloc0(sizeof(GAState));
     s->conn_channel = NULL;
     s->path = path;
diff --git a/qga/guest-agent-commands.c b/qga/guest-agent-commands.c
index a09c8ca..656dde8 100644
--- a/qga/guest-agent-commands.c
+++ b/qga/guest-agent-commands.c
@@ -62,6 +62,8 @@  struct GuestAgentInfo *qmp_guest_info(Error **err)
     char **cmd_list_head, **cmd_list;
 
     info->version = g_strdup(QGA_VERSION);
+    info->support_level = g_malloc0(sizeof(GuestAgentSupportLevel));
+    *info->support_level = ga_get_support_level();
 
     cmd_list_head = cmd_list = qmp_get_command_list();
     if (*cmd_list_head == NULL) {
@@ -87,6 +89,17 @@  out:
     return info;
 }
 
+void qmp_guest_set_support_level(int64_t major, int64_t minor, bool has_micro,
+                                 int64_t micro, Error **errp)
+{
+    GuestAgentSupportLevel level = {
+        major,
+        minor,
+        has_micro ? micro : 0
+    };
+    ga_set_support_level(level);
+}
+
 void qmp_guest_shutdown(bool has_mode, const char *mode, Error **err)
 {
     int ret;
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index e42b91d..7327e73 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -12,8 +12,16 @@ 
  */
 #include "qapi/qmp-core.h"
 #include "qemu-common.h"
+#include "qga-qapi-types.h"
 
 #define QGA_VERSION "1.0"
+#define QGA_SUPPORT_LEVEL_MAJOR_DEFAULT 1
+#define QGA_SUPPORT_LEVEL_MINOR_DEFAULT 0
+#define QGA_SUPPORT_LEVEL_MICRO_DEFAULT 0
+/* lowest possible support level */
+#define QGA_SUPPORT_LEVEL_MAJOR_MIN 1
+#define QGA_SUPPORT_LEVEL_MINOR_MIN 0
+#define QGA_SUPPORT_LEVEL_MICRO_MIN 0
 #define QGA_READ_COUNT_DEFAULT 4 << 10
 
 typedef struct GAState GAState;
@@ -29,3 +37,6 @@  GACommandState *ga_command_state_new(void);
 bool ga_logging_enabled(GAState *s);
 void ga_disable_logging(GAState *s);
 void ga_enable_logging(GAState *s);
+bool ga_has_support_level(int major, int minor, int micro);
+void ga_set_support_level(GuestAgentSupportLevel level);
+GuestAgentSupportLevel ga_get_support_level(void);