Re: [Qemu-trivial] [PATCH v2 2/2] configure: disallow spaces and colons in source path and build path

[Laurent Vivier]

On 22/05/2019 17:01, Antonio Ospite wrote:
> On 22/05/19 15:57, Laurent Vivier wrote:
> > On 09/05/2019 16:42, Peter Maydell wrote:
> > > On Mon, 6 May 2019 at 18:27, Eric Blake <address@hidden> wrote:
> > > > On 5/3/19 3:27 AM, Antonio Ospite wrote:
> > > > > From: Antonio Ospite <address@hidden>
> > > > >
> > > > > The configure script breaks when the qemu source directory is in a 
> > > > > path
> > > > > containing white spaces, in particular the list of targets is not
> > > > > correctly generated when calling "./configure --help" because of 
> > > > > how the
> > > > > default_target_list variable is built.
> > > > >
> > > > > In addition to that, *building* qemu from a directory with spaces 
> > > > > breaks
> > > > > some assumptions in the Makefiles, even if the original source path 
> > > > > does
> > > > > not contain spaces like in the case of an out-of-tree build, or when
> > > > > symlinks are involved.
> > > > >
> > > > > To avoid these issues, refuse to run the configure script and the
> > > > > Makefile if there are spaces or colons in the source path or the build
> > > > > path, taking as inspiration what the kbuild system in linux does.
> > > > >
> > > > > Buglink: https://bugs.launchpad.net/qemu/+bug/1817345
> > > > >
> > > > > Signed-off-by: Antonio Ospite <address@hidden>
> > > > > ---
> > > > >   Makefile  | 4 ++++
> > > > >   configure | 6 ++++++
> > > > >   2 files changed, 10 insertions(+)
> > > >
> > > > > +++ b/Makefile
> > > > > @@ -1,5 +1,9 @@
> > > > >   # Makefile for QEMU.
> > > > >
> > > > > +ifneq ($(words $(subst :, ,$(CURDIR))), 1)
> > > > > +  $(error main directory cannot contain spaces nor colons)
> > > > > +endif
> > > > > +
> > > > >   # Always point to the root of the build tree (needs GNU make).
> > > > >   BUILD_DIR=$(CURDIR)
> > > > >
> > > > > diff --git a/configure b/configure
> > > > > index 9832cbca5c..f7ad4381bd 100755
> > > > > --- a/configure
> > > > > +++ b/configure
> > > > > @@ -279,6 +279,12 @@ ld_has() {
> > > > >   # make source path absolute
> > > > >   source_path=$(cd "$(dirname -- "$0")"; pwd)
> > > > >
> > > > > +if printf "%s\n" "$source_path" | grep -q "[[:space:]:]" ||
> > > > > +  printf "%s\n" "$PWD" | grep -q "[[:space:]:]";
> > > >
> > > > For less typing and fewer processes, you could shorten this to:
> > > >
> > > > if printf %s\\n "$source_path" "$PWD" | grep -q "[[:space:]:]";
> > > >
> > > > but that's trivial enough for a maintainer to fold in if desired.
> > > >
> > > > Reviewed-by: Eric Blake <address@hidden>
> > >
> > > What tree is this going to go in via? I suggest the
> > > -trivial tree.
> >
> >
> > Applied (unchanged) to my trivial-patches branch.
>
> Thank you Laurent.
>
> I'll think about sending a followup patch with the changes proposed by 
> Eric and I'll CC you if I do.

If you want to send a v3 of this patch to update it, I can wait.

Thanks,
Laurent