[RFC,4/7,v2] fwts: Print names of missing features, rather than a cryptic bitmask

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@ozlabs.org>

---
 src/lib/include/fwts_firmware.h |    4 ++
 src/lib/src/fwts_firmware.c     |   47 ++++++++++++++++++++++++++++++++
 src/lib/src/fwts_framework.c    |    4 +-
 3 files changed, 52 insertions(+), 3 deletions(-)

On 28/04/14 02:35, 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@ozlabs.org>
> 
> ---
>  src/lib/include/fwts_firmware.h |    4 ++
>  src/lib/src/fwts_firmware.c     |   47 ++++++++++++++++++++++++++++++++
>  src/lib/src/fwts_framework.c    |    4 +-
>  3 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lib/include/fwts_firmware.h b/src/lib/include/fwts_firmware.h
> index 265ef4d..52d0a85 100644
> --- a/src/lib/include/fwts_firmware.h
> +++ b/src/lib/include/fwts_firmware.h
> @@ -29,11 +29,13 @@ 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(int features);
>  
>  static inline bool fwts_firmware_has_features(int features)
>  {
> diff --git a/src/lib/src/fwts_firmware.c b/src/lib/src/fwts_firmware.c
> index 4b59cef..136d617 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" },

Most of fwts "acpi" is in capitals, perhaps this should be "ACPI" just
for consistency

> +};
> +
>  /*
>   *  fwts_memory_map_entry_compare()
>   *	callback used to sort memory_map entries on start address
> @@ -64,3 +71,43 @@ int fwts_firmware_features(void)
>  
>  	return features;
>  }
> +
> +const char *fwts_firmware_feature_string(int features)

very minor quibble, perhaps that could be: const int features

> +{
> +	const int n = FWTS_ARRAY_LEN(feature_names);
> +	const char sep[] = ", ";
> +	static char str[50];
> +	int i, len;

perhaps len could be size_t

> +	char *p;
> +
> +	/* 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;
>

Hi Colin,

>> --- 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" },
> 
> Most of fwts "acpi" is in capitals, perhaps this should be "ACPI" just
> for consistency

OK, happy to go with whatever the convention is.

>> +};
>> +
>>  /*
>>   *  fwts_memory_map_entry_compare()
>>   *	callback used to sort memory_map entries on start address
>> @@ -64,3 +71,43 @@ int fwts_firmware_features(void)
>>  
>>  	return features;
>>  }
>> +
>> +const char *fwts_firmware_feature_string(int features)
> 
> very minor quibble, perhaps that could be: const int features

I noticed that in a few places. Happy to change it, but curious as to
why you have consts for pass-by-value arguments?

>> +{
>> +	const int n = FWTS_ARRAY_LEN(feature_names);
>> +	const char sep[] = ", ";
>> +	static char str[50];
>> +	int i, len;
> 
> perhaps len could be size_t

Sounds good.

Thanks,


Jeremy