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

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

Hello, Zdenek,

I addressed your comments. Please, review once more.

Regards,
Dmitry

03.03.2014 23:12, Zdenek Styblik пишет:
> On Tue, Feb 18, 2014 at 11:07 AM, Dmitry Bazhenov
> <di...@pi...> wrote:
> [...]
>>> * 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.
>>
> Good.
>
>>> * 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" is the default storage class for functions in C. In this example
>> the extern linkage is just stated explicitly.
>>
> Ok.
>
>>> * 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?
>>
> The argument is - it's just over-complicated. Please, return as early
> possible and don't do if() for if().
>
> ~~~
> if (rsp == NULL) {
>    /* error */
>    return (-1);
> }
> if (rsp->ccode > 0) {
>    /* different error */
>    return rsp->ccode;
> }
> ~~~
>
>>> ~~~
>>> +        uploadCmd.req =
>>> malloc(ipmi_intf_get_max_request_data_size(intf));
>>> ~~~
> It should be split into something like:
> ~~~
> int allocate_i = ipmi_intf_get_max_request_data_size(intf);
> uploadCmd.req = malloc(allocate_i);
> ~~~
>
> I mean, don't use return value directly as an argument for function.
>
>>> * 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.
> Ok.
>
>>> ~~~
>>> +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.
>>
> Yeah, we had several discussions with friend of mine. It turned out we
> should reconsider our view on 'static'. So, agreed. My bad.
>
>>> * 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.
> Haha, not what I meant, but great :)
>
>>> * 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.
> Good.
>
> And I'm sorry for delay. It took some time to discuss 'static' thing,
> but the fact is, I just kept this reply on back-burner. If this ever
> happens again,  just kick me to see whether I'm alive.
>
> Thanks,
> Z.