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

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

[Daniel P. Berrange]

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.

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.

There is no need to preserve the existing 'qemu' field,
since QMP is not yet declared stable.
---
 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": "" }
  */
 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)
-- 
1.6.6.1

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" <berrange@redhat.com> 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)

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

[Daniel P. Berrange]

On Fri, Jun 04, 2010 at 03:34:48PM -0300, Luiz Capitulino wrote:
> On Wed,  2 Jun 2010 13:40:06 +0100
> "Daniel P. Berrange" <berrange@redhat.com> 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?

There doesn't appear to be one, but with 3 entry tuples, calling
them major, minor, micro is fairly standard terminology. Other
suggestions welcome if people don't like that
 
> 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".

This is entirely the point of the 'package' member really. It is there
for OS distro package maintainers to put an arbitrary vendor specific
version data in. In RPM world this would be the 'Release' field.

>    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"?

KVM version I think would not be in the 'package' field, but in some
other key.

> > 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.

Perhaps it should be nested then,

 { 
   "qemu": { "major": 0, "minor": 11, "micro": 5, } 
   "package": ""
 }

To allow for multiple version numbers to be included in their fully
split up format.

> > @@ -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).

Ok, I'll fix this when next rebasing


Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|