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.
------------------------------------------------------------------------------
Subversion Kills Productivity. Get off Subversion & Make the Move to Perforce.
With Perforce, you get hassle-free workflows. Merge that actually works.
Faster operations. Version large binaries. Built-in WAN optimization and the
freedom to use Git, Perforce or both. Make the move to Perforce.
http://pubads.g.doubleclick.net/gampad/clk?id=122218951&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
