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

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

On Thu, Mar 13, 2014 at 5:29 AM, Dmitry Bazhenov <di...@pi...> wrote:
> Hello, Zdenek,
>
> Did you have a chance to make a review of the updated patch?
>
> Regards,
> Dmitry
>

Hi Dmitry,

yes, although not as deeply as I'd like to.

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?

~~~
+    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;
+    }
~~~

This seems to have been missed. :)

~~~
     if (my_long_packet_set == 1) {
+        if (ipmi_oem_active(ipmi_main_intf, "kontron"))
+        {
         /* Restore defaults */
         ipmi_kontronoem_set_large_buffer( ipmi_main_intf, 0 );
+        }
~~~

This seems to be have strange indentation.

Other than these and code formatting, I guess it's ok.

Have a nice weekend,
Z.


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