[PATCH] Introduce IPMI BMC Info
Deb McLemore
debmc at linux.vnet.ibm.com
Tue May 3 13:40:45 UTC 2016
Hi Jeremy, comments in-line below:
On 05/03/2016 02:58 AM, Jeremy Kerr wrote:
> Hi Deb,
>
>> This feature adds the foundation to perform an IPMI BMC Info
>> check that will determine if a Host is capable of
>> IPMI messaging and if so will perform a basic IPMI message
>> exchange to determine the version of IPMI running
>> on the hardware. In the future the IPMI infrastructure
>> can be used to further interrogate the FRU Inventory and other
>> Sensors to help correlate data and surface any
>> discrepancies on inventory or hardware characteristics.
> Nice! A few comments:
>
>> +#include <unistd.h>
>> +
>> +#include "fwts.h"
>> +
>> +int fwts_bmc_info_check(
>> + fwts_framework *fw)
>> +{
>> + fwts_ipmi_rsp *fwts_bmc_info;
>> + fwts_bmc_info = calloc(1, sizeof(fwts_ipmi_rsp));
> Do we need to calloc() that? Can we just use it directly from the stack
> instead?
Will fix, we did a bit of restructuring on the code, so will change to
stack.
>
>> diff --git a/src/lib/src/fwts_firmware.c b/src/lib/src/fwts_firmware.c
>> index 5092ac9..6001cd5 100644
>> --- a/src/lib/src/fwts_firmware.c
>> +++ b/src/lib/src/fwts_firmware.c
>> @@ -32,6 +32,7 @@ static struct {
>> } feature_names[] = {
>> { FWTS_FW_FEATURE_ACPI, "ACPI" },
>> { FWTS_FW_FEATURE_DEVICETREE, "devicetree" },
>> + { FWTS_FW_FEATURE_IPMI, "IPMI" },
>> };
>>
>> /*
>> @@ -63,14 +64,18 @@ int fwts_firmware_features(void)
>> {
>> int features = 0;
>>
>> - /* we just have static feature definitions for now */
>> + /* we check for IPMI presence dynamically to assure available */
>> +
>> + if (fwts_ipmi_present())
>> + features = FWTS_FW_FEATURE_IPMI;
>> +
> I'd probably do this after the switch (fwts_firmware_detect())
> statement; that assigns the base features, then we can OR-in more
> non-type-specific features with these checks.
>
> So, we'd do something like:
>
> switch (fwts_firmware_detect()) {
> case FWTS_FIRMWARE_BIOS:
> case FWTS_FIRMWARE_UEFI:
> features = FWTS_FW_FEATURE_ACPI;
> break;
> case FWTS_FIRMWARE_OPAL:
> features = FWTS_FW_FEATURE_DEVICETREE;
> break;
> default:
> break;
> }
>
> if (fwts_ipmi_present())
> features |= FWTS_FW_FEATURE_IPMI;
>
> if (fwts_other_new_feature_present())
> features |= FWTS_FW_FEATURE_NEW_THING;
Will re-order.
>
>
>> @@ -83,7 +88,7 @@ const char *fwts_firmware_feature_string(const int features)
>> {
>> const int n = FWTS_ARRAY_LEN(feature_names);
>> const char sep[] = ", ";
>> - static char str[50];
>> + static char str[60];
> Did you hit the FWTS_ASSERT before adding this change? I would have
> thought that 50 bytes was large enough to contain the new "IPMI"
> string too (strlen("ACPI, devicetree, IPMI") + 1 == 22).
>
> Or did I get the FWTS_ASSERT check wrong?
Here's some output on the ASSERT check, let me know if this was not the
original intent:
static struct {
enum firmware_feature feature;
const char name[16];
} feature_names[] = {
{ FWTS_FW_FEATURE_ACPI, "ACPI" },
{ FWTS_FW_FEATURE_DEVICETREE, "devicetree" },
{ FWTS_FW_FEATURE_IPMI, "IPMI" },
};
const int n = FWTS_ARRAY_LEN(feature_names);
const char sep[] = ", ";
static char str[60];
size_t len;
char *p;
int i;
/* ensure we have enough space in str to store n names, plus n-1
* separators, plus a trailing nul */
printf("size of feature_names[0].name is %ld.\n",
sizeof(feature_names[0].name));
printf("size of str is %ld.\n", sizeof(str));
printf("size of sep is %ld.\n", sizeof(sep));
printf("n is %i.\n", n);
printf("assert value is %ld.\n",
(n*(sizeof(feature_names[0].name)-1)+( (n-1)*(sizeof(sep)-1) )+1) );
FWTS_ASSERT((n * (sizeof(feature_names[0].name) - 1)) +
((n-1) * (sizeof(sep) - 1)) + 1 <
sizeof(str), str_too_small);
bmc_info: BMC Info
--------------------------------------------------------------------------------------------------------------------------------------------------------
size of feature_names[0].name is 16.
size of str is 60.
size of sep is 3.
n is 3.
assert value is 50.
3 * (16-1) +( 2*2 ) + 1 =
45 + 4 + 1 = 50
>
>
>> diff --git a/src/lib/src/fwts_framework.c b/src/lib/src/fwts_framework.c
>> index d0979c9..2faeb67 100644
>> --- a/src/lib/src/fwts_framework.c
>> +++ b/src/lib/src/fwts_framework.c
>> @@ -593,16 +593,6 @@ static int fwts_framework_run_test(fwts_framework *fw, fwts_framework_test *test
>>
>> fwts_framework_minor_test_progress(fw, 0, "");
>>
>> - if (!fwts_firmware_has_features(test->fw_features)) {
>> - int missing = test->fw_features & ~fwts_firmware_features();
>> - fwts_log_info(fw, "Test skipped, missing features: %s",
>> - fwts_firmware_feature_string(missing));
>> - fw->current_major_test->results.skipped +=
>> - test->ops->total_tests;
>> - fw->total.skipped += test->ops->total_tests;
>> - goto done;
>> - }
>> -
>> if ((test->flags & FWTS_FLAG_ROOT_PRIV) &&
>> (fwts_check_root_euid(fw, true) != FWTS_OK)) {
>> fwts_log_error(fw, "Aborted test, insufficient privilege.");
>> @@ -615,6 +605,16 @@ static int fwts_framework_run_test(fwts_framework *fw, fwts_framework_test *test
>> goto done;
>> }
>>
>> + if (!fwts_firmware_has_features(test->fw_features)) {
>> + int missing = test->fw_features & ~fwts_firmware_features();
>> + fwts_log_info(fw, "Test skipped, missing features: %s",
>> + fwts_firmware_feature_string(missing));
>> + fw->current_major_test->results.skipped +=
>> + test->ops->total_tests;
>> + fw->total.skipped += test->ops->total_tests;
>> + goto done;
>> + }
>> +
> If I'm reading it correctly, this change moves the feature check to
> *after* the ROOT_PRIV check. Was that your intention? Do we need to do
> that?
Moved the ROOT_PRIV check first to bail if ROOT_PRIV's are
required first, so that seems most likely the overall usage flows.
This was changed for IPMI cases since we needed ROOT_PRIV to do the
access check for R_OK | W_OK to successfully function, but
as you point out other users for IPMI may be able to W_OK, so we
can add access check for W_OK in IPMI BMC Info check.
>
> Or is that related to...
>
>> +unsigned int fwts_ipmi_msg_id = 1;
>> +static const char *fwts_ipmi_devnode = "/dev/ipmi0";
>> +
>> +bool fwts_ipmi_present(void) {
>> + return !access(fwts_ipmi_devnode, R_OK | W_OK);
>> +}
> Semantic nit: the platform supports IPMI if !access(..., R_OK); however,
> we can only use it if W_OK.
>
> So, the feature check should be on R_OK, but the test should require
> ROOT_PRIV (or even better, we'd do another W_OK check in the test, as
> the IPMI device may be writable by non-root...).
>
> Cheers,
>
>
> Jeremy
>
--
==========================================
Deb McLemore
IBM OpenPower - IBM Systems
(512) 286 9980
debmc at us.ibm.com
debmc at linux.vnet.ibm.com - (plain text)
==========================================
More information about the fwts-devel
mailing list