Re: [Qemu-devel] [PATCH] configure: Add missing space when using --with-pkgversion

[Daniel P . Berrangé]

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 <address@hidden>
> >> ---
> >>   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 :|