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

[Zdenek S. <zde...@gm...>]

On Wed, Mar 19, 2014 at 8:33 AM, Dmitry Bazhenov <di...@pi...> wrote:
[...]
>> I got a bit "scared" by solution applied to
>> ipmi_intf_get_max_request_data_size() and
>> ipmi_intf_get_max_response_data_size(). But then I've tried to compile
>> just this one function with all kinds of switches and compiler didn't
>> comply, so I guess it's ok.
>> I wonder, shouldn't be the same logic applied to
>> ipmi_lanp_set_max_rq_data_size() and ipmi_lanp_set_max_rp_data_size()
>> as well?
>
> [DB] Calculations in the ipmi_intf_get_max_request_data_size() are required
> for the case if the target IPMC device is accessed via IPMI bridging. Since
> we can not deduce the target channel maximum message size, we use the
> minimum required size. These calculations are not needed for direct IPMC
> device access.
> [DB] Set max size functions are required if maximum message size over the
> chosen interface must be somehow modified from the value received from the
> interface properties. This is the case for the encrypted RMCP+ payload where
> maximum message size must be reduced by the confidentiality header/trailer
> sizes. Other interface types do not even implement these callbacks.
>

What I meant is whether under/over-flow shouldn't be checked in those
functions as well.

[...]

> [DB] My late thanks).
> [DB] If there will be no additional review comments, could you explicitly
> confirm acceptance of the patch? If such, I will re-base my repository in
> order to prepare subsequent feature patches.

I pretty much guess there won't be any more comments on my side.

Take care,
Z.


> [DB] No more comments below. 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.
>>>>
>>>>
>