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

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

Hello, Zdenek,

Did you have a chance to make a review of the updated patch?

Regards,
Dmitry

03.03.2014 23:29, Dmitry Bazhenov пишет:
> 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.
>