ok uploaded the latest patch.
--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.
> 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
Index: doc/ipmitool.1
===================================================================
RCS file: /cvsroot/ipmitool/ipmitool/doc/ipmitool.1,v
retrieving revision 1.56
diff -u -p -b -r1.56 ipmitool.1
--- doc/ipmitool.1 26 Nov 2013 04:25:46 -0000 1.56
+++ doc/ipmitool.1 11 Dec 2013 18:55:17 -0000
@@ -2375,6 +2375,13 @@ This command lists the PEF policy table
entry describes an alert destination. A policy set is a
collection of table entries. PEF alert actions reference policy sets.
.TP
+\fIsetpolicy\fP <\fBid\fP> <\fBenable\fP|\fBdisable\fP>
+.br
+
+This command enables or disables a PEF policy entry. The
+policy ID is the first column from the output of command
+\fBipmitool pef policy\fP
+.TP
\fIlist\fP
.br
Index: include/ipmitool/ipmi_pef.h
===================================================================
RCS file: /cvsroot/ipmitool/ipmitool/include/ipmitool/ipmi_pef.h,v
retrieving revision 1.7
diff -u -p -b -r1.7 ipmi_pef.h
--- include/ipmitool/ipmi_pef.h 9 Jun 2009 15:38:09 -0000 1.7
+++ include/ipmitool/ipmi_pef.h 11 Dec 2013 18:55:17 -0000
@@ -555,6 +555,18 @@ struct pef_cfgparm_policy_table_entry {
#ifdef HAVE_PRAGMA_PACK
#pragma pack(1)
#endif
+struct pef_cfgparm_set_policy_table_entry {
+ uint8_t id;
+ uint8_t data1;
+ struct pef_policy_entry entry;
+} ATTRIBUTE_PACKING;
+#ifdef HAVE_PRAGMA_PACK
+#pragma pack(0)
+#endif
+
+#ifdef HAVE_PRAGMA_PACK
+#pragma pack(1)
+#endif
struct pef_cfgparm_system_guid {
#define PEF_SYSTEM_GUID_USED_IN_PET 0x01
uint8_t data1;
@@ -936,6 +948,7 @@ BIT_DESC_MAP_LIST,
#endif
#define IPMI_CMD_GET_PEF_CAPABILITIES 0x10
+#define IPMI_CMD_SET_PEF_CONFIG_PARMS 0x12
#define IPMI_CMD_GET_PEF_CONFIG_PARMS 0x13
#define IPMI_CMD_GET_LAST_PROCESSED_EVT_ID 0x15
#define IPMI_CMD_GET_SYSTEM_GUID 0x37
Index: lib/ipmi_pef.c
===================================================================
RCS file: /cvsroot/ipmitool/ipmitool/lib/ipmi_pef.c,v
retrieving revision 1.22
diff -u -p -b -r1.22 ipmi_pef.c
--- lib/ipmi_pef.c 26 Nov 2013 04:25:46 -0000 1.22
+++ lib/ipmi_pef.c 11 Dec 2013 18:55:17 -0000
@@ -256,6 +256,64 @@ ipmi_pef_get_policy_table(struct ipmi_in
return(tbl_size);
}
+static int
+ipmi_pef_set_policy_table_entry(struct ipmi_intf * intf, int id, struct pef_cfgparm_policy_table_entry *entry)
+{
+ struct ipmi_rs *rsp;
+ struct ipmi_rq req;
+ struct pef_cfgparm_set_policy_table_entry psel;
+
+ memset(&req, 0, sizeof(req));
+ req.msg.netfn = IPMI_NETFN_SE;
+ req.msg.cmd = IPMI_CMD_SET_PEF_CONFIG_PARMS;
+ req.msg.data = &psel;
+ req.msg.data_len = sizeof(psel);
+
+ memset(&psel, 0, sizeof(psel));
+ psel.id = PEF_CFGPARM_ID_PEF_ALERT_POLICY_TABLE_ENTRY;
+ psel.data1 = id & PEF_POLICY_TABLE_ID_MASK;
+ memcpy(&psel.entry, &entry->entry, sizeof(entry->entry));
+
+ 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;
+}
+
+/* Enable or disable a specific PEF policy */
+static int
+ipmi_pef_policy_enable(struct ipmi_intf * intf, int id, int enable)
+{
+ struct pef_cfgparm_policy_table_entry * ptbl = NULL;
+ int tbl_size;
+
+ /* policies are indexed 1..N, table is 0..N-1 */
+ id--;
+
+ tbl_size = ipmi_pef_get_policy_table(intf, &ptbl);
+ if (!tbl_size) {
+ if (ptbl) {
+ free(ptbl);
+ ptbl = NULL;
+ }
+ return -1;
+ }
+ if (id < 0 || id >= tbl_size) {
+ lprintf(LOG_ERR, "Invalid policy index, valid range = (1..%d)",
+ tbl_size);
+ return -1;
+ }
+ if (enable)
+ ptbl[id].entry.policy |= PEF_POLICY_ENABLED;
+ else
+ ptbl[id].entry.policy &= ~PEF_POLICY_ENABLED;
+ ipmi_pef_set_policy_table_entry(intf, id+1, &ptbl[id]);
+
+ return 0;
+}
+
static void
ipmi_pef_print_lan_dest(struct ipmi_intf * intf, uint8_t ch, uint8_t dest)
{ /*
@@ -873,6 +931,26 @@ int ipmi_pef_main(struct ipmi_intf * int
ipmi_pef_get_status(intf);
else if (!strncmp(argv[0], "policy", 6))
ipmi_pef_list_policies(intf);
+ else if (!strncmp(argv[0], "setpolicy", 6)) {
+ int set, enable;
+
+ set = enable = 0xFFFF;
+ if (argc > 2) {
+ if (str2int(argv[1], &set) < 0)
+ set = 0xFFFF;
+ if (!strcmp(argv[2], "disable"))
+ enable = 0;
+ else if (!strcmp(argv[2], "enable"))
+ enable = 1;
+ }
+ if (set == 0xFFFF || enable == 0xFFFF) {
+ lprintf(LOG_NOTICE, "setpolicy <policy number> <enable|disable>");
+ rc = -1;
+ }
+ else {
+ ipmi_pef_policy_enable(intf, set, enable);
+ }
+ }
else if (!strncmp(argv[0], "list", 4))
ipmi_pef_list_entries(intf);
else {
@@ -882,7 +960,7 @@ int ipmi_pef_main(struct ipmi_intf * int
}
if (help)
- lprintf(LOG_NOTICE, "PEF commands: info status policy list");
+ lprintf(LOG_NOTICE, "PEF commands: info status policy setpolicy list");
else if (!verbose)
printf("\n");
------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT
organizations don't have a clear picture of how application performance
affects their revenue. With AppDynamics, you get 100% visibility into your
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk
_______________________________________________
Ipmitool-devel mailing list
Ipmitool-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
