| Message ID | 1326326165-23201-1-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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);
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); >
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); > > >
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);
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(-)