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

[Laurent Vivier <laurent@vivier.eu>]

On 22/05/2019 17:26, Antonio Ospite wrote:
> On 22/05/19 17:21, Laurent Vivier wrote:
>> 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 <eblake@redhat.com> wrote:
>>>>>>
>>>>>> On 5/3/19 3:27 AM, Antonio Ospite wrote:
>>>>>>> From: Antonio Ospite <antonio.ospite@collabora.com>
>>>>>>>
>>>>>>> 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 <antonio.ospite@collabora.com>
>>>>>>> ---
>>>>>>>   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 <eblake@redhat.com>
>>>>>
>>>>> 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.
> 
> That works too, I was waiting for the maintainers to decide what to do.
> 
> I'll try to send a v3 before Monday then.

OK, I will queue your v3 in the pull-request of the next week.

Thanks,
Laurent