Re: [Qemu-devel] [PATCH] Change 'query-version' to output broken down version string

[Luiz Capitulino]

On Wed,  2 Jun 2010 13:40:06 +0100
"Daniel P. Berrange" <address@hidden> wrote:

> A previous discussion brought up the fact that clients should
> not have to parse version string from QMP, it should be given
> to them pre-split.

 Right.

> Change query-version output format from:
> 
>   { "qemu": "0.11.50", "package": "" }
> 
> to:
> 
>   { "major": 0, "minor": 11, "micro": 5, "package": "" }
> 
> major, minor & micro are all integer values. package is an
> arbitrary string whose format is defined by the OS package
> maintainer.

 Looks good to me, a few comments:

1. Does QEMU have a naming convention for its versioning scheme or are
   we creating one right now?

2. The "package" member concerns me a bit, as package maintainers can
   put anything there, for example qemu-kvm has "(qemu-kvm-devel)" and
   we could have worst cases like: "-0.341.121-foobar-release".

   Perhaps we could let package alone and have a "downstream" member which
   could be either just a plain string or a dict with keys for name and version.

3. Actually, qemu-kvm has " (qemu-kvm-devel)", the leading whitespace is a bug,
   maybe you could address it in this patch in case we decide to stay
   with "package"?


 Two more comments below.

> There is no need to preserve the existing 'qemu' field,
> since QMP is not yet declared stable.

 I've added the 'qemu' member to allow easy expansion in case we
decided to have a 'qmp' key.

> ---
>  monitor.c |   33 ++++++++++++++++++++++++++-------
>  1 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 3ee365c..a33f7a8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -673,8 +673,11 @@ static void do_info_version_print(Monitor *mon, const 
> QObject *data)
>  
>      qdict = qobject_to_qdict(data);
>  
> -    monitor_printf(mon, "%s%s\n", qdict_get_str(qdict, "qemu"),
> -                                  qdict_get_str(qdict, "package"));
> +    monitor_printf(mon, "%" PRId64 ".%" PRId64 ".%" PRId64 "%s\n",
> +                qdict_get_int(qdict, "major"),
> +                qdict_get_int(qdict, "minor"),
> +                qdict_get_int(qdict, "micro"),
> +                qdict_get_str(qdict, "package"));
>  }
>  
>  /**
> @@ -682,17 +685,33 @@ static void do_info_version_print(Monitor *mon, const 
> QObject *data)
>   *
>   * Return a QDict with the following information:
>   *
> - * - "qemu": QEMU's version
> - * - "package": package's version
> + * - "major": QEMU's major version
> + * - "minor": QEMU's minor version
> + * - "micro": QEMU's micro version
> + * - "package": QEMU packager's version
> + *
> + * The first three values are guarenteed to be
> + * integers. The final 'package' value is a string
> + * in an arbitrary packager specific format
>   *
>   * Example:
>   *
> - * { "qemu": "0.11.50", "package": "" }
> + * { "major": 0, "minor": 11, "micro": 5, "package": "" }
>   */

 This comment doesn't exist anymore, now we should update qemu-monitor.hx
(search for 'query-version' there).

>  static void do_info_version(QObject **ret_data)
>  {
> -    *ret_data = qobject_from_jsonf("{ 'qemu': %s, 'package': %s }",
> -                                   QEMU_VERSION, QEMU_PKGVERSION);
> +    const char *version = QEMU_VERSION;
> +    int major = 0, minor = 0, micro = 0;
> +    char *tmp;
> +
> +    major = strtol(version, &tmp, 10);
> +    tmp++;
> +    minor = strtol(tmp, &tmp, 10);
> +    tmp++;
> +    micro = strtol(tmp, &tmp, 10);
> +
> +    *ret_data = qobject_from_jsonf("{ 'major': %d, 'minor': %d, 'micro': %d, 
> 'package': %s }",
> +                                   major, minor, micro, QEMU_PKGVERSION);
>  }
>  
>  static void do_info_name_print(Monitor *mon, const QObject *data)