[flasher PATCH 1/2] Switch flasher script to subprocess

[flasher PATCH 1/2] Switch flasher script to subprocess

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

This avoids any issues re: quoting the commands we execute, such as
fdtput to modify the DTB sent to the flashing process.

The build script already uses subprocess, so this doesn't introduce any
new dependencies overall.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 tegra-uboot-flasher | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/tegra-uboot-flasher b/tegra-uboot-flasher
index ded8d171a14a..c9adba607c69 100755
--- a/tegra-uboot-flasher
+++ b/tegra-uboot-flasher
@@ -26,6 +26,7 @@ import os
 import os.path
 import shutil
 import stat
+import subprocess
 import sys
 import tempfile
 from tegraboardconfigs import *
@@ -42,15 +43,10 @@ def rmtree(path):
     if os.path.exists(path):
         shutil.rmtree(path)
 
-def run(dir, cmd):
-    oldcwd = os.getcwd()
-    print '+ cd', dir
-    os.chdir(dir)
-    print '+', cmd
-    ret = os.system(cmd)
-    if ret:
-        raise Exception('Command failed: %d' % ret)
-    os.chdir(oldcwd)
+def run(cd, cmd):
+    print '+ cd', cd
+    print '+', ' '.join(cmd)
+    subprocess.check_call(cmd, cwd=cd)
 
 def gen_flashcmd_mmc(flash_image_addr, readback_addr, flash_img_size):
     flash_id = config['flash-id-uboot']
@@ -97,7 +93,7 @@ def get_loadaddr():
 def gen_tegrarcm_cmd(bootloader, loadaddr):
     if type(loadaddr) != str:
         loadaddr = "0x%08x" % loadaddr
-    return 'tegrarcm --bct=' + bct + ' --bootloader=' + bootloader + ' --loadaddr=' + loadaddr
+    return ['tegrarcm', '--bct=' + bct, '--bootloader=' + bootloader , '--loadaddr=' + loadaddr]
 
 def find_config_dir():
     if not configs.has_key(args.configname):
@@ -198,8 +194,8 @@ def func_flash():
         u_boot_dtb_runflash = os.path.join(workdir, 'u-boot-runflash.dtb')
         cp(u_boot_dtb, u_boot_dtb_runflash)
 
-        # -2; never delay or interrupt
-        cmd = 'fdtput -p -t i ' + u_boot_dtb_runflash + ' /config bootdelay 0xfffffffe'
+        # 0xfffffffe==-2; never delay or interrupt
+        cmd = ['fdtput', '-p', '-t', 'i', u_boot_dtb_runflash, '/config', 'bootdelay', '0xfffffffe']
         run(workdir, cmd)
 
         bootcmd = ''
@@ -224,7 +220,7 @@ def func_flash():
         # If wanting to run installer, set installer_args.configname in environment, 'run bootcmd'
         bootcmd += 'reset'
         print 'bootcmd:', bootcmd
-        cmd = 'fdtput -p -t s ' + u_boot_dtb_runflash + ' /config bootcmd "' + bootcmd + '"'
+        cmd = ['fdtput', '-p', '-t', 's', u_boot_dtb_runflash, '/config', 'bootcmd', bootcmd]
         run(workdir, cmd)
 
         u_boot_dtb_runflash_size = os.path.getsize(u_boot_dtb_runflash)
@@ -249,12 +245,12 @@ def func_flash():
         os.fchmod(f.fileno(), stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO)
         f.write("#!/bin/sh\n")
         f.write("\n")
-        f.write(cmd)
+        f.write(' '.join(cmd))
         f.write("\n")
         f.close()
 
         if not args.gen_only:
-            run(workdir, flasher_sh)
+            run(workdir, [flasher_sh])
     except:
         raise
     finally:
-- 
1.8.1.5

[flasher PATCH 2/2] Wrap any env var settings in quotes

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

This allows run user to pass the following to the flashing script:

    --env bootcmd 'usb start ; dhcp zImage'

rather than having to manually escape the commands for U-Boot:

    --env bootcmd 'usb start \; dhcp zImage'

(The quoting in both cases is for the shell invoking tegra-uboot-flasher,
not for U-Boot's command-line. The removed escaping was to work around
the lack of quoting/escaping when passing the user's command to U-Boot's
setenv command during flashing.)

This change will interact badly with the user wanting to use single
quotes in environment variable values, but hopefully that's less likely,
and can be fixed later if needed.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 tegra-uboot-flasher | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tegra-uboot-flasher b/tegra-uboot-flasher
index c9adba607c69..ed2f2d91d336 100755
--- a/tegra-uboot-flasher
+++ b/tegra-uboot-flasher
@@ -213,7 +213,7 @@ def func_flash():
             bootcmd += 'setenv board ' + boardname + config['dtbfn-extra'] + ' ; '
         if args.env:
             for (var, value) in args.env:
-                bootcmd += 'setenv %s %s ; ' % (var, value)
+                bootcmd += 'setenv %s \'%s\' ; ' % (var, value)
         bootcmd += 'saveenv ; '
         bootcmd += 'echo >>> Flashing OK, rebooting... ; '
         # To update the bootloader, reset.
