On Thu, Mar 13, 2014 at 5:29 AM, Dmitry Bazhenov <dim...@pigeonpoint.com> 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
>>> <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
