Discussion:
[Ipmitool-devel] [PATCH] New PEF policy framework
Jordan Hargrave
2015-08-17 20:46:33 UTC
Permalink
Some time ago I proposed a patch to enable enabling/disabling individual PEF policy entries.
Support for this is needed for one of our utilities.  A new rework of the whole PEF framework was
requested.  I'm attaching a patch that (partially) implements this new scheme, please review.

http://sourceforge.net/p/ipmitool/bugs/312/


diff --git a/include/ipmitool/ipmi_mc.h b/include/ipmitool/ipmi_mc.h
index a840f78..fbd2c97 100644
--- a/include/ipmitool/ipmi_mc.h
+++ b/include/ipmitool/ipmi_mc.h
@@ -165,6 +165,8 @@ struct ipm_get_watchdog_rsp {
 #define IPMI_SYSINFO_OS_NAME        0x04
 #define IPMI_SYSINFO_DELL_OS_VERSION    0xe4
 #define IPMI_SYSINFO_DELL_URL        0xde
+#define IPMI_SYSINFO_DELL_IPV6_COUNT    0xe6
+#define IPMI_SYSINFO_DELL_IPV6_DESTADDR 0xf0
 
 int ipmi_mc_getsysinfo(struct ipmi_intf * intf, int param, int block, int set,
             int len, void *buffer);
diff --git a/lib/ipmi_pef.c b/lib/ipmi_pef.c
index 1beebf0..fd52663 100644
--- a/lib/ipmi_pef.c
+++ b/lib/ipmi_pef.c
@@ -38,6 +38,7 @@
 #include <ipmitool/helper.h>
 #include <ipmitool/log.h>
 #include <ipmitool/ipmi.h>
+#include <ipmitool/ipmi_mc.h>
 #include <ipmitool/ipmi_intf.h>
 #include <ipmitool/ipmi_pef.h>
 
@@ -206,7 +207,7 @@ ipmi_pef_msg_exchange(struct ipmi_intf * intf, struct ipmi_rq * req, char * txt)
 
 static uint8_t
 ipmi_pef_get_policy_table(struct ipmi_intf * intf,
-                                    struct pef_cfgparm_policy_table_entry ** table)
+            struct pef_cfgparm_policy_table_entry ** table)
 {    /*
     // get the PEF policy table: allocate space, fillin, and return its size
     //  NB: the caller must free the returned area (when returned size> 0)
@@ -257,6 +258,53 @@ ipmi_pef_get_policy_table(struct ipmi_intf * intf,
 }
 
 static void
+ipmi_pef_print_oem_lan_dest(struct ipmi_intf *intf, uint8_t ch, uint8_t dest)
+{
+    int rc, i;
+    uint8_t data[32];
+    char address[128];
+    int len, set, rlen;
+
+    if (ipmi_get_oem(intf) != IPMI_OEM_DELL) {
+        return;
+    }
+    /* Get # of IPV6 trap destinations */
+    rc = ipmi_mc_getsysinfo(intf, IPMI_SYSINFO_DELL_IPV6_COUNT, 0x00, 0x00, 4, data);
+    if (rc != 0 || dest> data[0]) {
+        return;
+    }
+    ipmi_pef_print_str("Alert destination type", "xxx");
+    ipmi_pef_print_str("PET Community", "xxx");
+    ipmi_pef_print_dec("ACK timeout/retry (secs)", 0);
+    ipmi_pef_print_dec("Retries", 0);
+
+    /* Get IPv6 destination string (may be in multiple sets) */
+    memset(address, 0, sizeof(address));
+    memset(data, 0, sizeof(data));
+    rc = ipmi_mc_getsysinfo(intf, IPMI_SYSINFO_DELL_IPV6_DESTADDR, 0x00, dest, 19, data);
+    if (rc != 0) {
+        return;
+    }
+    /* Total length of IPv6 string */
+    len = data[4];
+    if ((rlen = len)> (IPMI_SYSINFO_SET0_SIZE-3)) {
+        /* First set has 11 bytes */
+        rlen = IPMI_SYSINFO_SET0_SIZE - 3;
+    }
+    memcpy(address, data+8, rlen);
+    for (set = 1; len> 11; set++) {
+        rc = ipmi_mc_getsysinfo(intf, IPMI_SYSINFO_DELL_IPV6_DESTADDR, set, dest, 19, data);
+        if ((rlen = len-11)>= IPMI_SYSINFO_SETN_SIZE - 2) {
+            /* Remaining sets have 14 bytes */
+            rlen = IPMI_SYSINFO_SETN_SIZE - 2;
+        }
+        memcpy(address+set*11, data+3, rlen);
+        len -= rlen+3;
+    }
+    ipmi_pef_print_str("IPv6 Address", address);
+}
+
+static void
 ipmi_pef_print_lan_dest(struct ipmi_intf * intf, uint8_t ch, uint8_t dest)
 {    /*
     // print LAN alert destination info
@@ -284,6 +332,10 @@ ipmi_pef_print_lan_dest(struct ipmi_intf * intf, uint8_t ch, uint8_t dest)
         return;
     }
     tbl_size = (rsp->data[1] & PEF_LAN_DEST_TABLE_SIZE_MASK);
+    if (dest> tbl_size) {
+        ipmi_pef_print_oem_lan_dest(intf, ch, dest - tbl_size);
+        return;
+    }
     //if (tbl_size == 0 || dest == 0)    /* LAN alerting not supported */
     //    return;
 
@@ -579,7 +631,7 @@ ipmi_pef_print_event_info(struct pef_cfgparm_filter_table_entry * pef, char * bu
 
 static void
 ipmi_pef_print_entry(struct ipmi_rs * rsp, uint8_t id,
-                            struct pef_cfgparm_filter_table_entry * pef)
+             struct pef_cfgparm_filter_table_entry * pef)
 {    /*
     // print a PEF table entry
     */
@@ -686,50 +738,49 @@ ipmi_pef_list_policies(struct ipmi_intf * intf)
     req.msg.data = &ch;
     req.msg.data_len = sizeof(ch);
     for (ptmp=ptbl, i=1; i<=tbl_size; i++, ptmp++) {
-        if ((ptmp->entry.policy & PEF_POLICY_ENABLED) == PEF_POLICY_ENABLED) {
-            if (i> 1)
-                printf("\n");
-            first_field = 1;
-            ipmi_pef_print_dec("Alert policy table entry",
-                        (ptmp->data1 & PEF_POLICY_TABLE_ID_MASK));
-            ipmi_pef_print_dec("Policy set",
-                        (ptmp->entry.policy & PEF_POLICY_ID_MASK)>> PEF_POLICY_ID_SHIFT);
-            ipmi_pef_print_str("Policy entry rule",
-                        ipmi_pef_bit_desc(&pef_b2s_policies, (ptmp->entry.policy & PEF_POLICY_FLAGS_MASK)));
-
-            if (ptmp->entry.alert_string_key & PEF_POLICY_EVENT_SPECIFIC) {
-                ipmi_pef_print_str("Event-specific", "true");
-//                continue;
-            }           
-            wrk = ptmp->entry.chan_dest;
-
-            /* channel/description */
-            ch = (wrk & PEF_POLICY_CHANNEL_MASK)>> PEF_POLICY_CHANNEL_SHIFT;
-            rsp = ipmi_pef_msg_exchange(intf, &req, "Channel info");
-            if (!rsp || rsp->data[0] != ch) {
-                lprintf(LOG_ERR, " **Error retrieving %s",
-                    "Channel info");
-                continue;
-            }
-            medium = rsp->data[1];
-            ipmi_pef_print_dec("Channel number", ch);
-            ipmi_pef_print_str("Channel medium",
-                        ipmi_pef_bit_desc(&pef_b2s_ch_medium, medium));
-
-            /* destination/description */
-            wrk &= PEF_POLICY_DESTINATION_MASK;
-            switch (medium) {
-                case PEF_CH_MEDIUM_TYPE_LAN:
-                    ipmi_pef_print_lan_dest(intf, ch, wrk);
-                    break;
-                case PEF_CH_MEDIUM_TYPE_SERIAL:
-                    ipmi_pef_print_serial_dest(intf, ch, wrk);
-                    break;
-                default:
-                    ipmi_pef_print_dest(intf, ch, wrk);
-                    break;
-            }
+        first_field = 1;
+        ipmi_pef_print_dec("Alert policy table entry",
+                   (ptmp->data1 & PEF_POLICY_TABLE_ID_MASK));
+        ipmi_pef_print_dec("Policy set",
+                   (ptmp->entry.policy & PEF_POLICY_ID_MASK)>> PEF_POLICY_ID_SHIFT);
+        ipmi_pef_print_str("State",
+                   ptmp->entry.policy & PEF_POLICY_ENABLED ? "enabled" : "disabled");
+        ipmi_pef_print_str("Policy entry rule",
+                   ipmi_pef_bit_desc(&pef_b2s_policies, (ptmp->entry.policy & PEF_POLICY_FLAGS_MASK)));
+       
+        if (ptmp->entry.alert_string_key & PEF_POLICY_EVENT_SPECIFIC) {
+            ipmi_pef_print_str("Event-specific", "true");
+            //                continue;
+        }           
+        wrk = ptmp->entry.chan_dest;
+       
+        /* channel/description */
+        ch = (wrk & PEF_POLICY_CHANNEL_MASK)>> PEF_POLICY_CHANNEL_SHIFT;
+        rsp = ipmi_pef_msg_exchange(intf, &req, "Channel info");
+        if (!rsp || rsp->data[0] != ch) {
+            lprintf(LOG_ERR, " **Error retrieving %s",
+                "Channel info");
+            continue;
+        }
+        medium = rsp->data[1];
+        ipmi_pef_print_dec("Channel number", ch);
+        ipmi_pef_print_str("Channel medium",
+                   ipmi_pef_bit_desc(&pef_b2s_ch_medium, medium));
+       
+        /* destination/description */
+        wrk &= PEF_POLICY_DESTINATION_MASK;
+        switch (medium) {
+        case PEF_CH_MEDIUM_TYPE_LAN:
+            ipmi_pef_print_lan_dest(intf, ch, wrk);
+            break;
+        case PEF_CH_MEDIUM_TYPE_SERIAL:
+            ipmi_pef_print_serial_dest(intf, ch, wrk);
+            break;
+        default:
+            ipmi_pef_print_dest(intf, ch, wrk);
+            break;
         }
+        printf("\n");
     }
     free(ptbl);
     ptbl = NULL;
@@ -858,33 +909,258 @@ ipmi_pef_get_info(struct ipmi_intf * intf)
     ipmi_pef_print_flags(&pef_b2s_actions, P_SUPP, actions);
 }
 
+int
+ipmi_pef_timer(struct ipmi_intf *intf, int argc, char **argv)
+{
+    lprintf(LOG_ERR, "Not implemented");
+    return (-1);
+}
+
+void
+ipmi_pef_policy_help(void)
+{
+    lprintf(LOG_NOTICE,
+        "usage: pef policy <command> [options");
+    lprintf(LOG_NOTICE,
+        "    list    List enabled PEF policies");
+}
+
+void
+ipmi_pef2_policy_help(void)
+{
+    lprintf(LOG_NOTICE, "usage: pef policy list");
+    lprintf(LOG_NOTICE, "       pef policy enable <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef policy disable <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef policy create <id = 1..n> <params>");
+    lprintf(LOG_NOTICE, "       pef policy delete <id = 1..n>");
+}
+
+#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;
+
+
+static int
+ipmi_pef_set_policy_table_entry(struct ipmi_intf * intf, int set, 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.sel = set & 0x3F;
+    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/Disable specific PEF policy */
+static int
+ipmi_pef2_policy_enable(struct ipmi_intf *intf, int enable, int argc, char **argv)
+{
+    struct pef_cfgparm_policy_table_entry * ptbl = NULL;
+    int tbl_size, id;
+
+    if (argc < 1) {
+        ipmi_pef2_policy_help();
+        return (-1);
+    }
+    tbl_size = ipmi_pef_get_policy_table(intf, &ptbl);
+    if (!tbl_size) {
+        if (ptbl != NULL) {
+            free(ptbl);
+        }
+        return (-1);
+    }
+
+    /* Get policy ID number */
+    id = strtol(argv[0], NULL, 0);
+    if (id>= 1 && id <= tbl_size) {
+        /* Check if already enabled or disabled */
+        if ((enable && !(ptbl[id-1].entry.policy & PEF_POLICY_ENABLED)) ||
+            (!enable && (ptbl[id-1].entry.policy & PEF_POLICY_ENABLED))) {
+            ptbl[id-1].entry.policy ^= PEF_POLICY_ENABLED;
+            return ipmi_pef_set_policy_table_entry(intf, id, &ptbl[id-1]);
+        }
+        else {
+            lprintf(LOG_ERR, "Policy %d already %s", id, enable ? "enabled" : "disabled");
+        }
+    } else {
+        lprintf(LOG_ERR, "Invalid policy index, valid range = (1..%d)", tbl_size);
+        return (-1);
+    }
+    lprintf(LOG_INFO, "done\n");
+    return (0);
+}
+
+int
+ipmi_pef2_policy(struct ipmi_intf *intf, int argc, char **argv)
+{
+    int rc = 0;
+
+    if (argc < 1) {
+        lprintf(LOG_ERR, "Not enough parameters given.");
+        ipmi_pef2_policy_help();
+        rc = (-1);
+    } else if (!strncmp(argv[0], "help\0", 5)) {
+        ipmi_pef2_policy_help();
+    }
+    else if (!strncmp(argv[0], "list\0", 5)) {
+        ipmi_pef_list_policies(intf);
+    }
+    else if (!strncmp(argv[0], "enable\0", 7)) {
+        rc = ipmi_pef2_policy_enable(intf, 1, --argc, ++argv);
+    }
+    else if (!strncmp(argv[0], "disable\0", 8)) {
+        rc = ipmi_pef2_policy_enable(intf, 0, --argc, ++argv);
+    }
+    else if (!strncmp(argv[0], "create\0", 7)) {
+    }
+    else if (!strncmp(argv[0], "delete\0", 7)) {
+    }
+    else {
+        ipmi_pef2_policy_help();
+    }
+    return rc;
+}
+
+/* Enable/Disable specific PEF filter */
+static int
+ipmi_pef2_filter_enable(struct ipmi_intf *intf, int enable, int argc, char **argv)
+{
+}
+
+void
+ipmi_pef2_filter_help(void)
+{
+    lprintf(LOG_NOTICE, "usage: pef filter list");
+    lprintf(LOG_NOTICE, "       pef filter enable <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef filter disable <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef filter create <id = 1..n> <params>");
+    lprintf(LOG_NOTICE, "       pef filter delete <id = 1..n>");
+}
+
+int
+ipmi_pef2_filter(struct ipmi_intf *intf, int argc, char **argv)
+{
+    int rc = 0;
+
+    if (argc < 1) {   
+        lprintf(LOG_ERR, "Not enough parameters given.");
+        ipmi_pef2_filter_help();
+        rc = (-1);
+    }
+    else if (!strncmp(argv[0], "help\0", 5)) {
+        ipmi_pef2_filter_help();
+    }
+    else if (!strncmp(argv[0], "list\0", 5)) {
+        ipmi_pef_list_entries(intf);
+    }
+    else if (!strncmp(argv[0], "enable\0", 7)) {
+    }
+    else if (!strncmp(argv[0], "disable\0", 8)) {
+    }
+    else if (!strncmp(argv[0], "create\0", 7)) {
+    }
+    else if (!strncmp(argv[0], "delete\0", 7)) {
+    }
+    else {
+      //ipmi_pef2_filterhelp();
+    }
+    return rc;
+}
+
+void
+ipmi_pef2_help(void)
+{
+    lprintf(LOG_NOTICE, "usage: pef help");
+    lprintf(LOG_NOTICE, "       pef timer get");
+    lprintf(LOG_NOTICE, "       pef timer set");
+    lprintf(LOG_NOTICE, "       pef policy list");
+    lprintf(LOG_NOTICE, "       pef policy enable <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef policy disable <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef policy create <id = 1..n> <params>");
+    lprintf(LOG_NOTICE, "       pef policy delete <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef filter list");
+    lprintf(LOG_NOTICE, "       pef filter enable <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef filter disable <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef filter create <id = 1..n> <params>");
+    lprintf(LOG_NOTICE, "       pef filter delete <id = 1..n>");
+    lprintf(LOG_NOTICE, "       pef pet ack <params>");
+}
+
+/*
+   pef help
+   pef timer get
+   pef timer set <xxx>
+   pef policy list
+   pef policy enable <id>
+   pef policy disable <id>
+   pef policy create <id> <params>
+   pef policy delete <id>
+   pef filter list
+   pef filter enable <id>
+   pef filter disable <id>
+   pef filter create <id> <params>
+   pef filter delete <id>
+   pef pet ack <params>
+*/  
 int ipmi_pef_main(struct ipmi_intf * intf, int argc, char ** argv)
 {    /*
     // PEF subcommand handling
     */
-    int help = 0;
-    int rc = 0;
-
-    if (!argc || !strncmp(argv[0], "info", 4))
-        ipmi_pef_get_info(intf);
-    else if (!strncmp(argv[0], "help", 4))
-        help = 1;
-    else if (!strncmp(argv[0], "status", 6))
-        ipmi_pef_get_status(intf);
-    else if (!strncmp(argv[0], "policy", 6))
-        ipmi_pef_list_policies(intf);
-    else if (!strncmp(argv[0], "list", 4))
-        ipmi_pef_list_entries(intf);
+    int rc = 0;
+
+    if (argc < 1) {
+        lprintf(LOG_ERR, "Not enough parameters given.");
+        ipmi_pef2_help();
+        rc = (-1);
+    }
+    else if (!strncmp(argv[0], "help\0", 5)) {
+        ipmi_pef2_help();
+        rc = 0;
+    }
+    else if (!strncmp(argv[0], "info\0", 5)
+         || !strncmp(argv[0], "capabilities\0", 13)) {
+        //rc = ipmi_pef2_get_capabilities(intf);
+    }
+    else if (!strncmp(argv[0], "event\0", 6)) {
+        //rc = ipmi_pef2_event(intf, (argc - 1), ++argv);
+    }
+    else if (!strncmp(argv[0], "filter\0", 7)) {
+        rc = ipmi_pef2_filter(intf, (argc - 1), ++argv);
+    }
+    else if (!strncmp(argv[0], "policy\0", 7)) {
+        rc = ipmi_pef2_policy(intf, (argc - 1), ++argv);
+    }
+    else if (!strncmp(argv[0], "pet\0", 4)) {
+        //rc = ipmi_pef2_pet(intf, (argc - 1), ++argv);
+    }
+    else if (!strncmp(argv[0], "status\0", 7)) {
+        //rc = ipmi_pef2_get_status(intf, (argc - 1), ++argv);
+    }
+    else if (!strncmp(argv[0], "timer\0", 6)) {
+        //rc = ipmi_pef2_timer(intf, (argc - 1), ++argv);
+    }
     else {
-        help = 1;
-        rc   = -1;
         lprintf(LOG_ERR, "Invalid PEF command: '%s'\n", argv[0]);
+        rc = (-1);
     }
-
-    if (help)
-        lprintf(LOG_NOTICE, "PEF commands: info status policy list");
-    else if (!verbose)
-        printf("\n");
-
     return rc;
 }

------------------------------------------------------------------------------
Zdenek Styblik
2015-08-20 16:49:08 UTC
Permalink
On Mon, Aug 17, 2015 at 10:46 PM, Jordan Hargrave
<***@hotmail.com> wrote:
> Some time ago I proposed a patch to enable enabling/disabling individual PEF policy entries.
> Support for this is needed for one of our utilities. A new rework of the whole PEF framework was
> requested. I'm attaching a patch that (partially) implements this new scheme, please review.
>

Hello Jordan,

I'm not aware that rework of the whole PEF framework was prerequisite
for the patch you've posted. Patch you've posted has been rejected
because 'setpolicy' didn't and doesn't make sense in broad view. As a
follow up, new CLI PEF interface with "99%" coverage has been proposed
and cooperated on. Yes, discussion has quieted down and I'm to blame.
My apologies to Pat Donlin at SGI.
Only request that has been made towards you, resp. Dell, was to change
'setpolicy' to 'policy set' which makes much more sense. As far as I'm
aware, this was the only condition for the patch to be accepted and
merged in. If you've interpreted any of it as a
dependency/prerequisite, then I'm sorry.

> http://sourceforge.net/p/ipmitool/bugs/312/

static void
+ipmi_pef_print_oem_lan_dest(struct ipmi_intf *intf, uint8_t ch, uint8_t dest)

Doesn't it matter that something can go wrong in this function?

+ rc = ipmi_mc_getsysinfo(intf, IPMI_SYSINFO_DELL_IPV6_COUNT, 0x00,
0x00, 4, data);
+ if (rc != 0 || dest > data[0]) {
+ return;
+ }
+ ipmi_pef_print_str("Alert destination type", "xxx");
+ ipmi_pef_print_str("PET Community", "xxx");

Are these 'xxx' on purpose?

+ for (set = 1; len > 11; set++) {
+ rc = ipmi_mc_getsysinfo(intf,
IPMI_SYSINFO_DELL_IPV6_DESTADDR, set, dest, 19, data);

No rc check here

+ if ((rlen = len-11) >= IPMI_SYSINFO_SETN_SIZE - 2) {
+ /* Remaining sets have 14 bytes */
+ rlen = IPMI_SYSINFO_SETN_SIZE - 2;
+ }

+ tbl_size = ipmi_pef_get_policy_table(intf, &ptbl);
+ if (!tbl_size) {
+ if (ptbl != NULL) {
+ free(ptbl);

Missing ptbl = NULL;

+ /* Get policy ID number */
+ id = strtol(argv[0], NULL, 0);

strtol()? Nope.

+ else {
+ lprintf(LOG_ERR, "Policy %d already %s", id, enable ?
"enabled" : "disabled");
+ }

Is this error state or isn't?

+ } else {
+ lprintf(LOG_ERR, "Invalid policy index, valid range =
(1..%d)", tbl_size);
+ return (-1);
+ }
+ lprintf(LOG_INFO, "done\n");

Is '\n' here on purpose? If so, then it'd be better to use
lprintf(LOG_INFO, ""); to avoid confusion.

+ formatting
+ commented out code should be removed
+ all not-implemented-calls/sections should be handled by something like:

int
not_implemented() {
lprintf(LOG_NOTICE, "This function isn't implemented.");
return 1;
}

But that's just a suggestion.

Best regards,
Z.

------------------------------------------------------------------------------
hotmail
2015-09-02 16:21:31 UTC
Permalink
> Date: Thu, 20 Aug 2015 18:49:08 +0200
> Subject: Re: [Ipmitool-devel] [PATCH] New PEF policy framework
> From: ***@gmail.com
> To: ***@hotmail.com
> CC: ipmitool-***@lists.sourceforge.net; ***@sgi.com
>
> On Mon, Aug 17, 2015 at 10:46 PM, Jordan Hargrave
> wrote:
>> Some time ago I proposed a patch to enable enabling/disabling individual PEF policy entries.
>> Support for this is needed for one of our utilities. A new rework of the whole PEF framework was
>> requested. I'm attaching a patch that (partially) implements this new scheme, please review.
>>
>
> Hello Jordan,
>
> I'm not aware that rework of the whole PEF framework was prerequisite
> for the patch you've posted. Patch you've posted has been rejected
> because 'setpolicy' didn't and doesn't make sense in broad view. As a
> follow up, new CLI PEF interface with "99%" coverage has been proposed
> and cooperated on. Yes, discussion has quieted down and I'm to blame.
> My apologies to Pat Donlin at SGI.
> Only request that has been made towards you, resp. Dell, was to change
> 'setpolicy' to 'policy set' which makes much more sense. As far as I'm
> aware, this was the only condition for the patch to be accepted and
> merged in. If you've interpreted any of it as a
> dependency/prerequisite, then I'm sorry.
>

Ah well that makes things much easier! Would something like ipmitool pef policy and ipmitool pef policy be OK?

Signed-off-by: Jordan Hargrave
---
doc/ipmitool.1 | 5 +++
include/ipmitool/ipmi_pef.h | 14 +++++++++
lib/ipmi_pef.c | 75 +++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/doc/ipmitool.1 b/doc/ipmitool.1
index 0d8adcc..2cf7c99 100644
--- a/doc/ipmitool.1
+++ b/doc/ipmitool.1
@@ -2425,6 +2425,11 @@ processed by the BMC, etc).
This command lists the PEF policy table entries. Each policy
entry describes an alert destination. A policy set is a
collection of table entries. PEF alert actions reference policy sets.
+.TP
+\fIpolicy\fP
+.br
+
+This command enables or disables individual PEF policy table entries.
.TP
\fIlist\fP
.br
diff --git a/include/ipmitool/ipmi_pef.h b/include/ipmitool/ipmi_pef.h
index cdea4ec..2b392ff 100644
--- a/include/ipmitool/ipmi_pef.h
+++ b/include/ipmitool/ipmi_pef.h
@@ -555,6 +555,19 @@ 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 sel;
+ 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 +949,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
diff --git a/lib/ipmi_pef.c b/lib/ipmi_pef.c
index 1beebf0..305283d 100644
--- a/lib/ipmi_pef.c
+++ b/lib/ipmi_pef.c
@@ -256,6 +256,56 @@ ipmi_pef_get_policy_table(struct ipmi_intf * intf,
return(tbl_size);
}

+static int
+ipmi_pef_set_policy_table_entry(struct ipmi_intf * intf, int set, 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.sel = set & 0x3F;
+ 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;
+}
+
+static void
+ipmi_pef_policy_enable(struct ipmi_intf * intf, int set, int enable)
+{
+ struct pef_cfgparm_policy_table_entry * ptbl = NULL;
+ int tbl_size;
+
+ tbl_size = ipmi_pef_get_policy_table(intf, &ptbl);
+ if (!tbl_size) {
+ if (ptbl) {
+ free(ptbl);
+ }
+ return;
+ }
+ if (set> 0 && set <= tbl_size) {
+ if (enable)
+ ptbl[set-1].entry.policy |= PEF_POLICY_ENABLED;
+ else
+ ptbl[set-1].entry.policy &= ~PEF_POLICY_ENABLED;
+ ipmi_pef_set_policy_table_entry(intf, set, &ptbl[set-1]);
+ } else {
+ lprintf(LOG_ERR, "Invalid policy index, valid range = (1..%d)", tbl_size);
+ }
+}
+
static void
ipmi_pef_print_lan_dest(struct ipmi_intf * intf, uint8_t ch, uint8_t dest)
{ /*
@@ -858,6 +908,12 @@ ipmi_pef_get_info(struct ipmi_intf * intf)
ipmi_pef_print_flags(&pef_b2s_actions, P_SUPP, actions);
}

+struct valstr endis[] = {
+ { .str = "disable", .val = 0x00 },
+ { .str = "enable", .val = 0x01 },
+ { .val = 0xFFFF },
+};
+
int ipmi_pef_main(struct ipmi_intf * intf, int argc, char ** argv)
{ /*
// PEF subcommand handling
@@ -871,8 +927,23 @@ int ipmi_pef_main(struct ipmi_intf * intf, int argc, char ** argv)
help = 1;
else if (!strncmp(argv[0], "status", 6))
ipmi_pef_get_status(intf);
- else if (!strncmp(argv[0], "policy", 6))
- ipmi_pef_list_policies(intf);
+ else if (!strncmp(argv[0], "policy", 6)) {
+ if (argc == 3) {
+ uint16_t pol,en;
+
+ if (str2int(argv[1], &pol) != 0 ||
+ (en = str2val(argv[2], endis)) == 0xFFFF) {
+ lprintf(LOG_ERR, "Usage: ipmitool pef policy \n");
+ return (-1);
+ }
+ ipmi_pef_policy_enable(intf, pol, en);
+ } else if (argc == 1) {
+ ipmi_pef_list_policies(intf);
+ } else {
+ lprintf(LOG_NOTICE, "PEF policy commands: policy | policy ");
+ return (-1);
+ }
+ }
else if (!strncmp(argv[0], "list", 4))
ipmi_pef_list_entries(intf);
else {
--
1.8.3.1
Pat Donlin
2015-09-02 16:35:58 UTC
Permalink
Thank you for the heads up - this change to selectively enable/disable
policy table entries is good for SGI too.

Best regards,

Pat

On 9/2/2015 11:21 AM, hotmail wrote:
>
>> Date: Thu, 20 Aug 2015 18:49:08 +0200
>> Subject: Re: [Ipmitool-devel] [PATCH] New PEF policy framework
>> From: ***@gmail.com
>> To: ***@hotmail.com
>> CC: ipmitool-***@lists.sourceforge.net; ***@sgi.com
>>
>> On Mon, Aug 17, 2015 at 10:46 PM, Jordan Hargrave
>> wrote:
>>> Some time ago I proposed a patch to enable enabling/disabling individual PEF policy entries.
>>> Support for this is needed for one of our utilities. A new rework of the whole PEF framework was
>>> requested. I'm attaching a patch that (partially) implements this new scheme, please review.
>>>
>> Hello Jordan,
>>
>> I'm not aware that rework of the whole PEF framework was prerequisite
>> for the patch you've posted. Patch you've posted has been rejected
>> because 'setpolicy' didn't and doesn't make sense in broad view. As a
>> follow up, new CLI PEF interface with "99%" coverage has been proposed
>> and cooperated on. Yes, discussion has quieted down and I'm to blame.
>> My apologies to Pat Donlin at SGI.
>> Only request that has been made towards you, resp. Dell, was to change
>> 'setpolicy' to 'policy set' which makes much more sense. As far as I'm
>> aware, this was the only condition for the patch to be accepted and
>> merged in. If you've interpreted any of it as a
>> dependency/prerequisite, then I'm sorry.
>>
> Ah well that makes things much easier! Would something like ipmitool pef policy and ipmitool pef policy be OK?
>
> Signed-off-by: Jordan Hargrave
> ---
> doc/ipmitool.1 | 5 +++
> include/ipmitool/ipmi_pef.h | 14 +++++++++
> lib/ipmi_pef.c | 75 +++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/doc/ipmitool.1 b/doc/ipmitool.1
> index 0d8adcc..2cf7c99 100644
> --- a/doc/ipmitool.1
> +++ b/doc/ipmitool.1
> @@ -2425,6 +2425,11 @@ processed by the BMC, etc).
> This command lists the PEF policy table entries. Each policy
> entry describes an alert destination. A policy set is a
> collection of table entries. PEF alert actions reference policy sets.
> +.TP
> +\fIpolicy\fP
> +.br
> +
> +This command enables or disables individual PEF policy table entries.
> .TP
> \fIlist\fP
> .br
> diff --git a/include/ipmitool/ipmi_pef.h b/include/ipmitool/ipmi_pef.h
> index cdea4ec..2b392ff 100644
> --- a/include/ipmitool/ipmi_pef.h
> +++ b/include/ipmitool/ipmi_pef.h
> @@ -555,6 +555,19 @@ 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 sel;
> + 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 +949,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
> diff --git a/lib/ipmi_pef.c b/lib/ipmi_pef.c
> index 1beebf0..305283d 100644
> --- a/lib/ipmi_pef.c
> +++ b/lib/ipmi_pef.c
> @@ -256,6 +256,56 @@ ipmi_pef_get_policy_table(struct ipmi_intf * intf,
> return(tbl_size);
> }
>
> +static int
> +ipmi_pef_set_policy_table_entry(struct ipmi_intf * intf, int set, 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.sel = set & 0x3F;
> + 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;
> +}
> +
> +static void
> +ipmi_pef_policy_enable(struct ipmi_intf * intf, int set, int enable)
> +{
> + struct pef_cfgparm_policy_table_entry * ptbl = NULL;
> + int tbl_size;
> +
> + tbl_size = ipmi_pef_get_policy_table(intf, &ptbl);
> + if (!tbl_size) {
> + if (ptbl) {
> + free(ptbl);
> + }
> + return;
> + }
> + if (set> 0 && set <= tbl_size) {
> + if (enable)
> + ptbl[set-1].entry.policy |= PEF_POLICY_ENABLED;
> + else
> + ptbl[set-1].entry.policy &= ~PEF_POLICY_ENABLED;
> + ipmi_pef_set_policy_table_entry(intf, set, &ptbl[set-1]);
> + } else {
> + lprintf(LOG_ERR, "Invalid policy index, valid range = (1..%d)", tbl_size);
> + }
> +}
> +
> static void
> ipmi_pef_print_lan_dest(struct ipmi_intf * intf, uint8_t ch, uint8_t dest)
> { /*
> @@ -858,6 +908,12 @@ ipmi_pef_get_info(struct ipmi_intf * intf)
> ipmi_pef_print_flags(&pef_b2s_actions, P_SUPP, actions);
> }
>
> +struct valstr endis[] = {
> + { .str = "disable", .val = 0x00 },
> + { .str = "enable", .val = 0x01 },
> + { .val = 0xFFFF },
> +};
> +
> int ipmi_pef_main(struct ipmi_intf * intf, int argc, char ** argv)
> { /*
> // PEF subcommand handling
> @@ -871,8 +927,23 @@ int ipmi_pef_main(struct ipmi_intf * intf, int argc, char ** argv)
> help = 1;
> else if (!strncmp(argv[0], "status", 6))
> ipmi_pef_get_status(intf);
> - else if (!strncmp(argv[0], "policy", 6))
> - ipmi_pef_list_policies(intf);
> + else if (!strncmp(argv[0], "policy", 6)) {
> + if (argc == 3) {
> + uint16_t pol,en;
> +
> + if (str2int(argv[1], &pol) != 0 ||
> + (en = str2val(argv[2], endis)) == 0xFFFF) {
> + lprintf(LOG_ERR, "Usage: ipmitool pef policy \n");
> + return (-1);
> + }
> + ipmi_pef_policy_enable(intf, pol, en);
> + } else if (argc == 1) {
> + ipmi_pef_list_policies(intf);
> + } else {
> + lprintf(LOG_NOTICE, "PEF policy commands: policy | policy ");
> + return (-1);
> + }
> + }
> else if (!strncmp(argv[0], "list", 4))
> ipmi_pef_list_entries(intf);
> else {
Zdenek Styblik
2015-09-02 17:09:35 UTC
Permalink
On Wed, Sep 2, 2015 at 6:21 PM, hotmail <***@hotmail.com> wrote:
>
>
>> Date: Thu, 20 Aug 2015 18:49:08 +0200
>> Subject: Re: [Ipmitool-devel] [PATCH] New PEF policy framework
>> From: ***@gmail.com
>> To: ***@hotmail.com
>> CC: ipmitool-***@lists.sourceforge.net; ***@sgi.com
>>
>> On Mon, Aug 17, 2015 at 10:46 PM, Jordan Hargrave
>> wrote:
>>> Some time ago I proposed a patch to enable enabling/disabling individual PEF policy entries.
>>> Support for this is needed for one of our utilities. A new rework of the whole PEF framework was
>>> requested. I'm attaching a patch that (partially) implements this new scheme, please review.
>>>
>>
>> Hello Jordan,
>>
>> I'm not aware that rework of the whole PEF framework was prerequisite
>> for the patch you've posted. Patch you've posted has been rejected
>> because 'setpolicy' didn't and doesn't make sense in broad view. As a
>> follow up, new CLI PEF interface with "99%" coverage has been proposed
>> and cooperated on. Yes, discussion has quieted down and I'm to blame.
>> My apologies to Pat Donlin at SGI.
>> Only request that has been made towards you, resp. Dell, was to change
>> 'setpolicy' to 'policy set' which makes much more sense. As far as I'm
>> aware, this was the only condition for the patch to be accepted and
>> merged in. If you've interpreted any of it as a
>> dependency/prerequisite, then I'm sorry.
>>
>
> Ah well that makes things much easier! Would something like ipmitool pef policy and ipmitool pef policy be OK?
>

You mean to tell me the whole code review was for nothing? That you've
deliberately wasted my time? And now you're asking me to do yet
another code review? Well, good luck with that!

Z.


> Signed-off-by: Jordan Hargrave
> ---
> doc/ipmitool.1 | 5 +++
> include/ipmitool/ipmi_pef.h | 14 +++++++++
> lib/ipmi_pef.c | 75 +++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/doc/ipmitool.1 b/doc/ipmitool.1
> index 0d8adcc..2cf7c99 100644
> --- a/doc/ipmitool.1
> +++ b/doc/ipmitool.1
> @@ -2425,6 +2425,11 @@ processed by the BMC, etc).
> This command lists the PEF policy table entries. Each policy
> entry describes an alert destination. A policy set is a
> collection of table entries. PEF alert actions reference policy sets.
> +.TP
> +\fIpolicy\fP
> +.br
> +
> +This command enables or disables individual PEF policy table entries.
> .TP
> \fIlist\fP
> .br
> diff --git a/include/ipmitool/ipmi_pef.h b/include/ipmitool/ipmi_pef.h
> index cdea4ec..2b392ff 100644
> --- a/include/ipmitool/ipmi_pef.h
> +++ b/include/ipmitool/ipmi_pef.h
> @@ -555,6 +555,19 @@ 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 sel;
> + 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 +949,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
> diff --git a/lib/ipmi_pef.c b/lib/ipmi_pef.c
> index 1beebf0..305283d 100644
> --- a/lib/ipmi_pef.c
> +++ b/lib/ipmi_pef.c
> @@ -256,6 +256,56 @@ ipmi_pef_get_policy_table(struct ipmi_intf * intf,
> return(tbl_size);
> }
>
> +static int
> +ipmi_pef_set_policy_table_entry(struct ipmi_intf * intf, int set, 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.sel = set & 0x3F;
> + 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;
> +}
> +
> +static void
> +ipmi_pef_policy_enable(struct ipmi_intf * intf, int set, int enable)
> +{
> + struct pef_cfgparm_policy_table_entry * ptbl = NULL;
> + int tbl_size;
> +
> + tbl_size = ipmi_pef_get_policy_table(intf, &ptbl);
> + if (!tbl_size) {
> + if (ptbl) {
> + free(ptbl);
> + }
> + return;
> + }
> + if (set> 0 && set <= tbl_size) {
> + if (enable)
> + ptbl[set-1].entry.policy |= PEF_POLICY_ENABLED;
> + else
> + ptbl[set-1].entry.policy &= ~PEF_POLICY_ENABLED;
> + ipmi_pef_set_policy_table_entry(intf, set, &ptbl[set-1]);
> + } else {
> + lprintf(LOG_ERR, "Invalid policy index, valid range = (1..%d)", tbl_size);
> + }
> +}
> +
> static void
> ipmi_pef_print_lan_dest(struct ipmi_intf * intf, uint8_t ch, uint8_t dest)
> { /*
> @@ -858,6 +908,12 @@ ipmi_pef_get_info(struct ipmi_intf * intf)
> ipmi_pef_print_flags(&pef_b2s_actions, P_SUPP, actions);
> }
>
> +struct valstr endis[] = {
> + { .str = "disable", .val = 0x00 },
> + { .str = "enable", .val = 0x01 },
> + { .val = 0xFFFF },
> +};
> +
> int ipmi_pef_main(struct ipmi_intf * intf, int argc, char ** argv)
> { /*
> // PEF subcommand handling
> @@ -871,8 +927,23 @@ int ipmi_pef_main(struct ipmi_intf * intf, int argc, char ** argv)
> help = 1;
> else if (!strncmp(argv[0], "status", 6))
> ipmi_pef_get_status(intf);
> - else if (!strncmp(argv[0], "policy", 6))
> - ipmi_pef_list_policies(intf);
> + else if (!strncmp(argv[0], "policy", 6)) {
> + if (argc == 3) {
> + uint16_t pol,en;
> +
> + if (str2int(argv[1], &pol) != 0 ||
> + (en = str2val(argv[2], endis)) == 0xFFFF) {
> + lprintf(LOG_ERR, "Usage: ipmitool pef policy \n");
> + return (-1);
> + }
> + ipmi_pef_policy_enable(intf, pol, en);
> + } else if (argc == 1) {
> + ipmi_pef_list_policies(intf);
> + } else {
> + lprintf(LOG_NOTICE, "PEF policy commands: policy | policy ");
> + return (-1);
> + }
> + }
> else if (!strncmp(argv[0], "list", 4))
> ipmi_pef_list_entries(intf);
> else {
> --
> 1.8.3.1
>
Jordan Hargrave
2015-09-02 17:43:07 UTC
Permalink
----------------------------------------
> Date: Wed, 2 Sep 2015 19:09:35 +0200
> Subject: Re: [Ipmitool-devel] [PATCH] New PEF policy framework
> From: ***@gmail.com
> To: ***@hotmail.com
> CC: ipmitool-***@lists.sourceforge.net; ***@sgi.com
>
> On Wed, Sep 2, 2015 at 6:21 PM, hotmail <***@hotmail.com> wrote:
>>
>>
>>> Date: Thu, 20 Aug 2015 18:49:08 +0200
>>> Subject: Re: [Ipmitool-devel] [PATCH] New PEF policy framework
>>> From: ***@gmail.com
>>> To: ***@hotmail.com
>>> CC: ipmitool-***@lists.sourceforge.net; ***@sgi.com
>>>
>>> On Mon, Aug 17, 2015 at 10:46 PM, Jordan Hargrave
>>> wrote:
>>>> Some time ago I proposed a patch to enable enabling/disabling individual PEF policy entries.
>>>> Support for this is needed for one of our utilities. A new rework of the whole PEF framework was
>>>> requested. I'm attaching a patch that (partially) implements this new scheme, please review.
>>>>
>>>
>>> Hello Jordan,
>>>
>>> I'm not aware that rework of the whole PEF framework was prerequisite
>>> for the patch you've posted. Patch you've posted has been rejected
>>> because 'setpolicy' didn't and doesn't make sense in broad view. As a
>>> follow up, new CLI PEF interface with "99%" coverage has been proposed
>>> and cooperated on. Yes, discussion has quieted down and I'm to blame.
>>> My apologies to Pat Donlin at SGI.
>>> Only request that has been made towards you, resp. Dell, was to change
>>> 'setpolicy' to 'policy set' which makes much more sense. As far as I'm
>>> aware, this was the only condition for the patch to be accepted and
>>> merged in. If you've interpreted any of it as a
>>> dependency/prerequisite, then I'm sorry.
>>>
>>
>> Ah well that makes things much easier! Would something like ipmitool pef policy and ipmitool pef policy be OK?
>>
>
> You mean to tell me the whole code review was for nothing? That you've
> deliberately wasted my time? And now you're asking me to do yet
> another code review? Well, good luck with that!
>
> Z.

Don't be an ass.

This is the original patch that I posted in 2013 and have been trying to get upstream so that we can implement additional features.  Several weeks of my time was was wasted developing that PEF rework patch due to your unclear instructions.

--jordan hargrave
Sr. Software Engineer
Dell Enterprise Linux Engineering
Zdenek Styblik
2015-09-02 18:06:23 UTC
Permalink
On Wed, Sep 2, 2015 at 7:43 PM, Jordan Hargrave
<***@hotmail.com> wrote:
>>> Ah well that makes things much easier! Would something like ipmitool pef policy and ipmitool pef policy be OK?
>>>
>>
>> You mean to tell me the whole code review was for nothing? That you've
>> deliberately wasted my time? And now you're asking me to do yet
>> another code review? Well, good luck with that!
>>
>> Z.
>
> Don't be an ass.
>

You call me an ass?!

> This is the original patch that I posted in 2013 and have been trying to get upstream so that we can implement additional features. Several weeks of my time was was wasted developing that PEF rework patch due to your unclear instructions.

Yep, I've rejected it before and guess what, I'll do that again. Get
it in line with the rest of the code in ipmitool and/or in that
particular C file and try your luck again. If you want to throw away
"several weeks of wasted time" due to my "unclear instructions"
instead of contributing and helping others; hey, that's fine by me.
Your call. And btw didn't you get paid for that wasted time?
But there is no way in hell the patch you've posted in that particular
form is going to make it in!

Have a good day, Sir!
Z.

>
> --jordan hargrave
> Sr. Software Engineer
> Dell Enterprise Linux Engineering
>
d F
2015-09-03 16:02:22 UTC
Permalink
This back and forth was brought to mind when looking at a recent paper, which said in the abstract:

Our results show that the level of politeness in the communication process among developers does have an e ect on the time required to x issues and, in the majority of the analysed projects, it has a positive correlation with attractiveness of the project to both active and potential developers. The more polite developers were, the less time it took to x an issue, and, in the majority of the analysed cases, the more the developers wanted to be part of project, the more they were willing to continue working on the project over time.

"Would you mind fixing this issue? An Empirical Analysis of Politeness and Attractiveness in Software Developed Using Agile Boards”

http://www.researchgate.net/publication/271384912_Would_you_mind_fixing_this_issue_An_Empirical_Analysis_of_Politeness_and_Attractiveness_in_Software_Developed_Using_Agile_Boards <http://www.researchgate.net/publication/271384912_Would_you_mind_fixing_this_issue_An_Empirical_Analysis_of_Politeness_and_Attractiveness_in_Software_Developed_Using_Agile_Boards>

— d

> On Sep 2, 2015, at 11:06 AM, Zdenek Styblik <***@gmail.com> wrote:
>
> On Wed, Sep 2, 2015 at 7:43 PM, Jordan Hargrave
> <***@hotmail.com> wrote:
>>>> Ah well that makes things much easier! Would something like ipmitool pef policy and ipmitool pef policy be OK?
>>>>
>>>
>>> You mean to tell me the whole code review was for nothing? That you've
>>> deliberately wasted my time? And now you're asking me to do yet
>>> another code review? Well, good luck with that!
>>>
>>> Z.
>>
>> Don't be an ass.
>>
>
> You call me an ass?!
>
>> This is the original patch that I posted in 2013 and have been trying to get upstream so that we can implement additional features. Several weeks of my time was was wasted developing that PEF rework patch due to your unclear instructions.
>
> Yep, I've rejected it before and guess what, I'll do that again. Get
> it in line with the rest of the code in ipmitool and/or in that
> particular C file and try your luck again. If you want to throw away
> "several weeks of wasted time" due to my "unclear instructions"
> instead of contributing and helping others; hey, that's fine by me.
> Your call. And btw didn't you get paid for that wasted time?
> But there is no way in hell the patch you've posted in that particular
> form is going to make it in!
>
> Have a good day, Sir!
> Z.
>
>>
>> --jordan hargrave
>> Sr. Software Engineer
>> Dell Enterprise Linux Engineering
>>
>
> ------------------------------------------------------------------------------
> Monitor Your Dynamic Infrastructure at Any Scale With Datadog!
> Get real-time metrics from all of your servers, apps and tools
> in one place.
> SourceForge users - Click here to start your Free Trial of Datadog now!
> http://pubads.g.doubleclick.net/gampad/clk?id=241902991&iu=/4140
> _______________________________________________
> Ipmitool-devel mailing list
> Ipmitool-***@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
Loading...