Re: [OE-core] [PATCH 3/3 v2] meta: Fix return value checks from subprocess.call()'s

[Mikko.Rapeli]

Hi,

On Thu, Jun 01, 2017 at 06:53:14PM +0300, Mikko Rapeli wrote:
> Python function subprocess.call() returns the return value of the
> executed process. If return values are not checked, errors may
> go unnoticed and bad things can happen.
> 
> Change all callers of subprocess.call() which do not check for
> the return value to use subprocess.check_call() which raises
> CalledProcessError if the subprocess returns with non-zero value.
> 
> https://docs.python.org/2/library/subprocess.html#using-the-subprocess-module
> 
> All users of the function were found with:
> 
> $ git grep "subprocess\.call" | \
>   egrep -v 'if.*subprocess\.call|=\ 
> +subprocess\.call|return.*subprocess\.call'
> 
> Tested similar patch on top of yocto jethro. Only compile tested
> core-image-minimal on poky master branch.

The bitbake and scripts parts from this patch series were merged already, but
this not.

Are there problems with this patch?

-Mikko

> Signed-off-by: Mikko Rapeli 
> ---
>  meta/classes/archiver.bbclass| 2 +-
>  meta/classes/cml1.bbclass| 2 +-
>  meta/classes/kernel-module-split.bbclass | 2 +-
>  meta/classes/sstate.bbclass  | 4 ++--
>  meta/lib/oeqa/utils/buildproject.py  | 2 +-
>  meta/lib/oeqa/utils/targetbuild.py   | 4 ++--
>  meta/recipes-extended/cups/cups.inc  | 2 +-
>  7 files changed, 9 insertions(+), 9 deletions(-)
> 
> v2:
> split patches to bitbake, meta and scripts
> 
> v1:
> http://lists.openembedded.org/pipermail/openembedded-core/2017-May/136901.html
> 
> diff --git a/meta/classes/archiver.bbclass b/meta/classes/archiver.bbclass
> index 2c04557..703eacb 100644
> --- a/meta/classes/archiver.bbclass
> +++ b/meta/classes/archiver.bbclass
> @@ -288,7 +288,7 @@ def create_diff_gz(d, src_orig, src, ar_outdir):
>  os.chdir(dirname)
>  out_file = os.path.join(ar_outdir, '%s-diff.gz' % d.getVar('PF'))
>  diff_cmd = 'diff -Naur %s.orig %s.patched | gzip -c > %s' % (basename, 
> basename, out_file)
> -subprocess.call(diff_cmd, shell=True)
> +subprocess.check_call(diff_cmd, shell=True)
>  bb.utils.remove(src_patched, recurse=True)
>  
>  # Run do_unpack and do_patch
> diff --git a/meta/classes/cml1.bbclass b/meta/classes/cml1.bbclass
> index 38e6613..eb8e790 100644
> --- a/meta/classes/cml1.bbclass
> +++ b/meta/classes/cml1.bbclass
> @@ -63,7 +63,7 @@ python do_diffconfig() {
>  
>  if isdiff:
>  statement = 'diff --unchanged-line-format= --old-line-format= 
> --new-line-format="%L" ' + configorig + ' ' + config + '>' + fragment
> -subprocess.call(statement, shell=True)
> +subprocess.check_call(statement, shell=True)
>  
>  shutil.copy(configorig, config)
>  
> diff --git a/meta/classes/kernel-module-split.bbclass 
> b/meta/classes/kernel-module-split.bbclass
> index 5e10dcf..1035525 100644
> --- a/meta/classes/kernel-module-split.bbclass
> +++ b/meta/classes/kernel-module-split.bbclass
> @@ -47,7 +47,7 @@ python split_kernel_module_packages () {
>  tf = tempfile.mkstemp()
>  tmpfile = tf[1]
>  cmd = "%sobjcopy -j .modinfo -O binary %s %s" % 
> (d.getVar("HOST_PREFIX") or "", file, tmpfile)
> -subprocess.call(cmd, shell=True)
> +subprocess.check_call(cmd, shell=True)
>  f = open(tmpfile)
>  l = f.read().split("\000")
>  f.close()
> diff --git a/meta/classes/sstate.bbclass b/meta/classes/sstate.bbclass
> index 0a12935..f446c3d 100644
> --- a/meta/classes/sstate.bbclass
> +++ b/meta/classes/sstate.bbclass
> @@ -404,7 +404,7 @@ python sstate_hardcode_path_unpack () {
>  return
>  
>  bb.note("Replacing fixme paths in sstate package: %s" % 
> (sstate_hardcode_cmd))
> -subprocess.call(sstate_hardcode_cmd, shell=True)
> +subprocess.check_call(sstate_hardcode_cmd, shell=True)
>  
>  # Need to remove this or we'd copy it into the target directory and 
> may 
>  # conflict with another writer
> @@ -453,7 +453,7 @@ def sstate_clean_manifest(manifest, d, prefix=None):
>  if os.path.exists(manifest + ".postrm"):
>  import subprocess
>  os.chmod(postrm, 0o755)
> -subprocess.call(postrm, shell=True)
> +subprocess.check_call(postrm, shell=True)
>  oe.path.remove(postrm)
>  
>  oe.path.remove(manifest)
> diff --git a/meta/lib/oeqa/utils/buildproject.py 
> b/meta/lib/oeqa/utils/buildproject.py
> index 487f08b..721f35d 100644
> --- a/meta/lib/oeqa/utils/buildproject.py
> +++ b/meta/lib/oeqa/utils/buildproject.py
> @@ -52,4 +52,4 @@ class BuildProject(metaclass=ABCMeta):
>  
>  def clean(self):
>  self._run('rm -rf %s' % self.targetdir)
> -subprocess.call('rm -f %s' % self.localarchive, shell=True)
> +subprocess.check_call('rm -f %s' % self.localarchive, shell=True)
> diff --git a/meta/lib/oeqa/utils/targetbuild.py 
> b/meta/lib/oeqa/utils/targetbuild.py
> index 9249fa2..1202d57 100644
> --- 

[OE-core] [PATCH 3/3 v2] meta: Fix return value checks from subprocess.call()'s

[Mikko Rapeli]

Python function subprocess.call() returns the return value of the
executed process. If return values are not checked, errors may
go unnoticed and bad things can happen.

Change all callers of subprocess.call() which do not check for
the return value to use subprocess.check_call() which raises
CalledProcessError if the subprocess returns with non-zero value.

https://docs.python.org/2/library/subprocess.html#using-the-subprocess-module

All users of the function were found with:

$ git grep "subprocess\.call" | \
  egrep -v 'if.*subprocess\.call|=\ +subprocess\.call|return.*subprocess\.call'

Tested similar patch on top of yocto jethro. Only compile tested
core-image-minimal on poky master branch.

Signed-off-by: Mikko Rapeli 
---
 meta/classes/archiver.bbclass| 2 +-
 meta/classes/cml1.bbclass| 2 +-
 meta/classes/kernel-module-split.bbclass | 2 +-
 meta/classes/sstate.bbclass  | 4 ++--
 meta/lib/oeqa/utils/buildproject.py  | 2 +-
 meta/lib/oeqa/utils/targetbuild.py   | 4 ++--
 meta/recipes-extended/cups/cups.inc  | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

v2:
split patches to bitbake, meta and scripts

v1:
http://lists.openembedded.org/pipermail/openembedded-core/2017-May/136901.html

diff --git a/meta/classes/archiver.bbclass b/meta/classes/archiver.bbclass
index 2c04557..703eacb 100644
--- a/meta/classes/archiver.bbclass
+++ b/meta/classes/archiver.bbclass
@@ -288,7 +288,7 @@ def create_diff_gz(d, src_orig, src, ar_outdir):
 os.chdir(dirname)
 out_file = os.path.join(ar_outdir, '%s-diff.gz' % d.getVar('PF'))
 diff_cmd = 'diff -Naur %s.orig %s.patched | gzip -c > %s' % (basename, 
basename, out_file)
-subprocess.call(diff_cmd, shell=True)
+subprocess.check_call(diff_cmd, shell=True)
 bb.utils.remove(src_patched, recurse=True)
 
 # Run do_unpack and do_patch
diff --git a/meta/classes/cml1.bbclass b/meta/classes/cml1.bbclass
index 38e6613..eb8e790 100644
--- a/meta/classes/cml1.bbclass
+++ b/meta/classes/cml1.bbclass
@@ -63,7 +63,7 @@ python do_diffconfig() {
 
 if isdiff:
 statement = 'diff --unchanged-line-format= --old-line-format= 
--new-line-format="%L" ' + configorig + ' ' + config + '>' + fragment
-subprocess.call(statement, shell=True)
+subprocess.check_call(statement, shell=True)
 
 shutil.copy(configorig, config)
 
diff --git a/meta/classes/kernel-module-split.bbclass 
b/meta/classes/kernel-module-split.bbclass
index 5e10dcf..1035525 100644
--- a/meta/classes/kernel-module-split.bbclass
+++ b/meta/classes/kernel-module-split.bbclass
@@ -47,7 +47,7 @@ python split_kernel_module_packages () {
 tf = tempfile.mkstemp()
 tmpfile = tf[1]
 cmd = "%sobjcopy -j .modinfo -O binary %s %s" % 
(d.getVar("HOST_PREFIX") or "", file, tmpfile)
-subprocess.call(cmd, shell=True)
+subprocess.check_call(cmd, shell=True)
 f = open(tmpfile)
 l = f.read().split("\000")
 f.close()
diff --git a/meta/classes/sstate.bbclass b/meta/classes/sstate.bbclass
index 0a12935..f446c3d 100644
--- a/meta/classes/sstate.bbclass
+++ b/meta/classes/sstate.bbclass
@@ -404,7 +404,7 @@ python sstate_hardcode_path_unpack () {
 return
 
 bb.note("Replacing fixme paths in sstate package: %s" % 
(sstate_hardcode_cmd))
-subprocess.call(sstate_hardcode_cmd, shell=True)
+subprocess.check_call(sstate_hardcode_cmd, shell=True)
 
 # Need to remove this or we'd copy it into the target directory and 
may 
 # conflict with another writer
@@ -453,7 +453,7 @@ def sstate_clean_manifest(manifest, d, prefix=None):
 if os.path.exists(manifest + ".postrm"):
 import subprocess
 os.chmod(postrm, 0o755)
-subprocess.call(postrm, shell=True)
+subprocess.check_call(postrm, shell=True)
 oe.path.remove(postrm)
 
 oe.path.remove(manifest)
diff --git a/meta/lib/oeqa/utils/buildproject.py 
b/meta/lib/oeqa/utils/buildproject.py
index 487f08b..721f35d 100644
--- a/meta/lib/oeqa/utils/buildproject.py
+++ b/meta/lib/oeqa/utils/buildproject.py
@@ -52,4 +52,4 @@ class BuildProject(metaclass=ABCMeta):
 
 def clean(self):
 self._run('rm -rf %s' % self.targetdir)
-subprocess.call('rm -f %s' % self.localarchive, shell=True)
+subprocess.check_call('rm -f %s' % self.localarchive, shell=True)
diff --git a/meta/lib/oeqa/utils/targetbuild.py 
b/meta/lib/oeqa/utils/targetbuild.py
index 9249fa2..1202d57 100644
--- a/meta/lib/oeqa/utils/targetbuild.py
+++ b/meta/lib/oeqa/utils/targetbuild.py
@@ -69,7 +69,7 @@ class BuildProject(metaclass=ABCMeta):
 
 def clean(self):
 self._run('rm -rf %s' % self.targetdir)
-subprocess.call('rm -f %s' % self.localarchive, shell=True)
+subprocess.check_call('rm -f %s' % self.localarchive, shell=True)
 pass
 
 class TargetBuildProject(BuildProject):
@@ -136,4 +136,4 @@ class