[PATCH] Introduce IPMI BMC Info

[Jeremy Kerr]

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?

> 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;


> @@ -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?


> 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?

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