Make ipmitool interface robust against prepended error messages

[patrislav1]

I got a use case where ipmitool doesn't fail, but prepends the raw data with a error message line, probably due to a MicroTCA MCH failing a capabilities request. The requested IPMI access as such does succeed and data is printed out. example:

$ ipmitool -I lan -H 192.168.1.209 -A NONE -T 0x82 -B 0 -t 0x7a -b 7 raw 0x30 0xf0 0x00
Get HPM.x Capabilities request failed, compcode = c9
 4d 4d 43 20 43 6f 6e 73 6f 6c 65

pyipmi.interfaces.ipmitool.send_and_receive_raw() tries to parse the error message as hex dump (see below)

This patch throws out all error message lines (those that contain the word 'failed') to make sure ipmitool's hex output is still parsed.

Running ipmitool "ipmitool -I lan -H 192.168.1.209 -p 623 -U "" -P "" -T 0x82 -B 0 -t 0x7a -b 7 -l 0 raw 0x30 0xf0 0x00 2>&1"
return with rc=0, output was:
b'Get HPM.x Capabilities request failed, compcode = c9\n 4d 4d 43 20 43 6f 6e 73 6f 6c 65\n'
Traceback (most recent call last):
  File "~/src/python-ipmi/pyipmi/__init__.py", line 220, in raw_command
    raw_bytes)
  File "~/src/python-ipmi/pyipmi/interfaces/ipmitool.py", line 133, in send_and_receive_raw
    data.append(int(value, 16))
ValueError: invalid literal for int() with base 16: 'Get'

[mwalle]

Hi,

would be nice to have some tests with your specific output

[hthiery]

I would suggest to move the parsing stuff to a function like:

--- a/pyipmi/interfaces/ipmitool.py
+++ b/pyipmi/interfaces/ipmitool.py
@@ -90,6 +90,13 @@ class Ipmitool(object):

         return accessible

+    def _parse_output(self, output):
+        # check for errors
+        completion_code = self.re_completion_code.match(output)
+        timeout = self.re_timeout.match(output)
+
+        return (completion_code, timeout)
+
     def send_and_receive_raw(self, target, lun, netfn, raw_bytes):

         if self._interface_type in ['lan', 'lanplus']:
@@ -104,15 +111,13 @@ class Ipmitool(object):
                                self._interface_type)

         output, rc = self._run_ipmitool(cmd)
+        (completion_code, timeout) = self._parse_output(output)

-        # check for errors
-        match_completion_code = self.re_completion_code.match(output)
-        match_timeout = self.re_timeout.match(output)
         data = array('B')
-        if match_completion_code:
-            cc = int(match_completion_code.group(1), 16)
+        if completion_code:
+            cc = int(completion_code.group(1), 16)
             data.append(cc)
-        elif match_timeout:
+        elif timeout:
             raise IpmiTimeoutError()
         else:
             if rc != 0:

and write some tests for that ... what do you think?

[hthiery]

e some tests for that ... what do you think?

I would suggest to move the parsing stuff to a function like:

--- a/pyipmi/interfaces/ipmitool.py
+++ b/pyipmi/interfaces/ipmitool.py
@@ -90,6 +90,13 @@ class Ipmitool(object):

         return accessible

+    def _parse_output(self, output):
+        # check for errors
+        completion_code = self.re_completion_code.match(output)
+        timeout = self.re_timeout.match(output)
+
+        return (completion_code, timeout)
+
     def send_and_receive_raw(self, target, lun, netfn, raw_bytes):

         if self._interface_type in ['lan', 'lanplus']:
@@ -104,15 +111,13 @@ class Ipmitool(object):
                                self._interface_type)

         output, rc = self._run_ipmitool(cmd)
+        (completion_code, timeout) = self._parse_output(output)

-        # check for errors
-        match_completion_code = self.re_completion_code.match(output)
-        match_timeout = self.re_timeout.match(output)
         data = array('B')
-        if match_completion_code:
-            cc = int(match_completion_code.group(1), 16)
+        if completion_code:
+            cc = int(completion_code.group(1), 16)
             data.append(cc)
-        elif match_timeout:
+        elif timeout:
             raise IpmiTimeoutError()
         else:
             if rc != 0:

and write some tests for that ... what do you think?

this is not enough since the valid data output parsing also includes the return data ... so my suggestion should not be the final solution ;-/

[patrislav1]

I would suggest to move the parsing stuff to a function like:

Good point - all the parsing should be deferred to after ipmitool's error messages have been thrown out.

[patrislav1]

Added separate parsing function and tests as suggested.