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

[Sandro Bonazzola]

Il 04/02/2014 16:05, Bryn M. Reeves ha scritto:
> 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.).

done

> 
>> +            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.

done this too, loosing stderr output in logs.


> 
> Regards,
> Bryn.
> 


-- 
Sandro Bonazzola
Better technology. Faster innovation. Powered by community collaboration.
See how it works at redhat.com