[libvirt] [PATCH] qemu driver: fix version check typos
Looks like some cut'n paste error to me. Signed-off-by: Chris Wright <chrisw@redhat.com> --- src/qemu/qemu_conf.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 3e334dc..6d2ac6e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1291,13 +1291,13 @@ int qemudParseHelpStr(const char *qemu, ++p; minor = virParseNumber(&p); - if (major == -1 || *p != '.') + if (minor == -1 || *p != '.') goto fail; ++p; micro = virParseNumber(&p); - if (major == -1) + if (micro == -1) goto fail; SKIP_BLANKS(p);
On 05/19/2010 04:37 PM, Chris Wright wrote:
Looks like some cut'n paste error to me.
Signed-off-by: Chris Wright <chrisw@redhat.com> --- src/qemu/qemu_conf.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 3e334dc..6d2ac6e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1291,13 +1291,13 @@ int qemudParseHelpStr(const char *qemu, ++p;
minor = virParseNumber(&p); - if (major == -1 || *p != '.') + if (minor == -1 || *p != '.') goto fail;
++p;
micro = virParseNumber(&p); - if (major == -1) + if (micro == -1)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/19/2010 04:37 PM, Chris Wright wrote:
Looks like some cut'n paste error to me.
While we're at it, there have been some complaints, at least on IRC, that some recent qemu builds changed -help output to start with "QEMU emulator version" instead of "QEMU PC emulator version", which fails to match QEMU_VERSION_STR. Is that something we should be worrying about, while we're touching this function? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* Eric Blake (eblake@redhat.com) wrote:
On 05/19/2010 04:37 PM, Chris Wright wrote:
Looks like some cut'n paste error to me.
While we're at it, there have been some complaints, at least on IRC, that some recent qemu builds changed -help output to start with "QEMU emulator version" instead of "QEMU PC emulator version", which fails to match QEMU_VERSION_STR. Is that something we should be worrying about, while we're touching this function?
That's what had me looking at it ;) [chrisw@x200 qemu-kvm]$ ./x86_64-softmmu/qemu-system-x86_64 -help | head -1 QEMU emulator version 0.12.50 (qemu-kvm-devel), Copyright (c) 2003-2008 Fabrice Bellard [chrisw@x200 qemu-kvm]$ qemu-kvm -help | head -1 QEMU PC emulator version 0.11.0 (qemu-kvm-0.11.0), Copyright (c) 2003-2008 Fabrice Bellard This keys off of only "emulator version", so should not have the same parsing issue. Signed-off-by: Chris Wright <chrisw@redhat.com> --- src/qemu/qemu_conf.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5fa8c0a..359c311 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1247,6 +1247,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, /* We parse the output of 'qemu -help' to get the QEMU * version number. The first bit is easy, just parse * 'QEMU PC emulator version x.y.z'. + * or + * 'QEMU emulator version x.y.z'. * * With qemu-kvm, however, that is followed by a string * in parenthesis as follows: @@ -1259,7 +1261,7 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STR "QEMU PC emulator version" +#define QEMU_VERSION_STR "emulator version" #define QEMU_KVM_VER_PREFIX "(qemu-kvm-" #define KVM_VER_PREFIX "(kvm-" @@ -1277,9 +1279,10 @@ int qemudParseHelpStr(const char *qemu, *flags = *version = *is_kvm = *kvm_version = 0; - if (!STRPREFIX(p, QEMU_VERSION_STR)) + p = strstr(p, QEMU_VERSION_STR); + if (!p) goto fail; p += strlen(QEMU_VERSION_STR);
Chris Wright wrote:
* Eric Blake (eblake@redhat.com) wrote:
On 05/19/2010 04:37 PM, Chris Wright wrote:
Looks like some cut'n paste error to me.
While we're at it, there have been some complaints, at least on IRC, that some recent qemu builds changed -help output to start with "QEMU emulator version" instead of "QEMU PC emulator version", which fails to match QEMU_VERSION_STR. Is that something we should be worrying about, while we're touching this function?
That's what had me looking at it ;)
[chrisw@x200 qemu-kvm]$ ./x86_64-softmmu/qemu-system-x86_64 -help | head -1 QEMU emulator version 0.12.50 (qemu-kvm-devel), Copyright (c) 2003-2008 Fabrice Bellard
[chrisw@x200 qemu-kvm]$ qemu-kvm -help | head -1 QEMU PC emulator version 0.11.0 (qemu-kvm-0.11.0), Copyright (c) 2003-2008 Fabrice Bellard
This keys off of only "emulator version", so should not have the same parsing issue.
Signed-off-by: Chris Wright <chrisw@redhat.com> --- src/qemu/qemu_conf.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5fa8c0a..359c311 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1247,6 +1247,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, /* We parse the output of 'qemu -help' to get the QEMU * version number. The first bit is easy, just parse * 'QEMU PC emulator version x.y.z'. + * or + * 'QEMU emulator version x.y.z'. * * With qemu-kvm, however, that is followed by a string * in parenthesis as follows: @@ -1259,7 +1261,7 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STR "QEMU PC emulator version" +#define QEMU_VERSION_STR "emulator version" #define QEMU_KVM_VER_PREFIX "(qemu-kvm-" #define KVM_VER_PREFIX "(kvm-"
@@ -1277,9 +1279,10 @@ int qemudParseHelpStr(const char *qemu,
*flags = *version = *is_kvm = *kvm_version = 0;
- if (!STRPREFIX(p, QEMU_VERSION_STR)) + p = strstr(p, QEMU_VERSION_STR); + if (!p) goto fail; p += strlen(QEMU_VERSION_STR);
Hi Chris, That patch looks fine, and is nicely minimal. However, it does relax the test significantly. Rather than requiring that QEMU_VERSION_STR be a prefix, it would allow it to appear anywhere within the -help output. While I really doubt it'd ever make a difference, it's easy to retain the stricter test, so I wrote this. Either way is fine with me. diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5c14eb8..f43c6e2 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1259,7 +1259,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STR "QEMU PC emulator version" +#define QEMU_VERSION_STR_1 "QEMU PC emulator version" +#define QEMU_VERSION_STR_2 "QEMU emulator version" #define QEMU_KVM_VER_PREFIX "(qemu-kvm-" #define KVM_VER_PREFIX "(kvm-" @@ -1277,11 +1278,13 @@ int qemudParseHelpStr(const char *qemu, *flags = *version = *is_kvm = *kvm_version = 0; - if (!STRPREFIX(p, QEMU_VERSION_STR)) + if (STRPREFIX(p, QEMU_VERSION_STR_1)) + p += strlen(QEMU_VERSION_STR_1); + else if (STRPREFIX(p, QEMU_VERSION_STR_2)) + p += strlen(QEMU_VERSION_STR_2); + else goto fail; - p += strlen(QEMU_VERSION_STR); - SKIP_BLANKS(p); major = virParseNumber(&p);
On Thu, May 20, 2010 at 03:39:05PM +0200, Jim Meyering wrote:
Chris Wright wrote:
* Eric Blake (eblake@redhat.com) wrote:
On 05/19/2010 04:37 PM, Chris Wright wrote:
Looks like some cut'n paste error to me.
While we're at it, there have been some complaints, at least on IRC, that some recent qemu builds changed -help output to start with "QEMU emulator version" instead of "QEMU PC emulator version", which fails to match QEMU_VERSION_STR. Is that something we should be worrying about, while we're touching this function?
That's what had me looking at it ;)
[chrisw@x200 qemu-kvm]$ ./x86_64-softmmu/qemu-system-x86_64 -help | head -1 QEMU emulator version 0.12.50 (qemu-kvm-devel), Copyright (c) 2003-2008 Fabrice Bellard
[chrisw@x200 qemu-kvm]$ qemu-kvm -help | head -1 QEMU PC emulator version 0.11.0 (qemu-kvm-0.11.0), Copyright (c) 2003-2008 Fabrice Bellard
This keys off of only "emulator version", so should not have the same parsing issue.
Signed-off-by: Chris Wright <chrisw@redhat.com> --- src/qemu/qemu_conf.c | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5fa8c0a..359c311 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1247,6 +1247,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, /* We parse the output of 'qemu -help' to get the QEMU * version number. The first bit is easy, just parse * 'QEMU PC emulator version x.y.z'. + * or + * 'QEMU emulator version x.y.z'. * * With qemu-kvm, however, that is followed by a string * in parenthesis as follows: @@ -1259,7 +1261,7 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STR "QEMU PC emulator version" +#define QEMU_VERSION_STR "emulator version" #define QEMU_KVM_VER_PREFIX "(qemu-kvm-" #define KVM_VER_PREFIX "(kvm-"
@@ -1277,9 +1279,10 @@ int qemudParseHelpStr(const char *qemu,
*flags = *version = *is_kvm = *kvm_version = 0;
- if (!STRPREFIX(p, QEMU_VERSION_STR)) + p = strstr(p, QEMU_VERSION_STR); + if (!p) goto fail; p += strlen(QEMU_VERSION_STR);
Hi Chris,
That patch looks fine, and is nicely minimal. However, it does relax the test significantly. Rather than requiring that QEMU_VERSION_STR be a prefix, it would allow it to appear anywhere within the -help output.
While I really doubt it'd ever make a difference, it's easy to retain the stricter test, so I wrote this. Either way is fine with me.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5c14eb8..f43c6e2 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1259,7 +1259,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STR "QEMU PC emulator version" +#define QEMU_VERSION_STR_1 "QEMU PC emulator version" +#define QEMU_VERSION_STR_2 "QEMU emulator version" #define QEMU_KVM_VER_PREFIX "(qemu-kvm-" #define KVM_VER_PREFIX "(kvm-"
@@ -1277,11 +1278,13 @@ int qemudParseHelpStr(const char *qemu,
*flags = *version = *is_kvm = *kvm_version = 0;
- if (!STRPREFIX(p, QEMU_VERSION_STR)) + if (STRPREFIX(p, QEMU_VERSION_STR_1)) + p += strlen(QEMU_VERSION_STR_1); + else if (STRPREFIX(p, QEMU_VERSION_STR_2)) + p += strlen(QEMU_VERSION_STR_2); + else goto fail;
- p += strlen(QEMU_VERSION_STR); - SKIP_BLANKS(p);
major = virParseNumber(&p);
ACK, prefer the stricter check. One day QEMU might even provide this in a reliably parsable format like JSON... 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 :|
Daniel P. Berrange wrote: ...
That patch looks fine, and is nicely minimal. However, it does relax the test significantly. Rather than requiring that QEMU_VERSION_STR be a prefix, it would allow it to appear anywhere within the -help output. ... ACK, prefer the stricter check. One day QEMU might even provide this in a reliably parsable format like JSON...
FYI, here's the complete patch I'll push in an hour or so. Note that I reordered, so that we check the new string before the "old" one.
From 3b7179866bd9e495332335df74b327cc156a298c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 20 May 2010 15:43:47 +0200 Subject: [PATCH] qemu_conf.c: also recognize new first line of qemu -help output
* src/qemu/qemu_conf.c (QEMU_VERSION_STR_1, QEMU_VERSION_STR_2): Define these instead of... (QEMU_VERSION_STR): ... this. Remove definition. (qemudParseHelpStr): Check first for the new, shorter prefix, "QEMU emulator version", and then for the old one, "QEMU PC emulator version" when trying to parse the version number. Based on a patch by Chris Wright. --- src/qemu/qemu_conf.c | 15 ++++++++++----- 1 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 5c14eb8..e1be340 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1246,7 +1246,9 @@ static unsigned long long qemudComputeCmdFlags(const char *help, /* We parse the output of 'qemu -help' to get the QEMU * version number. The first bit is easy, just parse - * 'QEMU PC emulator version x.y.z'. + * 'QEMU PC emulator version x.y.z' + * or + * 'QEMU emulator version x.y.z'. * * With qemu-kvm, however, that is followed by a string * in parenthesis as follows: @@ -1259,7 +1261,8 @@ static unsigned long long qemudComputeCmdFlags(const char *help, * and later, we just need the QEMU version number and * whether it is KVM QEMU or mainline QEMU. */ -#define QEMU_VERSION_STR "QEMU PC emulator version" +#define QEMU_VERSION_STR_1 "QEMU emulator version" +#define QEMU_VERSION_STR_2 "QEMU PC emulator version" #define QEMU_KVM_VER_PREFIX "(qemu-kvm-" #define KVM_VER_PREFIX "(kvm-" @@ -1277,11 +1280,13 @@ int qemudParseHelpStr(const char *qemu, *flags = *version = *is_kvm = *kvm_version = 0; - if (!STRPREFIX(p, QEMU_VERSION_STR)) + if (STRPREFIX(p, QEMU_VERSION_STR_1)) + p += strlen(QEMU_VERSION_STR_1); + else if (STRPREFIX(p, QEMU_VERSION_STR_2)) + p += strlen(QEMU_VERSION_STR_2); + else goto fail; - p += strlen(QEMU_VERSION_STR); - SKIP_BLANKS(p); major = virParseNumber(&p); -- 1.7.1.262.g5ef3d
* Jim Meyering (jim@meyering.net) wrote:
That patch looks fine, and is nicely minimal. However, it does relax the test significantly. Rather than requiring that QEMU_VERSION_STR be a prefix, it would allow it to appear anywhere within the -help output.
While I really doubt it'd ever make a difference, it's easy to retain the stricter test, so I wrote this. Either way is fine with me.
Yours works for me too. thanks, -chris
Chris Wright wrote:
Looks like some cut'n paste error to me.
Signed-off-by: Chris Wright <chrisw@redhat.com> --- src/qemu/qemu_conf.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 3e334dc..6d2ac6e 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1291,13 +1291,13 @@ int qemudParseHelpStr(const char *qemu, ++p;
minor = virParseNumber(&p); - if (major == -1 || *p != '.') + if (minor == -1 || *p != '.') goto fail;
++p;
micro = virParseNumber(&p); - if (major == -1) + if (micro == -1) goto fail;
SKIP_BLANKS(p);
Thanks, Chris. I've added to the log and will push this soon.
From e8353d366b761817850f9648084f5bf915175fdb Mon Sep 17 00:00:00 2001 From: Chris Wright <chrisw@redhat.com> Date: Wed, 19 May 2010 15:37:26 -0700 Subject: [PATCH] qemu driver: fix version check typos
* src/qemu/qemu_conf.c (qemudParseHelpStr): Fix errors that made it impossible to diagnose invalid minor and micro version number components. Signed-off-by: Chris Wright <chrisw@redhat.com> --- src/qemu/qemu_conf.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index be146fe..2e117c6 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1296,13 +1296,13 @@ int qemudParseHelpStr(const char *qemu, ++p; minor = virParseNumber(&p); - if (major == -1 || *p != '.') + if (minor == -1 || *p != '.') goto fail; ++p; micro = virParseNumber(&p); - if (major == -1) + if (micro == -1) goto fail; SKIP_BLANKS(p); -- 1.7.1.262.g5ef3d
participants (4)
-
Chris Wright -
Daniel P. Berrange -
Eric Blake -
Jim Meyering