On Thu, 12 Jan 2012 17:32:32 -0600
Michael Roth <mdr...@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<mdr...@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<mdr...@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);
> >
>
