[PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Abongwa Bonalais Amahnui]

Signed-off-by: Abongwa Bonalais Amahnui <abongwabonalais@gmail.com>
---
 scripts/bitbake-prserv-tool | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/scripts/bitbake-prserv-tool b/scripts/bitbake-prserv-tool
index e55d98c72e..68caa9fb66 100755
--- a/scripts/bitbake-prserv-tool
+++ b/scripts/bitbake-prserv-tool
@@ -5,7 +5,7 @@
 
 help ()
 {
-    base=`basename $0`
+    base=`basename "$0"`
     echo -e "Usage: $base command"
     echo "Avaliable commands:"
     echo -e "\texport <file.conf>: export and lock down the AUTOPR values from the PR service into a file for release."
@@ -16,7 +16,7 @@ clean_cache()
 {
     s=`bitbake -e | grep ^CACHE= | cut -f2 -d\"`
     if [ "x${s}" != "x" ]; then
-        rm -rf ${s}
+        rm -rf "${s}"
     fi
 }
 
@@ -24,14 +24,14 @@ do_export ()
 {
     file=$1
     [ "x${file}" == "x" ] && help && exit 1
-    rm -f ${file}
+    rm -f "${file}"
 
     clean_cache
     bitbake -R conf/prexport.conf -p
     s=`bitbake -R conf/prexport.conf -e | grep ^PRSERV_DUMPFILE= | cut -f2 -d\"`
     if [ "x${s}" != "x" ];
     then
-       [ -e $s ] && mv -f $s $file && echo "Exporting to file $file succeeded!"
+       [ -e "$s" ] && mv -f "$s" "$file" && echo "Exporting to file $file succeeded!"
        return 0
     fi
     echo "Exporting to file $file failed!"
@@ -44,7 +44,7 @@ do_import ()
     [ "x${file}" == "x" ] && help && exit 1
 
     clean_cache
-    bitbake -R conf/primport.conf -R $file -p
+    bitbake -R conf/primport.conf -R "$file" -p
     ret=$?
     [ $ret -eq 0 ] && echo "Importing from file $file succeeded!" || echo "Importing from file $file failed!"
     return $ret
@@ -60,13 +60,13 @@ do_migrate_localcount ()
         return 1
     fi
 
-    rm -rf $df
+    rm -rf "$df"
     clean_cache
     echo "Exporting LOCALCOUNT to AUTOINCs..."
     bitbake -R conf/migrate_localcount.conf -p
     [ ! $? -eq 0 ] && echo "Exporting to file $df failed!" && exit 1
 
-    if [ -e $df ];
+    if [ -e "$df" ];
     then
         echo "Exporting to file $df succeeded!"
     else
@@ -75,7 +75,7 @@ do_migrate_localcount ()
     fi
     
     echo "Importing generated AUTOINC entries..."
-    [ -e $df ] && do_import $df
+    [ -e "$df" ] && do_import "$df"
 
     if [ ! $? -eq 0 ]
     then
@@ -93,17 +93,17 @@ case $2 in
 *.conf|*.inc)
     ;;
 *)
-    echo ERROR: $2 must end with .conf or .inc!
+    echo ERROR: "$2" must end with .conf or .inc!
     exit 1
     ;;
 esac
 
 case $1 in
 export)
-    do_export $2
+    do_export "$2"
     ;;
 import)
-    do_import $2
+    do_import "$2"
     ;;
 migrate_localcount)
     do_migrate_localcount
-- 
2.25.1

Re: [OE-core] [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Mike Looijmans]

See comment below (our mail server injects signatures, sorry for that)


Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topicproducts.com
W: www.topic.nl

