On Mon, Aug 17, 2015 at 10:46 PM, Jordan Hargrave <jordan_hargr...@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. ------------------------------------------------------------------------------ _______________________________________________ Ipmitool-devel mailing list Ipmitool-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ipmitool-devel
