Patchwork [2/2] populate_sdk_base: ensure that filenames with empty space character are handled

login
register
mail settings
Submitter João Henrique Freitas
Date July 2, 2014, 1:58 a.m.
Message ID <1404266329-13524-3-git-send-email-joaohf@gmail.com>
Download mbox | patch
Permalink /patch/74571/
State New
Headers show

Comments

João Henrique Freitas - July 2, 2014, 1:58 a.m.
When extracting toolchain, if the list $executable_files has filenames
with empty space character, the list will created but relocate_sdk.sh
will not handle it well. This will lead to the below erro:

    ./tmp/deploy/sdk/buildtools-mytools-x86_64-nativesdk-standalone-1.6.1.0.sh
    Enter target directory for SDK (default: /opt/mydistro/mytoolset/1.6.1.0):
    You are about to install the SDK to "/opt/mydistro/mytoolset/1.6.1.0". Proceed[Y/n]?
    Extracting SDK...done
    Setting it up.../opt/mydistro/mytoolset/1.6.1.0/relocate_sdk.sh: line 2: sintaxe error `token'  `('
    /opt/mydistro/mytoolset/1.6.1.0/relocate_sdk.sh: line 2: `e

The same occurs with replacement of ${SDKPATH} in configs/scripts/etc files.

We should ensure that full path is protected before relocate_sdk.sh
and ${SDKPATH} replacement calls.

Signed-off-by: João Henrique Ferreira de Freitas <joaohf@gmail.com>
---
 meta/classes/populate_sdk_base.bbclass | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Saul Wold - July 9, 2014, 12:19 a.m.
On 07/01/2014 06:58 PM, João Henrique Ferreira de Freitas wrote:
> When extracting toolchain, if the list $executable_files has filenames
> with empty space character, the list will created but relocate_sdk.sh
> will not handle it well. This will lead to the below erro:
>
>      ./tmp/deploy/sdk/buildtools-mytools-x86_64-nativesdk-standalone-1.6.1.0.sh
>      Enter target directory for SDK (default: /opt/mydistro/mytoolset/1.6.1.0):
>      You are about to install the SDK to "/opt/mydistro/mytoolset/1.6.1.0". Proceed[Y/n]?
>      Extracting SDK...done
>      Setting it up.../opt/mydistro/mytoolset/1.6.1.0/relocate_sdk.sh: line 2: sintaxe error `token'  `('
>      /opt/mydistro/mytoolset/1.6.1.0/relocate_sdk.sh: line 2: `e
>
> The same occurs with replacement of ${SDKPATH} in configs/scripts/etc files.
>
> We should ensure that full path is protected before relocate_sdk.sh
> and ${SDKPATH} replacement calls.
>

I am not sure about this one, there are other cases where spaces in 
filenames/directory pathes just don't work, so fixing one place may not 
address all issues, I thought this was documented someplace, but I can't 
seem to find the associated bug or wiki entries.

Maybe someone else can remind me also where we talk about whitespace in 
pathe, obviously my search-fu is off right now.

Sau!

> Signed-off-by: João Henrique Ferreira de Freitas <joaohf@gmail.com>
> ---
>   meta/classes/populate_sdk_base.bbclass | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/meta/classes/populate_sdk_base.bbclass b/meta/classes/populate_sdk_base.bbclass
> index a12bf11..5311271 100644
> --- a/meta/classes/populate_sdk_base.bbclass
> +++ b/meta/classes/populate_sdk_base.bbclass
> @@ -252,7 +252,7 @@ if [ "$dl_path" = "" ] ; then
>   	echo "SDK could not be set up. Relocate script unable to find ld-linux.so. Abort!"
>   	exit 1
>   fi
> -executable_files=$($SUDO_EXEC find $native_sysroot -type f -perm /111)
> +executable_files=$($SUDO_EXEC find $native_sysroot -type f -perm /111 -exec echo \'{}\' \;)
>
>   tdir=`mktemp -d`
>   if [ x$tdir = x ] ; then
> @@ -273,7 +273,7 @@ if [ $relocate = 1 ] ; then
>   fi
>
>   # replace ${SDKPATH} with the new prefix in all text files: configs/scripts/etc
> -$SUDO_EXEC find $native_sysroot -type f -exec file '{}' \;|grep ":.*\(ASCII\|script\|source\).*text"|cut -d':' -f1|$SUDO_EXEC xargs sed -i -e "s:$DEFAULT_INSTALL_DIR:$target_sdk_dir:g"
> +$SUDO_EXEC find $native_sysroot -type f -exec file '{}' \;|grep ":.*\(ASCII\|script\|source\).*text"|cut -d':' -f1|tr "\n" "\0"|$SUDO_EXEC xargs -0 sed -i -e "s:$DEFAULT_INSTALL_DIR:$target_sdk_dir:g"
>
>   # change all symlinks pointing to ${SDKPATH}
>   for l in $($SUDO_EXEC find $native_sysroot -type l); do
>
Otavio Salvador - July 9, 2014, 1:33 a.m.
Hello Saul,

On Tue, Jul 8, 2014 at 9:19 PM, Saul Wold <sgw@linux.intel.com> wrote:
> On 07/01/2014 06:58 PM, João Henrique Ferreira de Freitas wrote:
>>
>> When extracting toolchain, if the list $executable_files has filenames
>> with empty space character, the list will created but relocate_sdk.sh
>> will not handle it well. This will lead to the below erro:
>>
>>
>> ./tmp/deploy/sdk/buildtools-mytools-x86_64-nativesdk-standalone-1.6.1.0.sh
>>      Enter target directory for SDK (default:
>> /opt/mydistro/mytoolset/1.6.1.0):
>>      You are about to install the SDK to
>> "/opt/mydistro/mytoolset/1.6.1.0". Proceed[Y/n]?
>>      Extracting SDK...done
>>      Setting it up.../opt/mydistro/mytoolset/1.6.1.0/relocate_sdk.sh: line
>> 2: sintaxe error `token'  `('
>>      /opt/mydistro/mytoolset/1.6.1.0/relocate_sdk.sh: line 2: `e
>>
>> The same occurs with replacement of ${SDKPATH} in configs/scripts/etc
>> files.
>>
>> We should ensure that full path is protected before relocate_sdk.sh
>> and ${SDKPATH} replacement calls.
>>
>
> I am not sure about this one, there are other cases where spaces in
> filenames/directory pathes just don't work, so fixing one place may not
> address all issues, I thought this was documented someplace, but I can't
> seem to find the associated bug or wiki entries.
>
> Maybe someone else can remind me also where we talk about whitespace in
> pathe, obviously my search-fu is off right now.

I agree this problem should exist in other places but I think this
does not invalidate Joao's patch and it should be reviewed and merged
if correct.

Now talking about  the patch itself, Joao, did you try to avoid the
extra fork calling echo? Using '-print0' option in find might help.
João Henrique Freitas - July 12, 2014, 1:20 p.m.
Hi Otavio

Em 08-07-2014 22:33, Otavio Salvador escreveu:
> Now talking about the patch itself, Joao, did you try to avoid the 
> extra fork calling echo? Using '-print0' option in find might help. 

Not until now. But I agree about your concern. So, instead of

executable_files=$($SUDO_EXEC find $native_sysroot -type f -perm /111 
-exec echo \'{}\' \;)

Its better do:

executable_files=$($SUDO_EXEC find $native_sysroot -type f -perm /111 
-printf "'%h/%f' ")

No extra fork was used with -printf "'%h/%f' ".

The argument -print0 is not an option because the output of find command 
will be 'path1 path2 path 3' a concatenation of strings not strings 
split by space and protected with ' '.

Both results are valid. But the second is nicer. What do you think?


When I did this patch I was not thinking to solve all problems about 
spaces in filename. However a deep dig is needed to track all points 
that have this issue.

Thanks.
Otavio Salvador - July 14, 2014, 3:10 p.m.
On Sat, Jul 12, 2014 at 10:20 AM, João Henrique Ferreira de Freitas
<joaohf@gmail.com> wrote:
>
> Hi Otavio
>
> Em 08-07-2014 22:33, Otavio Salvador escreveu:
>
>> Now talking about the patch itself, Joao, did you try to avoid the extra
>> fork calling echo? Using '-print0' option in find might help.
>
>
> Not until now. But I agree about your concern. So, instead of
>
>
> executable_files=$($SUDO_EXEC find $native_sysroot -type f -perm /111 -exec
> echo \'{}\' \;)
>
> Its better do:
>
> executable_files=$($SUDO_EXEC find $native_sysroot -type f -perm /111
> -printf "'%h/%f' ")
>
> No extra fork was used with -printf "'%h/%f' ".
>
> The argument -print0 is not an option because the output of find command
> will be 'path1 path2 path 3' a concatenation of strings not strings split by
> space and protected with ' '.
>
> Both results are valid. But the second is nicer. What do you think?

Nice.

> When I did this patch I was not thinking to solve all problems about spaces
> in filename. However a deep dig is needed to track all points that have this
> issue.

Fully agree but this shouldn't deny this patch to be applied. It is a
step further.

Patch

diff --git a/meta/classes/populate_sdk_base.bbclass b/meta/classes/populate_sdk_base.bbclass
index a12bf11..5311271 100644
--- a/meta/classes/populate_sdk_base.bbclass
+++ b/meta/classes/populate_sdk_base.bbclass
@@ -252,7 +252,7 @@  if [ "$dl_path" = "" ] ; then
 	echo "SDK could not be set up. Relocate script unable to find ld-linux.so. Abort!"
 	exit 1
 fi
-executable_files=$($SUDO_EXEC find $native_sysroot -type f -perm /111)
+executable_files=$($SUDO_EXEC find $native_sysroot -type f -perm /111 -exec echo \'{}\' \;)
 
 tdir=`mktemp -d`
 if [ x$tdir = x ] ; then
@@ -273,7 +273,7 @@  if [ $relocate = 1 ] ; then
 fi
 
 # replace ${SDKPATH} with the new prefix in all text files: configs/scripts/etc
-$SUDO_EXEC find $native_sysroot -type f -exec file '{}' \;|grep ":.*\(ASCII\|script\|source\).*text"|cut -d':' -f1|$SUDO_EXEC xargs sed -i -e "s:$DEFAULT_INSTALL_DIR:$target_sdk_dir:g"
+$SUDO_EXEC find $native_sysroot -type f -exec file '{}' \;|grep ":.*\(ASCII\|script\|source\).*text"|cut -d':' -f1|tr "\n" "\0"|$SUDO_EXEC xargs -0 sed -i -e "s:$DEFAULT_INSTALL_DIR:$target_sdk_dir:g"
 
 # change all symlinks pointing to ${SDKPATH}
 for l in $($SUDO_EXEC find $native_sysroot -type l); do