Please consider the environment before printing this e-mail
On 13-04-2022 11:35, Abongwa Amahnui Bonalais via lists.openembedded.org wrote:
> Signed-off-by: Abongwa Bonalais Amahnui <abongwabonalais@gmail.com>
> ---
>   scripts/bitbake-prserv-tool | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/scripts/bitbake-prserv-tool b/scripts/bitbake-prserv-tool
> index e55d98c72e..68caa9fb66 100755
> --- a/scripts/bitbake-prserv-tool
> +++ b/scripts/bitbake-prserv-tool
> @@ -5,7 +5,7 @@
>   
>   help ()
>   {
> -    base=`basename $0`
> +    base=`basename "$0"`
>       echo -e "Usage: $base command"
>       echo "Avaliable commands:"
>       echo -e "\texport <file.conf>: export and lock down the AUTOPR values from the PR service into a file for release."
> @@ -16,7 +16,7 @@ clean_cache()
>   {
>       s=`bitbake -e | grep ^CACHE= | cut -f2 -d\"`
>       if [ "x${s}" != "x" ]; then
> -        rm -rf ${s}
> +        rm -rf "${s}"
>       fi
>   }
>   
> @@ -24,14 +24,14 @@ do_export ()
>   {
>       file=$1

You'd want to quote this one too I think.

>       [ "x${file}" == "x" ] && help && exit 1
> -    rm -f ${file}
> +    rm -f "${file}"
>   
>       clean_cache
>       bitbake -R conf/prexport.conf -p
>       s=`bitbake -R conf/prexport.conf -e | grep ^PRSERV_DUMPFILE= | cut -f2 -d\"`
>       if [ "x${s}" != "x" ];
>       then
> -       [ -e $s ] && mv -f $s $file && echo "Exporting to file $file succeeded!"
> +       [ -e "$s" ] && mv -f "$s" "$file" && echo "Exporting to file $file succeeded!"
>          return 0
>       fi
>       echo "Exporting to file $file failed!"
> @@ -44,7 +44,7 @@ do_import ()
>       [ "x${file}" == "x" ] && help && exit 1
>   
>       clean_cache
> -    bitbake -R conf/primport.conf -R $file -p
> +    bitbake -R conf/primport.conf -R "$file" -p
>       ret=$?
>       [ $ret -eq 0 ] && echo "Importing from file $file succeeded!" || echo "Importing from file $file failed!"
>       return $ret
> @@ -60,13 +60,13 @@ do_migrate_localcount ()
>           return 1
>       fi
>   
> -    rm -rf $df
> +    rm -rf "$df"
>       clean_cache
>       echo "Exporting LOCALCOUNT to AUTOINCs..."
>       bitbake -R conf/migrate_localcount.conf -p
>       [ ! $? -eq 0 ] && echo "Exporting to file $df failed!" && exit 1
>   
> -    if [ -e $df ];
> +    if [ -e "$df" ];
>       then
>           echo "Exporting to file $df succeeded!"
>       else
> @@ -75,7 +75,7 @@ do_migrate_localcount ()
>       fi
>       
>       echo "Importing generated AUTOINC entries..."
> -    [ -e $df ] && do_import $df
> +    [ -e "$df" ] && do_import "$df"
>   
>       if [ ! $? -eq 0 ]
>       then
> @@ -93,17 +93,17 @@ case $2 in
>   *.conf|*.inc)
>       ;;
>   *)
> -    echo ERROR: $2 must end with .conf or .inc!
> +    echo ERROR: "$2" must end with .conf or .inc!
>       exit 1
>       ;;
>   esac
>   
>   case $1 in
>   export)
> -    do_export $2
> +    do_export "$2"
>       ;;
>   import)
> -    do_import $2
> +    do_import "$2"
>       ;;
>   migrate_localcount)
>       do_migrate_localcount
> 
> 
> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#164310): https://lists.openembedded.org/g/openembedded-core/message/164310
> Mute This Topic: https://lists.openembedded.org/mt/90437025/3618446
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [mike.looijmans@topic.nl]
> -=-=-=-=-=-=-=-=-=-=-=-
> 

Re: [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Abongwa Amahnui Bonalais]

