Re: [Qemu-devel] [PATCH] configure: disallow spaces and colons in source path

[Antonio Ospite]

Hi, thanks for the comments.

On Fri, 15 Mar 2019 13:48:25 -0500
Eric Blake <address@hidden> wrote:

> On 3/15/19 1:40 PM, Peter Maydell wrote:
> 
> > If you do this after the point where we make the source path absolute, you
> > can skip the realpath (which avoids the problem that 'realpath' doesn't 
> > exist
> > on OSX by default). It will also then be after the handling of the
> > --source-path option argument.
> >
> > Do we also need to check for spaces in the path of the build directory
> > (which is always the current working directory of the script) ?
> 
> I wasn't thinking about VPATH builds, but yes, in general, we should
> ensure that both srcdir and builddir are sane names, while still
> allowing symlinks to work around otherwise problematic canonical names.
> 
[...]

On Fri, 15 Mar 2019 13:46:58 -0500
Eric Blake <address@hidden> wrote:
>
> Why realpath? If my sources live in "/home/me/bad dir" but I have a
> symlink "/home/me/good", this prevents me from building even though I
> won't trip the problem.
> 

The rationale behind the current patch was that the check should be
done as soon as possible to avoid unneeded work, and since $source_path
is a relative path early on in the script I thought about realpath.

TBH I used realpath unconditionally because I saw it in the Makefile
but I overlooked the fact that it is an internal function in make.

I will move the check after the expansion of $source_path.

After Eric's comment I also tried building from a sane symlink, and
while configure is fine with it "make" still does not like it for
in-tree builds: apparently CURDIR (used to set BUILD_PATH in the
Makefile) resolves symlinks and brings back the bad path.

I do get your point tho, do I did some testing to see the current status
and base the changes on that.

Assuming this setup:

├── no_spaces
│   ├── build
│   ├── qemu
│   └── qemulink -> ../with spaces/qemu
└── with spaces
    ├── build
    ├── qemu
    └── qemulink -> ../no_spaces/qemu

This are the results with the current master branch:

in-tree build:

no_spaces/qemu $ ./configure       # OK
no_spaces/qemu $ make              # OK

no_spaces/qemulink $ ./configure   # OK
no_spaces/qemulink $ make          # FAILS because of CURDIR

with spaces/qemu $ ./configure     # FAILS because of source_path
with spaces/qemu $ make            # FAILS because of SRC_PATH

with spaces/qemulink $ ./configure # FAILS because of source_path
with spaces/qemulink $ make        # FAILS because of SRC_PATH

out-of-tree builds:

no_spaces/build $ ../qemu/configure                 # OK
no_spaces/build $ make                              # OK

no_spaces/build $ ../qemulink/configure             # OK
no_spaces/build $ make                              # OK

no_spaces/build $ ../../with\ spaces/qemu/configure # FAILS
no_spaces/build $ make                              # FAILS no Makefile

no_spaces/build $ ../../with\ spaces/qemulink/configure # FAILS
no_spaces/build $ make                                  # FAILS ^

with spaces/build $ ../qemu/configure               # FAILS
with spaces/build $ make                            # FAILS no Makefile

with spaces/build $ ../qemulink/configure           # FAILS
with spaces/build $ make                            # FAILS no Makefile

with spaces/build $ ../../no_spaces/qemu/configure  # OK
with spaces/build $ make                            # FAILS (CURDIR)

with spaces/build $ ../../no_spaces/qemulink/configure # OK
with spaces/build $ make                               # FAILS (CURDIR)

So checking both source_path and PWD is a probably a good thing.

I'd add the check in the Makefile too, to be on the safe side and cover
the case of: no_spaces/qemulink $ make

Yeah, it is a slow Saturday. :)

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?