--jordan hargrave
Dell Enterprise Linux Engineering
________________________________________
From: Zdenek Styblik [[email protected]]
Sent: Monday, December 09, 2013 12:24 PM
To: Hargrave, Jordan
Cc: ipmitool-devel
Subject: Re: Code Review - ID: 86 - Add support for enable/disable PEF policy
entries
> @@@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.
@@@@ Ok have made the change into ipmi_pef.h
> @@@ 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.
@@@@ Ok made that change too
>
> ~~~
> + 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;
}
~~~
@@@@ Should it? ptbl isn't used elsewhere. That's more a personal perference
style, but I can add it.
[...]
> ~~~
> +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 ;)
@@@@ I like the use though, makes the code cleaner IMHO. But I can add the
strcmps.
[...]
> 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.
Z.
@@@@ ah. could do. but it's also checked in setpolicy.
>
> Also, please, read
> http://sourceforge.net/p/ipmitool/wiki/coding_standards/ and change
> code formatting in your patch.
>
> Best regards,
> Z.
>
> --
> Zdenek Styblik
> email: [email protected]
> jabber: [email protected]
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
