Ok thanks for the update.. see below. @@@
--jordan hargrave
Dell Enterprise Linux Engineering
________________________________________
From: Zdenek Styblik [zdenek.styb...@gmail.com]
Sent: Monday, December 09, 2013 2:51 AM
To: Hargrave, Jordan
Cc: ipmitool-devel
Subject: Code Review - ID: 86 - Add support for enable/disable PEF policy
entries
Hello Jordan,
since SF.net kinda sucks, let's do code review over e-mail.
~~~
+<<<<<<< ipmi_mc.h
+/* IPMI 2.0 command for system information*/
[...]
+=======
[...]
+>>>>>>> 1.12
~~~
Errr ... what???
@@@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.
~~~
+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);
~~~
If you want to make these available outside of 'lib/ipmi_mc.c', then
names of these functions must be 'ipmi_mc_...'. Therefore, two
steps/tickets are required. One is to change names of these functions
and second is the one under discussion.
@@@See above. Those functions are already in the existing .h file
~~~
+#define IPMI_CMD_SET_PEF_CONFIG_PARMS 0x12
+struct pef_cfgparm_set_policy_table_entry
+{
+ uint8_t id;
+ uint8_t sel;
+ struct pef_policy_entry entry;
+} ATTRIBUTE_PACKING;
~~~
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.
~~~
+ rsp = ipmi_pef_msg_exchange(intf, &req, "Set Alert policy table entry");
+ if (!rsp) {
+ lprintf(LOG_ERR, " **Error setting Alert policy table entry");
+ return -1;
+ }
+ return 0;
~~~
No check for ccode? Huh?
@@@ ccode is already checked in ipmi_pef_msg_exchange
~~~
+static void
+ipmi_pef_policy_enable(struct ipmi_intf * intf, int set, int enable)
+{
~~~
No, this function *returns* integer.
@@@ should it? the other pef (ipmi_pef_list_policies) etc do not.
~~~
+ if (ptbl) {
+ free(ptbl);
+ }
~~~
You've missed ``ptbl = NULL;'' there.
@@@ do you mean if (ptbl != NULL)
~~~
+ if (set > 0 && set <= tbl_size) {
[...]
+ } else {
+ lprintf(LOG_ERR, "Invalid policy index, valid range =
(1..%d)", tbl_size);
+ }
~~~
Invert ``if'' and return early.
@@@ ok, can do that.
~~~
+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.
~~~
+ else if (!strncmp(argv[0], "setpolicy", 6)) {
~~~
Now, I'm not exactly fond of adding 'setpolicy'. How about to give
'pef' a bit of thought and redesign 'pef' cli interface? I find it
rather, well, not good, to be honest. And this isn't making it any
better.
I can only wonder why there isn't something like 'pef list
<policy|entry>' instead of 'policy' which lists policies and 'list'
which list entries. I'm sorry, but it makes no sense.
~~~
+ if (argc > 2) {
+ str2int(argv[1], &set);
~~~
A good one. :)
@@@ str2int is used elsewhere in the code as well
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
