Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion
On 02/15/2018 04:08 AM, Daniel P. Berrangé wrote:
Hmm - here you're changing who supplies the (). But that argues that
maybe the callsites should supply " (" and ")" themselves.
Yeah, that's likely the saner way to do this. The question is: What
about the query-version QMP command? Should it report parentheses or
not? I think I'd look nicer if it reports "package": "foo" instead of
"package": "(foo)" - but we maybe could break some users who expect
parentheses there (no matter whether there is a preceding space or not)?
The pkgversion is an opaque string - users/apps should never try to
interpret its contents, because its format can vary arbitrarily between
distros. It is merely intended as an informative string to help the
package maintainer identify which version of QEMU was used when someone
submits a bug reoprt.
IOW it is totally valid to change the command to omit '()', and if anything
breaks that is their own fault for trying to interpret an opaque blob of
data.
Agreed - the fact that we had a leading space in the QMP output for a
long time (and even inconsistent at whether it was there), with no one
noticing, means that dropping the () from QMP won't hurt anyone.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion
On Thu, Feb 15, 2018 at 07:02:40AM +0100, Thomas Huth wrote:
> On 14.02.2018 21:23, Eric Blake wrote:
> > On 02/14/2018 11:31 AM, Thomas Huth wrote:
> >> When running configure with --with-pkgversion=foo there is no
> >> space anymore between the version number and the parentheses:
> >>
> >> $ m68k-softmmu/qemu-system-m68k -version
> >> QEMU emulator version 2.11.50(foo)
> >>
> >> Fix it by moving the space from the configure script to the Makefile.
> >>
> >> Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979
> >> Buglink: https://bugs.launchpad.net/qemu/+bug/1673373
> >> Signed-off-by: Thomas Huth
> >> ---
> >> Makefile | 2 +-
> >> configure | 2 +-
> >> 2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/Makefile b/Makefile
> >> index 4ec7a3c..41adbc9 100644
> >> --- a/Makefile
> >> +++ b/Makefile
> >> @@ -369,7 +369,7 @@ qemu-version.h: FORCE
> >> (cd $(SRC_PATH); \
> >> printf '#define QEMU_PKGVERSION '; \
> >> if test -n "$(PKGVERSION)"; then \
> >> - printf '"$(PKGVERSION)"\n'; \
> >> + printf '" ($(PKGVERSION))"\n'; \
> >
> > I would argue that putting a space here is awkward; wouldn't it instead
> > be easier to have all CLIENTS of QEMU_PKGVERSION in the source code
> > assume that the macro does NOT have a leading space, and to supply a
> > space themselves?
> >
> > That is, change THESE locations:
> >
> > bsd-user/main.c: printf("qemu-" TARGET_NAME " version " QEMU_VERSION
> > QEMU_PKGVERSION
> > linux-user/main.c: printf("qemu-" TARGET_NAME " version "
> > QEMU_VERSION QEMU_PKGVERSION
> > qemu-img.c:#define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION
> > QEMU_PKGVERSION \
> > qemu-io.c: printf("%s version " QEMU_VERSION QEMU_PKGVERSION
> > "\n"
> > qemu-nbd.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n"
> > qga/main.c:"QEMU Guest Agent " QEMU_VERSION QEMU_PKGVERSION "\n"
> > scsi/qemu-pr-helper.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n"
> > ui/cocoa.m: @"QEMU emulator version %s%s", QEMU_VERSION,
> > QEMU_PKGVERSION];
> > vl.c: printf("QEMU emulator version " QEMU_VERSION QEMU_PKGVERSION "\n"
> >
> > to instead supply the missing space, and have configure/Makefile always
> > generate without a leading space.
> >
> >> +++ b/configure
> >> @@ -1162,7 +1162,7 @@ for opt do
> >> ;;
> >> --disable-blobs) blobs="no"
> >> ;;
> >> - --with-pkgversion=*) pkgversion=" ($optarg)"
> >> + --with-pkgversion=*) pkgversion="$optarg"
> >
> > Hmm - here you're changing who supplies the (). But that argues that
> > maybe the callsites should supply " (" and ")" themselves.
>
> Yeah, that's likely the saner way to do this. The question is: What
> about the query-version QMP command? Should it report parentheses or
> not? I think I'd look nicer if it reports "package": "foo" instead of
> "package": "(foo)" - but we maybe could break some users who expect
> parentheses there (no matter whether there is a preceding space or not)?
The pkgversion is an opaque string - users/apps should never try to
interpret its contents, because its format can vary arbitrarily between
distros. It is merely intended as an informative string to help the
package maintainer identify which version of QEMU was used when someone
submits a bug reoprt.
IOW it is totally valid to change the command to omit '()', and if anything
breaks that is their own fault for trying to interpret an opaque blob of
data.
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion
On 14.02.2018 21:23, Eric Blake wrote:
> On 02/14/2018 11:31 AM, Thomas Huth wrote:
>> When running configure with --with-pkgversion=foo there is no
>> space anymore between the version number and the parentheses:
>>
>> $ m68k-softmmu/qemu-system-m68k -version
>> QEMU emulator version 2.11.50(foo)
>>
>> Fix it by moving the space from the configure script to the Makefile.
>>
>> Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1673373
>> Signed-off-by: Thomas Huth
>> ---
>> Makefile | 2 +-
>> configure | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 4ec7a3c..41adbc9 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -369,7 +369,7 @@ qemu-version.h: FORCE
>> (cd $(SRC_PATH); \
>> printf '#define QEMU_PKGVERSION '; \
>> if test -n "$(PKGVERSION)"; then \
>> - printf '"$(PKGVERSION)"\n'; \
>> + printf '" ($(PKGVERSION))"\n'; \
>
> I would argue that putting a space here is awkward; wouldn't it instead
> be easier to have all CLIENTS of QEMU_PKGVERSION in the source code
> assume that the macro does NOT have a leading space, and to supply a
> space themselves?
>
> That is, change THESE locations:
>
> bsd-user/main.c: printf("qemu-" TARGET_NAME " version " QEMU_VERSION
> QEMU_PKGVERSION
> linux-user/main.c: printf("qemu-" TARGET_NAME " version "
> QEMU_VERSION QEMU_PKGVERSION
> qemu-img.c:#define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION
> QEMU_PKGVERSION \
> qemu-io.c: printf("%s version " QEMU_VERSION QEMU_PKGVERSION
> "\n"
> qemu-nbd.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n"
> qga/main.c:"QEMU Guest Agent " QEMU_VERSION QEMU_PKGVERSION "\n"
> scsi/qemu-pr-helper.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n"
> ui/cocoa.m: @"QEMU emulator version %s%s", QEMU_VERSION,
> QEMU_PKGVERSION];
> vl.c: printf("QEMU emulator version " QEMU_VERSION QEMU_PKGVERSION "\n"
>
> to instead supply the missing space, and have configure/Makefile always
> generate without a leading space.
>
>> +++ b/configure
>> @@ -1162,7 +1162,7 @@ for opt do
>> ;;
>> --disable-blobs) blobs="no"
>> ;;
>> - --with-pkgversion=*) pkgversion=" ($optarg)"
>> + --with-pkgversion=*) pkgversion="$optarg"
>
> Hmm - here you're changing who supplies the (). But that argues that
> maybe the callsites should supply " (" and ")" themselves.
Yeah, that's likely the saner way to do this. The question is: What
about the query-version QMP command? Should it report parentheses or
not? I think I'd look nicer if it reports "package": "foo" instead of
"package": "(foo)" - but we maybe could break some users who expect
parentheses there (no matter whether there is a preceding space or not)?
Thomas
Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion
On 02/14/2018 11:31 AM, Thomas Huth wrote:
When running configure with --with-pkgversion=foo there is no
space anymore between the version number and the parentheses:
$ m68k-softmmu/qemu-system-m68k -version
QEMU emulator version 2.11.50(foo)
Fix it by moving the space from the configure script to the Makefile.
Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979
Buglink: https://bugs.launchpad.net/qemu/+bug/1673373
Signed-off-by: Thomas Huth
---
Makefile | 2 +-
configure | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile
index 4ec7a3c..41adbc9 100644
--- a/Makefile
+++ b/Makefile
@@ -369,7 +369,7 @@ qemu-version.h: FORCE
(cd $(SRC_PATH); \
printf '#define QEMU_PKGVERSION '; \
if test -n "$(PKGVERSION)"; then \
- printf '"$(PKGVERSION)"\n'; \
+ printf '" ($(PKGVERSION))"\n'; \
I would argue that putting a space here is awkward; wouldn't it instead
be easier to have all CLIENTS of QEMU_PKGVERSION in the source code
assume that the macro does NOT have a leading space, and to supply a
space themselves?
That is, change THESE locations:
bsd-user/main.c:printf("qemu-" TARGET_NAME " version " QEMU_VERSION
QEMU_PKGVERSION
linux-user/main.c:printf("qemu-" TARGET_NAME " version "
QEMU_VERSION QEMU_PKGVERSION
qemu-img.c:#define QEMU_IMG_VERSION "qemu-img version " QEMU_VERSION
QEMU_PKGVERSION \
qemu-io.c:printf("%s version " QEMU_VERSION QEMU_PKGVERSION "\n"
qemu-nbd.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n"
qga/main.c:"QEMU Guest Agent " QEMU_VERSION QEMU_PKGVERSION "\n"
scsi/qemu-pr-helper.c:"%s " QEMU_VERSION QEMU_PKGVERSION "\n"
ui/cocoa.m:@"QEMU emulator version %s%s", QEMU_VERSION,
QEMU_PKGVERSION];
vl.c:printf("QEMU emulator version " QEMU_VERSION QEMU_PKGVERSION "\n"
to instead supply the missing space, and have configure/Makefile always
generate without a leading space.
+++ b/configure
@@ -1162,7 +1162,7 @@ for opt do
;;
--disable-blobs) blobs="no"
;;
- --with-pkgversion=*) pkgversion=" ($optarg)"
+ --with-pkgversion=*) pkgversion="$optarg"
Hmm - here you're changing who supplies the (). But that argues that
maybe the callsites should supply " (" and ")" themselves.
Here's how coreutils does it, by using gnulib's version-etc module:
src/local.mk: $(AM_V_at)printf 'char const *Version =
"$(PACKAGE_VERSION)";\n' >> $@t
src/system.h:version_etc (stdout, Program_name, PACKAGE_NAME,
Version, Authors,
version_etc_arn (FILE *stream,
const char *command_name, const char *package,
const char *version,
const char * const * authors, size_t n_authors)
{
if (command_name)
fprintf (stream, "%s (%s) %s\n", command_name, package, version);
which means that the Makefile magic outputs JUST the text that goes
inside the (), and the callsite that outputs version information is what
supplies the " (" and ")".
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion
On 02/14/2018 02:00 PM, Thomas Huth wrote:
On 14.02.2018 19:33, Peter Maydell wrote:
On 14 February 2018 at 17:31, Thomas Huth wrote:
When running configure with --with-pkgversion=foo there is no
space anymore between the version number and the parentheses:
$ m68k-softmmu/qemu-system-m68k -version
QEMU emulator version 2.11.50(foo)
It would be nice to document this as '--version' rather than '-version'
(both work; but see our BiteSized task:
https://wiki.qemu.org/BiteSizedTasks#Consistent_option_usage_in_documentation)
-- does this patch change the QMP reported string
unexpectedly?
Without my patch and with --with-pkgversion=foo :
{ "execute": "query-version" }
{"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": "(foo)"}}
Looks nice. And the version info is ALSO passed as part of the initial
handshake, even before you call query-version.
With my patch and with --with-pkgversion=foo :
{ "execute": "query-version" }
{"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": "
(foo)"}}
Potential regression (arguably cosmetic, though)
Without my patch and without --with-pkgversion :
{ "execute": "query-version" }
{"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": "
(v2.11.0-1512-g02f4fbe)"}}
And that means we're already inconsistent, so your patch at least made
things consistent,
Using the old QEMU version 67a1de0d19~1 with --with-pkgversion=foo :
{ "execute": "query-version" }
{"return": {"qemu": {"micro": 50, "minor": 6, "major": 2}, "package": " (foo)"}}
and that means we've already "regressed", which further means:
- it definitely is cosmetic, if no one is complaining
- changing it to look nicer won't break anyone
So yes, this patch changes the behavior of query-version, but the new
behavior is now the same behavior that you get without --with-pkgversion
(i.e. a space is included) and it is the same as the behavior that we had
in the past, before commit 67a1de0d19 has been merged. So I think this
is the right way to go.
Alternatively, we could maybe change query-version to always skip the
initial space?
Yes, I like that option.
So, if I'm summarizing correctly, your v2 patch will have:
$ qemu --version
QEMU emulator version 2.11.50 (foo)
QMP query-version
{"return": {"qemu": {"micro": 50, "minor": 6, "major": 2}, "package":
"(foo)"}}
regardless of whether --with-pkgversion was used during configure.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion
On 14.02.2018 19:33, Peter Maydell wrote:
> On 14 February 2018 at 17:31, Thomas Huth wrote:
>> When running configure with --with-pkgversion=foo there is no
>> space anymore between the version number and the parentheses:
>>
>> $ m68k-softmmu/qemu-system-m68k -version
>> QEMU emulator version 2.11.50(foo)
>>
>> Fix it by moving the space from the configure script to the Makefile.
>>
>> Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1673373
>> Signed-off-by: Thomas Huth
>
> I see that in the bug report I wrote
> "Also it looks like we return QEMU_PKGVERSION as part of the
> QMP qmp_query_version() code, so we should check to see what
> the expected behaviour there is regarding having the space
> or not."
>
> -- does this patch change the QMP reported string
> unexpectedly?
Without my patch and with --with-pkgversion=foo :
{ "execute": "query-version" }
{"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": "(foo)"}}
With my patch and with --with-pkgversion=foo :
{ "execute": "query-version" }
{"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": "
(foo)"}}
Without my patch and without --with-pkgversion :
{ "execute": "query-version" }
{"return": {"qemu": {"micro": 50, "minor": 11, "major": 2}, "package": "
(v2.11.0-1512-g02f4fbe)"}}
Using the old QEMU version 67a1de0d19~1 with --with-pkgversion=foo :
{ "execute": "query-version" }
{"return": {"qemu": {"micro": 50, "minor": 6, "major": 2}, "package": " (foo)"}}
So yes, this patch changes the behavior of query-version, but the new
behavior is now the same behavior that you get without --with-pkgversion
(i.e. a space is included) and it is the same as the behavior that we had
in the past, before commit 67a1de0d19 has been merged. So I think this
is the right way to go.
Alternatively, we could maybe change query-version to always skip the
initial space?
Thomas
Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion
On 14 February 2018 at 17:31, Thomas Huth wrote: > When running configure with --with-pkgversion=foo there is no > space anymore between the version number and the parentheses: > > $ m68k-softmmu/qemu-system-m68k -version > QEMU emulator version 2.11.50(foo) > > Fix it by moving the space from the configure script to the Makefile. > > Fixes: 67a1de0d195a6185c39b436159c9ffc7720bf979 > Buglink: https://bugs.launchpad.net/qemu/+bug/1673373 > Signed-off-by: Thomas Huth I see that in the bug report I wrote "Also it looks like we return QEMU_PKGVERSION as part of the QMP qmp_query_version() code, so we should check to see what the expected behaviour there is regarding having the space or not." -- does this patch change the QMP reported string unexpectedly? thanks -- PMM