[-- Attachment #1: Type: text/plain, Size: 78 bytes --]

hi Mike, I have sent a patch that has the changes from your review.
cheers.

[-- Attachment #2: Type: text/html, Size: 82 bytes --]

RE: [OE-core] [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Peter Kjellerstedt]

> -----Original Message-----
> From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> On Behalf Of Mike Looijmans
> Sent: den 13 april 2022 11:52
> To: openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

There shouldn't be quotes in the subject above. No idea how you 
managed to get them there...

Also, "gobbling" is not a word, AFAIK. Did you mean "globbing"?

> 
> See comment below (our mail server injects signatures, sorry for that)
> 
> 
> Met vriendelijke groet / kind regards,
> 
> Mike Looijmans
> System Expert
> 
> 
> TOPIC Embedded Products B.V.
> Materiaalweg 4, 5681 RJ Best
> The Netherlands
> 
> T: +31 (0) 499 33 69 69
> E: mike.looijmans@topicproducts.com
> W: www.topic.nl
> 
> Please consider the environment before printing this e-mail
> On 13-04-2022 11:35, Abongwa Amahnui Bonalais via lists.openembedded.org wrote:
> > Signed-off-by: Abongwa Bonalais Amahnui <abongwabonalais@gmail.com>
> > ---
> >   scripts/bitbake-prserv-tool | 22 +++++++++++-----------
> >   1 file changed, 11 insertions(+), 11 deletions(-)
> >
> > diff --git a/scripts/bitbake-prserv-tool b/scripts/bitbake-prserv-tool
> > index e55d98c72e..68caa9fb66 100755
> > --- a/scripts/bitbake-prserv-tool
> > +++ b/scripts/bitbake-prserv-tool
> > @@ -5,7 +5,7 @@
> >
> >   help ()
> >   {
> > -    base=`basename $0`
> > +    base=`basename "$0"`
> >       echo -e "Usage: $base command"
> >       echo "Avaliable commands:"
> >       echo -e "\texport <file.conf>: export and lock down the AUTOPR values from the PR service into a file for release."
> > @@ -16,7 +16,7 @@ clean_cache()
> >   {
> >       s=`bitbake -e | grep ^CACHE= | cut -f2 -d\"`
> >       if [ "x${s}" != "x" ]; then
> > -        rm -rf ${s}
> > +        rm -rf "${s}"
> >       fi
> >   }
> >
> > @@ -24,14 +24,14 @@ do_export ()
> >   {
> >       file=$1
> 
> You'd want to quote this one too I think.

Actually, as inconsistent as it may seem, quotes are _not_ needed above.

> 
> >       [ "x${file}" == "x" ] && help && exit 1
> > -    rm -f ${file}
> > +    rm -f "${file}"
> >
> >       clean_cache
> >       bitbake -R conf/prexport.conf -p
> >       s=`bitbake -R conf/prexport.conf -e | grep ^PRSERV_DUMPFILE= | cut -f2 -d\"`
> >       if [ "x${s}" != "x" ];
> >       then
> > -       [ -e $s ] && mv -f $s $file && echo "Exporting to file $file succeeded!"
> > +       [ -e "$s" ] && mv -f "$s" "$file" && echo "Exporting to file $file succeeded!"
> >          return 0
> >       fi
> >       echo "Exporting to file $file failed!"
> > @@ -44,7 +44,7 @@ do_import ()
> >       [ "x${file}" == "x" ] && help && exit 1
> >
> >       clean_cache
> > -    bitbake -R conf/primport.conf -R $file -p
> > +    bitbake -R conf/primport.conf -R "$file" -p
> >       ret=$?
> >       [ $ret -eq 0 ] && echo "Importing from file $file succeeded!" || echo "Importing from file $file failed!"
> >       return $ret
> > @@ -60,13 +60,13 @@ do_migrate_localcount ()
> >           return 1
> >       fi
> >
> > -    rm -rf $df
> > +    rm -rf "$df"
> >       clean_cache
> >       echo "Exporting LOCALCOUNT to AUTOINCs..."
> >       bitbake -R conf/migrate_localcount.conf -p
> >       [ ! $? -eq 0 ] && echo "Exporting to file $df failed!" && exit 1
> >
> > -    if [ -e $df ];
> > +    if [ -e "$df" ];
> >       then
> >           echo "Exporting to file $df succeeded!"
> >       else
> > @@ -75,7 +75,7 @@ do_migrate_localcount ()
> >       fi
> >
> >       echo "Importing generated AUTOINC entries..."
> > -    [ -e $df ] && do_import $df
> > +    [ -e "$df" ] && do_import "$df"
> >
> >       if [ ! $? -eq 0 ]
> >       then
> > @@ -93,17 +93,17 @@ case $2 in
> >   *.conf|*.inc)
> >       ;;
> >   *)
> > -    echo ERROR: $2 must end with .conf or .inc!
> > +    echo ERROR: "$2" must end with .conf or .inc!

The quote above is strictly not needed as the output from echo 
will be the same regardless if there are spaces in $2 or not. 
However, if you really want to quote that line, a more natural 
way would be:

    echo "ERROR: $2 must end with .conf or .inc!"

> >       exit 1
> >       ;;
> >   esac
> >
> >   case $1 in
> >   export)
> > -    do_export $2
> > +    do_export "$2"
> >       ;;
> >   import)
> > -    do_import $2
> > +    do_import "$2"
> >       ;;
> >   migrate_localcount)
> >       do_migrate_localcount

