[sos-devel] [PATCH] ovirt-engine: new plugin for oVirt project

[Bryn M. Reeves]

On 02/04/2014 02:56 PM, Sandro Bonazzola wrote:
> +    def setup(self):
> +        if self.get_option('jbosstrace'):
> +            proc = subprocess.Popen(
> +                args=[
> +                    '/usr/bin/pgrep',
> +                    '-f',
> +                    'jboss',
> +                ],
> +                stdout=subprocess.PIPE,
> +            )
> +            output, err = proc.communicate()
> +            returncode = proc.returncode

Any reason to open code this rather than use an existing interface e.g.
call_ext_prog()? This returns a tuple (status, output, runtime) which
should give you everything you need.

We try to avoid plugins using things like Popen directly as it means we
have to make sure they all handle things like the environment and file
descriptor inheritance properly (see bz1051009 for e.g.).

> +            jboss_pids = set()
> +            if returncode == 0:
> +                jboss_pids = set([int(x) for x in output.splitlines()])
> +                proc = subprocess.Popen(
> +                    args=[
> +                        '/usr/bin/pgrep',
> +                        '-f',
> +                        'ovirt-engine',
> +                    ],
> +                    stdout=subprocess.PIPE,
> +                )
> +                engine_output, err = proc.communicate()

Ditto - no need for handrolled Popen here.

Other than that the plugin looks pretty good - I can change these to use
call_ext_prog() if you like but it's probably easier for you to test the
changes directly than me.

Regards,
Bryn.