Hello, Zdenek,
Please, see my comments below.
14.02.2014 1:14, Zdenek Styblik пишет:
> 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, ...
Currently, only utility functions. In the future there will be some
commands. I'll rename the files.
> * 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.
"Extern" is the default storage class for functions in C. In this
example the extern linkage is just stated explicitly.
>
> * 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.
I do not understand the argument. Please, elaborate more on this. The
code does what is required. What's wrong?
>
> * 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?
No at all. Boards may support HPM.2 and may not. If they are, the
function will set appropriate payload sizes.
>
> ~~~
> +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?
This functions are for the internal module use. Using static storage
type is logical here.
>
> * src/plugins/open/open.c
> ~~~
> +#define IPMI_OPENIPMI_DEFAULT_PAYLOAD_SIZE 37
> ~~~
>
> I hope this doesn't break anything :)
This surely does not break for requests, but can break for responses.
I'll fix it.
>
> * 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.
I'll add necessary checks where it is appropriate.
No more comments below.
Regards,
Dmitry
>
> So much for the first quick reading through.
>
> Z.
------------------------------------------------------------------------------
Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.
http://pubads.g.doubleclick.net/gampad/clk?id=121054471&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
