kontron / python-ipmi

A pure python IPMI library
GNU Lesser General Public License v2.1
187 stars 74 forks source link

Provide a way of logging extra bytes in message #103

Closed EmilioPeJu closed 2 years ago

EmilioPeJu commented 2 years ago

It has been encountered some cases in which a reply is padded with extra zero bytes. The reply content is still valid but the extra bytes can be ignored. Logging that condition (instead of raising an exception) will allow returning the reply.

EmilioPeJu commented 2 years ago

Actually, I found that the padding was added by ipmitool, when sending a command to delete a SEL entry, the answer contained an extra zero byte. The ipmitool version used is 1.8.18, not sure if a newer version will not have that issue.

hthiery commented 2 years ago

Are you sure that the extra byte is added by ipmitool? I suspect that the addressed ipmi device sends this additional byte in the response.

EmilioPeJu commented 2 years ago

I reported the issue to the device vendor and they replied saying that they did some test and confirmed that the IPMI reply didn't have an extra zero and therefore it would be ipmitools' doing. I have done a capture using wireshark and the reply is as follows: 81 2c 53 20 28 46 00 00 00 00 72 It looks like it's actually coming from the device. Anyways, I think python-ipmi should have a way of working around these cases.

EmilioPeJu commented 2 years ago

Here is an alternative solution:

diff --git a/pyipmi/msgs/message.py b/pyipmi/msgs/message.py
index c399778..a0bc398 100644
--- a/pyipmi/msgs/message.py
+++ b/pyipmi/msgs/message.py
@@ -385,7 +385,11 @@ class Message(object):
                 break

         if (cc is None or cc == 0) and len(data) > 0:
-            raise DecodingError('Data has extra bytes')
+            self.on_extra_data()
+
+    def on_extra_data(self):
+        """What to do when not expected extra data is found in a message"""
+        raise DecodingError('Data has extra bytes')

     def _is_request(self):
         return self.__netfn__ & 1 == 0

That way, I can monkey patch on_extra_data to not raise an exception (in the code using python-ipmi)

hthiery commented 2 years ago

What about introducing a new Exception type (e.g. MessageExtraDataError)? With that you could catch that and do what you want to do without monkey patching.

EmilioPeJu commented 2 years ago

There are 3 problems with raising an exception:

I need get_and_clear_sel_entry() to work normally in spite of having an extra zero byte in the reply for delete_sel_entry()

hthiery commented 2 years ago

ah I see. sou let's do it the way you proposed.

EmilioPeJu commented 2 years ago

I guess I'm the only one having this issue (and it's not worth making a change addressing it), I can still monkey patch the _decode() function as a workaround

hthiery commented 2 years ago

sorry ... but i had accidentally closed #107. actually i wanted to merge this one.