On Wed, Mar 19, 2014 at 8:33 AM, Dmitry Bazhenov <dim...@pigeonpoint.com> 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
>>>>> <dim...@pigeonpoint.com> 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.
>>>>
>>>>
>
------------------------------------------------------------------------------
Learn Graph Databases - Download FREE O'Reilly Book
"Graph Databases" is the definitive new guide to graph databases and their
applications. Written by three acclaimed leaders in the field,
this first edition is now available. Download your free book today!
http://p.sf.net/sfu/13534_NeoTech
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
