[sos-devel] [PATCH v2] sosreport: Capture IBM Power RAID storage adapter configuration information

[Bryn M. Reeves]

On Fri, Sep 19, 2014 at 11:23:25AM +0530, Aruna Balakrishnaiah wrote:
>  sos/plugins/powerpc_iprconfig.py |  116 ++++++++++++++++++++++++++++++++++++++

No need for the 'powerpc_' prefix; iprconfig is sufficiently unique on
its own and plugin names with underscores are cumbersome to enter at the
command line (we put up with it for things like openstack_* but it's
something to avoid where possible).

> +    plugin_name = 'powerpc_iprconfig'
> +
> +    def check_enabled(self):
> +        return (self.policy().get_arch() == "ppc64")

How is the iprconfig command normally provided? If it's an RPM or Deb
package then it would be better to use a package check here as it
avoids the need to check /proc/cpuinfo. Alternately if it's not usual
to find it packaged you can just test is_executable("iprconfig").

> +    def setup(self):
> +        try:
> +            with open('/proc/cpuinfo', 'r') as fp:
> +                contents = fp.read()
> +                ispSeries = "pSeries" in contents
> +                isPowerNV = "PowerNV" in contents
> +        except:
> +            ispSeries = False
> +            isPowerNV = False
> +
> +        if ispSeries or isPowerNV:
> +            self.add_cmd_outputs([
> +                "iprconfig -c show-config",
> +                "iprconfig -c show-alt-config",
> +                "iprconfig -c show-arrays",
> +                "iprconfig -c show-jbod-disks",
> +                "iprconfig -c show-ioas",
> +            ])
> +
> +            iprconfig_result = self.call_ext_prog("iprconfig -c show-ioas")
> +            if not (iprconfig_result['status'] == 0):
> +                 return
> +
> +            iprconfig_output = iprconfig_result['output']
> +            devices = []
> +            if iprconfig_output:
> +                for line in iprconfig_output.splitlines():
> +                     temp = line.split(' ')
> +                     p = re.compile('sg')

p is a loop invariant; don't compile it on every iteration (python's
magix regex caching will catch this but it's still bad practice).

> +                     # temp[0] holds the device name
> +                     if p.search(temp[0]):
> +                         devices.append(temp[0])
> +
> +            for device in devices:
> +                self.add_cmd_output("iprconfig -c show-details %s" % (device,))
> +
> +            # Look for IBM Power RAID enclosures (iprconfig lists them)
> +            ipr_result = self.call_ext_prog("iprconfig -c show-config")
> +            if not (ipr_result['status'] == 0):

Why a new variable for the 2nd iprconfig call? Just re-use
iprconfig_result (or call them both ipr_result; it's more readable).

> +            ipr_output = ipr_result['output']
> +            if ipr_output:

Ditto for ipr_output.

> +               for line in ipr_output.splitlines():

If you assign ipr_output to another name (say 'show_config') then you
can again re-use the ipr_result name here and avoid introducing a 3rd
temporary variable for holding iprconfig output.

> +                  if "Enclosure" in line:
> +                     temp = re.split('\s+', line)
> +                     # temp[1] holds the PCI/SCSI location
> +                     pci, scsi = temp[1].split('/')
> +                     altconfig_result = self.call_ext_prog("iprconfig -c "
> +                                                           "show-alt-config")

Why is this inside the loop? Nothing in this command changes so we're
just running "iprconfig -c show-alt-config" every time round the loop.

It seems you just want one set of output for "show-config" (to find the
enclosure devices) and one set of "show-alt-config" to match them up to
sg device names.

Collect both then iterate over the first checking to see if its address
exists in the second. Bonus points for using something efficient to do
the lookups (although for the number of iterations we're talking about
here it doesn't really matter that much).

> +                     altconfig_output = altconfig_result['output']
> +                     if altconfig_output:
> +                        for line in altconfig_output.splitlines():
> +                            if scsi in line:
> +                               temp = line.split(' ')
> +                               # temp[0] holds device name
> +                               self.add_cmd_output("iprconfig -c "
> +                                             "query-ses-mode %s" % (temp[0],))

Same thing: this doesn't need to be in a nested loop.

Regards,
Bryn.