//Peter

Re: [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Abongwa Amahnui Bonalais]

[-- Attachment #1: Type: text/plain, Size: 161 bytes --]

I meant to say globbing sorry.
I will remove the quotes from the subject.

If quotes are not needed in the file=$1, does that means the first patch is fine?

[-- Attachment #2: Type: text/html, Size: 173 bytes --]

Re: [OE-core] [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Richard Purdie]

On Wed, 2022-04-13 at 10:35 +0100, Abongwa Amahnui Bonalais wrote:
> Signed-off-by: Abongwa Bonalais Amahnui <abongwabonalais@gmail.com>
> ---
>  scripts/bitbake-prserv-tool | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)

I want to suggest a little bit of caution with the direction these changes are
going.

Showing the user a clear error if that have spaces in a build path is a good
thing. Making oe-init-build-env handle that correctly is therefore desirable so
we can get the proper error from bitbake. This is likely the entry point most
users would find (toaster is another)

Since we don't support spaces in build paths and we're unlikely to do so given
that autotools can't, I'm not sure we want to go through too many of our scripts
doing this kind of change though. Most users would use the prserv tool as an
initial introduction to the project.

We did already see potential problems being introduced in the toaster changes on
the bitbake list and I do worry about potential regressions.

Cheers,

Richard

Re: [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Abongwa Amahnui Bonalais]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

Okay thank you what about Using $(...) notation instead of legacy backticked `...`.
base=`basename $0`, changing it to base=$(basename $0)

Should I commence those changes?
I'm seeing that based om recommendations when I run shellcheck <name of file>

[-- Attachment #2: Type: text/html, Size: 288 bytes --]

Re: [OE-core] [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Richard Purdie]

On Wed, 2022-04-13 at 03:50 -0700, Abongwa Amahnui Bonalais wrote:
> Okay thank you what about Using $(...) notation instead of legacy backticked
> `...`.
> base=`basename $0`, changing it to base=$(basename $0)
> 
> Should I commence those changes?
> I'm seeing that based om recommendations when I run shellcheck <name of file>

It depends on how you're testing the end result.

I don't mind improving the scripts but what we don't really want are patches
made by just doing what that tool says without the result being tested.

How are you testing the results?

Cheers,

Richard

Re: [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Abongwa Amahnui Bonalais]

[-- Attachment #1: Type: text/plain, Size: 180 bytes --]

I test the results by running bitbake core-image-sato.
If it builds successsfully, then I assume that it is fine,
Please what method do you recommend I use to test the results.

[-- Attachment #2: Type: text/html, Size: 194 bytes --]

Re: [OE-core] [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Richard Purdie]

On Wed, 2022-04-13 at 05:52 -0700, Abongwa Amahnui Bonalais wrote:
> I test the results by running bitbake core-image-sato.
> If it builds successsfully, then I assume that it is fine, 
> Please what method do you recommend I use to test the results.

You need to find a test which tests the component you're changing. "bitbake
core-image-sato" would test oe-init-build-env but it does not test toaster or 
bitbake-prserv-tool since that are not involved with that command.

An easy way to check is to actively break the script, put something in there you
know will break. If your test doesn't show it not working, it must not be
testing it.

Cheers,

Richard

Re: [PATCH] "bitbake-prserv-tool: Added quotes to variables to prevent splitting and gobbling"

[Abongwa Amahnui Bonalais]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]

Please is there a documentation that contains the possible commands and the files they affect?
If there is not, please what method do I use to know which command will affect a particular shell script except try and error?

[-- Attachment #2: Type: text/html, Size: 228 bytes --]