Re: [Ipmitool-devel] [patches:#89] [BMR #81324] HPM.2 long message support

[Zdenek S. <zde...@gm...>]

On Tue, Feb 11, 2014 at 9:00 AM, Zdenek Styblik
<zde...@gm...> wrote:
[...]

Dmitry,

my comments from pass#1 reading are as follows.

* Formatting - no worries about this one at this time ;)
* name 'ipmi_hpm2' - since it's not a CLI module, I think it would be
more fitting to name it 'hpm2' or whatever, but not 'ipmi_hpm2'. As
far as I've noticed, CLI modules are prefixed by 'ipmi_' whether
"support" modules aren't. I think it would be wise to keep this
convention. Or are there any plans 'ipmi_hpm2' becoming CLI module as
well? Although, it can be renamed any time in the future. For, as long
as it isn't CLI module, ...
* include/ipmitool/ipmi_hpm2.h

~~~
+extern int hpm2_get_capabilities(struct ipmi_intf * intf,
+        struct hpm2_lan_attach_capabilities * caps);
+extern int hpm2_get_lan_channel_capabilities(struct ipmi_intf * intf,
+        uint8_t hpm2_lan_params_start,
+        struct hpm2_lan_channel_capabilities * caps);
+extern int hpm2_detect_max_payload_size(struct ipmi_intf * intf);
~~~

Extern? I mean, extern? I haven't tried to compile without it, but it
just seems odd to use 'extern' here.

* lib/ipmi_hpm2.c

~~~
+    if (rsp) {
+        if (rsp->ccode == 0xC1) {
+            lprintf(LOG_DEBUG,"IPM Controller is not HPM.2 compatible");
+            return rsp->ccode;
+        } else if (rsp->ccode) {
+            lprintf(LOG_NOTICE,"Get HPM.x Capabilities request failed,"
+                    " compcode = %x\n", rsp->ccode);
+            return rsp->ccode;
+        }
+    } else {
+        lprintf(LOG_NOTICE,"Error sending request\n");
+        return -1;
+    }
~~~
->
~~~
if (rsp == NULL) {
  lprintf(LOG_NOTICE,"Error sending request\n");
  return -1;
}
+        if (rsp->ccode == 0xC1) {
+            lprintf(LOG_DEBUG,"IPM Controller is not HPM.2 compatible");
+            return rsp->ccode;
+        } else if (rsp->ccode) {
+            lprintf(LOG_NOTICE,"Get HPM.x Capabilities request failed,"
+                    " compcode = %x\n", rsp->ccode);
+            return rsp->ccode;
+        }
[ continue with the code in function
~~~

~~~
if (caps->lan_channel_mask) {
[...]
}
return 0;
~~~
->
~~~
if (!caps->lan_channel_mask) {
  return 0;
}
[ continue with code from if() block here ]
~~~

This repeats couple times through the file in question.

* lib/ipmi_hpmfwupg.c

~~~
+        uploadCmd.req = malloc(ipmi_intf_get_max_request_data_size(intf));
~~~

This doesn't look like a good idea. You just shouldn't do things like this.

* src/plugins/lan/lan.c and src/plugins/lanplus/lanplus.c
~~~
+    /* automatically detect interface request and response sizes */
+    hpm2_detect_max_payload_size(intf);
+
~~~

Aren't you interested in errors?

~~~
+static void ipmi_lan_set_max_rq_data_size(struct ipmi_intf * intf,
uint16_t size);
+static void ipmi_lan_set_max_rp_data_size(struct ipmi_intf * intf,
uint16_t size);
~~~

I don't believe 'static' is required, is it?

* src/plugins/open/open.c
~~~
+#define IPMI_OPENIPMI_DEFAULT_PAYLOAD_SIZE 37
~~~

I hope this doesn't break anything :)

* Now, my biggest concern - data size calculation. Have you considered
the fact given/supplied size could be so small you will underflow
uint? I believe this should be handled. You should check whether
addition or subtraction doesn't cause overflow/underflow. At least in
my opinion.

So much for the first quick reading through.

Z.