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

[Sandro Bonazzola]

Il 04/02/2014 17:32, Sandro Bonazzola ha scritto:
> 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.

Any comment?


> 
> 
>>
>> Regards,
>> Bryn.
>>
> 
> 


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