On Tue, Feb 11, 2014 at 9:00 AM, Zdenek Styblik
<zdenek.styb...@gmail.com> 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.
------------------------------------------------------------------------------
Android apps run on BlackBerry 10
Introducing the new BlackBerry 10.2.1 Runtime for Android apps.
Now with support for Jelly Bean, Bluetooth, Mapview and more.
Get your Android app in front of a whole new audience. Start now.
http://pubads.g.doubleclick.net/gampad/clk?id=124407151&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
