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

[Dmitry B. <di...@pi...>]

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
> <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, ...
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.