-- 
1.8.1.5

Re: [flasher PATCH 2/2] Wrap any env var settings in quotes

[Andreas Färber]

Hi,

Am 02.07.2014 19:32, schrieb Stephen Warren:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> This allows run user to pass the following to the flashing script:

Is "run user" a typo?

> 
>     --env bootcmd 'usb start ; dhcp zImage'
> 
> rather than having to manually escape the commands for U-Boot:
> 
>     --env bootcmd 'usb start \; dhcp zImage'
> 
> (The quoting in both cases is for the shell invoking tegra-uboot-flasher,
> not for U-Boot's command-line. The removed escaping was to work around
> the lack of quoting/escaping when passing the user's command to U-Boot's
> setenv command during flashing.)
> 
> This change will interact badly with the user wanting to use single
> quotes in environment variable values, but hopefully that's less likely,
> and can be fixed later if needed.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  tegra-uboot-flasher | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tegra-uboot-flasher b/tegra-uboot-flasher
> index c9adba607c69..ed2f2d91d336 100755
> --- a/tegra-uboot-flasher
> +++ b/tegra-uboot-flasher
> @@ -213,7 +213,7 @@ def func_flash():
>              bootcmd += 'setenv board ' + boardname + config['dtbfn-extra'] + ' ; '
>          if args.env:
>              for (var, value) in args.env:
> -                bootcmd += 'setenv %s %s ; ' % (var, value)
> +                bootcmd += 'setenv %s \'%s\' ; ' % (var, value)

Can't you just s/'/\\\'/g on value somehow, i.e. replace literal
single-quote with an escaped backslash, escaped single-quote sequence?

Regards,
Andreas

>          bootcmd += 'saveenv ; '
>          bootcmd += 'echo >>> Flashing OK, rebooting... ; '
>          # To update the bootloader, reset.

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [flasher PATCH 2/2] Wrap any env var settings in quotes

[Stephen Warren]

On 07/02/2014 12:42 PM, Andreas Färber wrote:
> Hi,
> 
> Am 02.07.2014 19:32, schrieb Stephen Warren:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> This allows run user to pass the following to the flashing script:
> 
> Is "run user" a typo?

s/run/the/ !

>>     --env bootcmd 'usb start ; dhcp zImage'
>>
>> rather than having to manually escape the commands for U-Boot:
>>
>>     --env bootcmd 'usb start \; dhcp zImage'

>> diff --git a/tegra-uboot-flasher b/tegra-uboot-flasher

>>          if args.env:
>>              for (var, value) in args.env:
>> -                bootcmd += 'setenv %s %s ; ' % (var, value)
>> +                bootcmd += 'setenv %s \'%s\' ; ' % (var, value)
> 
> Can't you just s/'/\\\'/g on value somehow, i.e. replace literal
> single-quote with an escaped backslash, escaped single-quote sequence?

I tried that manually at the command-line, and U-Boot's shell doesn't
seem to handle escaping quotes correctly. Escaped semicolons (which was
what I actually wanted to avoid writing manually) do work OK, but it's
simpler to just quote the whole thing rather than writing ever more
complicated parsers to quote certain characters or not based on whether
they're inside strings or not, etc. It'd be nice if U-Boot's setenv
could work like:

setenv -hex varname hex_encoded_value

... to completely avoid any quoting issues!

Re: [flasher PATCH 1/2] Switch flasher script to subprocess

[Stephen Warren]

On 07/02/2014 11:32 AM, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> This avoids any issues re: quoting the commands we execute, such as
> fdtput to modify the DTB sent to the flashing process.
> 
> The build script already uses subprocess, so this doesn't introduce any
> new dependencies overall.

I've applied the series, with the typo in patch 2's commit description
fixed.

Re: [flasher PATCH 1/2] Switch flasher script to subprocess

[Stephen Warren]

On 07/09/2014 02:38 AM, Vince Hsu wrote:
> Hi Stephen,
> 
> Sometimes I got the following errors with the tegra-uboot-flasher.
> Re-executing the script might work.

Sorry for the slow response; I was on vacation.

...
> downloading miniloader to target at address 0x4000e000 (136920 bytes)...
> miniloader downloaded successfully
> tegrarcm: retreiving platform info
> Traceback (most recent call last):
...
>   File "./tegra-uboot-flasher", line 49, in run
>     subprocess.check_call(cmd, cwd=cd)
>   File "/usr/lib/python2.7/subprocess.py", line 511, in check_call
>     raise CalledProcessError(retcode, cmd)
> subprocess.CalledProcessError: Command '['/tmp/tmpcgEOsW/flasher.sh']'
> returned non-zero exit status 1

I have not seen this. The error message simply means that the tegrarcm
binary exited with a non-zero error code. I think you'll need to run the
tegrarcm command under gdb (or add more debug prints) to find out why.
Passing the --debug and --save-work-dir parameters to
tegra-uboot-flasher will help you see exactly which commands and data
files tegra-uboot-flasher is executing, so you can run the same thing
manually.