--jordan hargrave
Dell Enterprise Linux Engineering
________________________________________
From: Zdenek Styblik [zdenek.styb...@gmail.com]
Sent: Monday, December 09, 2013 12:31 PM
To: Hargrave, Jordan
Cc: ipmitool-devel
Subject: Re: Code Review - ID: 86 - Add support for enable/disable PEF policy
entries
On Mon, Dec 9, 2013 at 7:24 PM, Zdenek Styblik <zdenek.styb...@gmail.com> wrote:
>> @@@oops. not sure how that got there. I did a cvs up of my source tree and
>> merged back in my changes in ipmi_pef.c and ipmitool.1 only.
>
> Alright.
>
>>
>> ~~~
>> +int ipmi_getsysinfo(struct ipmi_intf * intf, int param, int block, int set,
>> + int len, void *buffer);
>> +int ipmi_setsysinfo(struct ipmi_intf * intf, int len, void *buffer);
>> ~~~
> [...]
>> @@@See above. Those functions are already in the existing .h file
>
> Yes, they do. And they're prefixed as I've said. Therefore I'm a bit
> confused what were you actually doing there.
>
>>
>> Such things go into header file. Also, where is check whether
>> ATTRIBUTE_PACKING is supported/defined? What if it isn't???
>>
>> @@@ ATTRIBUTE_PACKING is used in many other places so I assume that is the
>> correct way.
>>
>
> Indeed it is, but it's conditional. Check other header files and
> copy-paste, because that's the correct way if you want attribute
> packing.
>
>> @@@ ccode is already checked in ipmi_pef_msg_exchange
>
> Ok.
>
>> @@@ should it? the other pef (ipmi_pef_list_policies) etc do not.
>
> Yes, it definitely should.
>
>>
>> ~~~
>> + if (ptbl) {
>> + free(ptbl);
>> + }
>> ~~~
>>
>> You've missed ``ptbl = NULL;'' there.
>>
>> @@@ do you mean if (ptbl != NULL)
>
> No, I meant:
>
> ~~~
> if (ptbl) {
> free(ptbl);
> ptbl = NULL;
> }
> ~~~
>
> [...]
>> ~~~
>> +static struct valstr endis[] = {
>> + { .str = "enable", .val = 0x01 },
>> + { .str = "disable", .val = 0x00 },
>> + { .val = -1 },
>> +};
>> +
>> ~~~
>>
>> No, use strcmp()/strncmp() or whatever.
>>
>> @@@ ok can do that. but str2val is also used with argv[] elsewhere in the
>> code.
>>
>
> Yes, it's and I don't want to see more of it. You don't have to repeat
> every stupid/silly thing somebody else did in the code ;)
>
> [...]
>> A good one. :)
>>
>> @@@ str2int is used elsewhere in the code as well
>
> Ok, I'll be more specific. You don't bother with checking return code.
>
I'm also sure there is only a limited amount of PEF policies or
whatever. This should be checked as well. Therefore no ID < 0 and ID >
MAX_PEF_POLICY_ID either.
Z.
@@@ On our systems it is currently 8 IIRC. Not sure of a hard limit.
> Z.
>
>>
>> Also, please, read
>> http://sourceforge.net/p/ipmitool/wiki/coding_standards/ and change
>> code formatting in your patch.
>>
>> Best regards,
>> Z.
>>
>> --
>> Zdenek Styblik
>> email: zdenek.styb...@gmail.com
>> jabber: zdenek.styb...@gmail.com
------------------------------------------------------------------------------
Sponsored by Intel(R) XDK
Develop, test and display web and hybrid apps with a single code base.
Download it for free now!
http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
