On Wed, 2 Jun 2010 13:40:06 +0100
"Daniel P. Berrange" <berra...@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)
