Re: [PATCH] QemuRunner: avoid tainting os.environ

[Richard Purdie <richard.purdie@linuxfoundation.org>]

On Wed, 2017-05-03 at 12:56 +0200, Patrick Ohly wrote:
> That a utility function permanently changes the process environment
> is
> bad style and leads to subtle, hard to debug problems.
> 
> For example, we had one oe-selftest which used runqemu() with an
> override for DEPLOY_DIR_IMAGE. Another test then just called runCmd()
> and ended up passing the wrong DEPLOY_DIR_IMAGE set earlier in
> os.environ.
> 
> The approach used here is to extend the launch command such that
> 'env'
> sets the environment variables. The specific values here should be
> safe to use this way, even without extra quoting. This approach is
> simple and has the advantage that the existing log.info('launchcmd=')
> output includes the environment variables.
> 
> A more complex approach would be to pass a modified environment hash
> to the subprocess module.
> 
> [YOCTO #11443]
> 
> Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
> ---
>  meta/lib/oeqa/utils/qemurunner.py | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/meta/lib/oeqa/utils/qemurunner.py
> b/meta/lib/oeqa/utils/qemurunner.py
> index ba44b96..c543797 100644
> --- a/meta/lib/oeqa/utils/qemurunner.py
> +++ b/meta/lib/oeqa/utils/qemurunner.py
> @@ -98,11 +98,12 @@ class QemuRunner:
>                  raise SystemExit
>  
>      def start(self, qemuparams = None, get_ip = True,
> extra_bootparams = None, runqemuparams='', launch_cmd=None,
> discard_writes=True):
> +        env = []
>          if self.display:
> -            os.environ["DISPLAY"] = self.display
> +            env.append("DISPLAY=" + self.display)
>              # Set this flag so that Qemu doesn't do any grabs as SDL
> grabs
>              # interact badly with screensavers.
> -            os.environ["QEMU_DONT_GRAB"] = "1"
> +            env.append("QEMU_DONT_GRAB=1")
>          if not os.path.exists(self.rootfs):
>              logger.error("Invalid rootfs %s" % self.rootfs)
>              return False
> @@ -110,12 +111,12 @@ class QemuRunner:
>              logger.error("Invalid TMPDIR path %s" % self.tmpdir)
>              return False
>          else:
> -            os.environ["OE_TMPDIR"] = self.tmpdir
> +            env.append("OE_TMPDIR=" + self.tmpdir)
>          if not os.path.exists(self.deploy_dir_image):
>              logger.error("Invalid DEPLOY_DIR_IMAGE path %s" %
> self.deploy_dir_image)
>              return False
>          else:
> -            os.environ["DEPLOY_DIR_IMAGE"] = self.deploy_dir_image
> +            env.append("DEPLOY_DIR_IMAGE=" + self.deploy_dir_image)
>  
>          if not launch_cmd:
>              launch_cmd = 'runqemu %s %s ' % ('snapshot' if
> discard_writes else '', runqemuparams)
> @@ -128,6 +129,9 @@ class QemuRunner:
>                  launch_cmd += ' nographic'
>              launch_cmd += ' %s %s' % (self.machine, self.rootfs)
>  
> +        if env:
> +            launch_cmd = 'env %s %s' % (' '.join(env), launch_cmd)
> +
>          return self.launch(launch_cmd, qemuparams=qemuparams,
> get_ip=get_ip, extra_bootparams=extra_bootparams)
>  
>      def launch(self, launch_cmd, get_ip = True, qemuparams = None,
> extra_bootparams = None):

Why not pass in an env to the subprocess call in launch()?

Cheers,

Richard