ACK: [PATCH 4/7 v3] fwts: Print names of missing features, rather than a cryptic bitmask

[Alex Hung]

On 07/24/2014 05:07 PM, Jeremy Kerr wrote:
> This change adds a function to return a comma-separated list of features
> in a bitmask, which we use when printing missing firmware features when
> we skip a test.
>
> Signed-off-by: Jeremy Kerr <jk at ozlabs.org>
>
> ---
>   src/lib/include/fwts_firmware.h |    6 ++--
>   src/lib/src/fwts_firmware.c     |   48 ++++++++++++++++++++++++++++++++
>   src/lib/src/fwts_framework.c    |    4 +-
>   3 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/src/lib/include/fwts_firmware.h b/src/lib/include/fwts_firmware.h
> index 265ef4d..cd57acc 100644
> --- a/src/lib/include/fwts_firmware.h
> +++ b/src/lib/include/fwts_firmware.h
> @@ -29,13 +29,15 @@ enum firmware_type {
>   };
>
>   enum firmware_feature {
> -	FWTS_FW_FEATURE_ACPI = 1 << 0,
> +	FWTS_FW_FEATURE_ACPI		= 0x1,
> +	FWTS_FW_FEATURE_ALL		= FWTS_FW_FEATURE_ACPI
>   };
>
>   int fwts_firmware_detect(void);
>   int fwts_firmware_features(void);
> +const char *fwts_firmware_feature_string(const int features);
>
> -static inline bool fwts_firmware_has_features(int features)
> +static inline bool fwts_firmware_has_features(const int features)
>   {
>   	return (fwts_firmware_features() & features) == features;
>   }
> diff --git a/src/lib/src/fwts_firmware.c b/src/lib/src/fwts_firmware.c
> index 4b59cef..8723906 100644
> --- a/src/lib/src/fwts_firmware.c
> +++ b/src/lib/src/fwts_firmware.c
> @@ -26,6 +26,13 @@
>   static enum firmware_type firmware_type;
>   static bool firmware_type_valid;
>
> +static struct {
> +	enum firmware_feature feature;
> +	const char name[16];
> +} feature_names[] = {
> +	{ FWTS_FW_FEATURE_ACPI,		"ACPI" },
> +};
> +
>   /*
>    *  fwts_memory_map_entry_compare()
>    *	callback used to sort memory_map entries on start address
> @@ -64,3 +71,44 @@ int fwts_firmware_features(void)
>
>   	return features;
>   }
> +
> +const char *fwts_firmware_feature_string(const int features)
> +{
> +	const int n = FWTS_ARRAY_LEN(feature_names);
> +	const char sep[] = ", ";
> +	static char str[50];
> +	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 */
> +	FWTS_ASSERT((n * (sizeof(feature_names[0].name) - 1)) +
> +				((n-1) * (sizeof(sep) - 1)) + 1 <
> +			sizeof(str), str_too_small);
> +
> +	/* ensure we have a name defined for all features */
> +	FWTS_ASSERT(((1 << n) - 1) == FWTS_FW_FEATURE_ALL,
> +			invalid_feature_names);
> +
> +	for (p = str, i = 0; i < n; i++) {
> +
> +		if (!(features & feature_names[i].feature))
> +			continue;
> +
> +		/* if this isn't the first feature, add a separator */
> +		if (p != str) {
> +			len = sizeof(sep) - 1;
> +			memcpy(p, sep, len);
> +			p += len;
> +		}
> +
> +		len = strlen(feature_names[i].name);
> +		memcpy(p, feature_names[i].name, len);
> +		p += len;
> +	}
> +
> +	*p = '\0';
> +
> +	return str;
> +}
> diff --git a/src/lib/src/fwts_framework.c b/src/lib/src/fwts_framework.c
> index d200d26..669a944 100644
> --- a/src/lib/src/fwts_framework.c
> +++ b/src/lib/src/fwts_framework.c
> @@ -507,8 +507,8 @@ static int fwts_framework_run_test(fwts_framework *fw, fwts_framework_test *test
>
>   	if (!fwts_firmware_has_features(test->fw_features)) {
>   		int missing = test->fw_features & ~fwts_firmware_features();
> -		fwts_log_info(fw, "Test skipped, missing features 0x%08x",
> -				missing);
> +		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;
>

Acked-by: Alex Hung <alex.hung at canonical.com>