Message ID | 1398648914.287225.697622787154.4.gpush@pablo |
---|---|
State | Rejected |
Headers | show |
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
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" }, +}; + /* * 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) +{ + const int n = FWTS_ARRAY_LEN(feature_names); + const char sep[] = ", "; + static char str[50]; + int i, len; + 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;
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(-)