Patchwork acpi: checksum: rename the test name from "checksum to "acpichecksum"

login
register
mail settings
Submitter Alex Hung
Date April 25, 2013, 2:43 a.m.
Message ID <1366857839-4726-1-git-send-email-alex.hung@canonical.com>
Download mbox | patch
Permalink /patch/239375/
State Rejected
Headers show

Comments

Alex Hung - April 25, 2013, 2:43 a.m.
The name "checksum" is too generic. The rename can help a user to understand
that the test targets ACPI tables and verifies the table checksum.

Signed-off-by: Alex Hung <alex.hung@canonical.com>
---
 src/acpi/checksum/checksum.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
Colin King - April 25, 2013, 7:22 p.m.
On 25/04/13 03:43, Alex Hung wrote:
> The name "checksum" is too generic. The rename can help a user to understand
> that the test targets ACPI tables and verifies the table checksum.
>
> Signed-off-by: Alex Hung <alex.hung@canonical.com>
> ---
>   src/acpi/checksum/checksum.c |   19 ++++++++++---------
>   1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/src/acpi/checksum/checksum.c b/src/acpi/checksum/checksum.c
> index e56ca66..a8ec15c 100644
> --- a/src/acpi/checksum/checksum.c
> +++ b/src/acpi/checksum/checksum.c
> @@ -26,7 +26,7 @@
>
>   #include "fwts.h"
>
> -static void checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info *table)
> +static void acpi_checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info *table)
>   {
>   	uint8_t checksum;
>   	fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp*)table->data;
> @@ -89,7 +89,7 @@ static void checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info *table)
>
>   }
>
> -static int checksum_scan_tables(fwts_framework *fw)
> +static int acpi_checksum_scan_tables(fwts_framework *fw)
>   {
>   	int i;
>
> @@ -108,7 +108,7 @@ static int checksum_scan_tables(fwts_framework *fw)
>   		hdr = (fwts_acpi_table_header*)table->data;
>
>   		if (strcmp("RSDP", table->name) == 0) {
> -			checksum_rsdp(fw, table);
> +			acpi_checksum_rsdp(fw, table);
>   			continue;
>   		}
>
> @@ -140,19 +140,20 @@ static int checksum_scan_tables(fwts_framework *fw)
>   	return FWTS_OK;
>   }
>
> -static int checksum_test1(fwts_framework *fw)
> +static int acpi_checksum_test1(fwts_framework *fw)
>   {
> -	return checksum_scan_tables(fw);
> +	return acpi_checksum_scan_tables(fw);
>   }
>
> -static fwts_framework_minor_test checksum_tests[] = {
> -	{ checksum_test1, "Check ACPI table checksums." },
> +static fwts_framework_minor_test acpi_checksum_tests[] = {
> +	{ acpi_checksum_test1, "Check ACPI table checksums." },
>   	{ NULL, NULL }
>   };
>
>   static fwts_framework_ops checksum_ops = {
>   	.description = "Check ACPI table checksum.",
> -	.minor_tests = checksum_tests
> +	.minor_tests = acpi_checksum_tests
>   };
>
> -FWTS_REGISTER("checksum", &checksum_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH);
> +FWTS_REGISTER("acpichecksum", &checksum_ops,
> +		FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH);
>

I've been thinking about this all day.  Indeed it is a good idea for us 
to name the tests in a meaningful way but we also need to consider 
ensuring we don't cause too much pain for users who are used to the 
current test naming scheme.

I think that we should re-examine all the current tests and see if any 
more require renaming and *then* apply these changes in one go rather 
than making the changes in a piecemeal fashion.

+1 for renaming this test, but also:
1)  We need the fwts-test renamed
2)  Can this wait until we have re-examined all the current test names.

Colin
Alex Hung - April 26, 2013, 12:46 a.m.
On 04/26/2013 03:22 AM, Colin Ian King wrote:
> On 25/04/13 03:43, Alex Hung wrote:
>> The name "checksum" is too generic. The rename can help a user to
>> understand
>> that the test targets ACPI tables and verifies the table checksum.
>>
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>   src/acpi/checksum/checksum.c |   19 ++++++++++---------
>>   1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/acpi/checksum/checksum.c b/src/acpi/checksum/checksum.c
>> index e56ca66..a8ec15c 100644
>> --- a/src/acpi/checksum/checksum.c
>> +++ b/src/acpi/checksum/checksum.c
>> @@ -26,7 +26,7 @@
>>
>>   #include "fwts.h"
>>
>> -static void checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info
>> *table)
>> +static void acpi_checksum_rsdp(fwts_framework *fw,
>> fwts_acpi_table_info *table)
>>   {
>>       uint8_t checksum;
>>       fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp*)table->data;
>> @@ -89,7 +89,7 @@ static void checksum_rsdp(fwts_framework *fw,
>> fwts_acpi_table_info *table)
>>
>>   }
>>
>> -static int checksum_scan_tables(fwts_framework *fw)
>> +static int acpi_checksum_scan_tables(fwts_framework *fw)
>>   {
>>       int i;
>>
>> @@ -108,7 +108,7 @@ static int checksum_scan_tables(fwts_framework *fw)
>>           hdr = (fwts_acpi_table_header*)table->data;
>>
>>           if (strcmp("RSDP", table->name) == 0) {
>> -            checksum_rsdp(fw, table);
>> +            acpi_checksum_rsdp(fw, table);
>>               continue;
>>           }
>>
>> @@ -140,19 +140,20 @@ static int checksum_scan_tables(fwts_framework *fw)
>>       return FWTS_OK;
>>   }
>>
>> -static int checksum_test1(fwts_framework *fw)
>> +static int acpi_checksum_test1(fwts_framework *fw)
>>   {
>> -    return checksum_scan_tables(fw);
>> +    return acpi_checksum_scan_tables(fw);
>>   }
>>
>> -static fwts_framework_minor_test checksum_tests[] = {
>> -    { checksum_test1, "Check ACPI table checksums." },
>> +static fwts_framework_minor_test acpi_checksum_tests[] = {
>> +    { acpi_checksum_test1, "Check ACPI table checksums." },
>>       { NULL, NULL }
>>   };
>>
>>   static fwts_framework_ops checksum_ops = {
>>       .description = "Check ACPI table checksum.",
>> -    .minor_tests = checksum_tests
>> +    .minor_tests = acpi_checksum_tests
>>   };
>>
>> -FWTS_REGISTER("checksum", &checksum_ops, FWTS_TEST_ANYTIME,
>> FWTS_FLAG_BATCH);
>> +FWTS_REGISTER("acpichecksum", &checksum_ops,
>> +        FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH);
>>
>
> I've been thinking about this all day.  Indeed it is a good idea for us
> to name the tests in a meaningful way but we also need to consider
> ensuring we don't cause too much pain for users who are used to the
> current test naming scheme.
>
> I think that we should re-examine all the current tests and see if any
> more require renaming and *then* apply these changes in one go rather
> than making the changes in a piecemeal fashion.
>
> +1 for renaming this test, but also:
> 1)  We need the fwts-test renamed
> 2)  Can this wait until we have re-examined all the current test names.
>
> Colin
>

Thanks Colin. That's a good idea. Let's examine all names and decide a 
naming convention first. :)

Cheers,
Alex Hung
Alex Hung - April 29, 2013, 8:05 a.m.
Hi,

I have some ideas to share on the naming of the tests.

As some names are general or the abbreviation are hard to understand, it 
may be a good idea to have a meaningful prefix. As firmware are all 
about hardware and software standards, it is good to use specification 
or document names such as acpi, cpu, pcie, uefi and so on.

With these prefixes, one knows which document to read when he/she has 
questions or needs to dig more info errors.

== examples ==

acpi (acpi specification):
mcfg -> acpimcfg (or acpi_mcfg)
fadt -> acpifadt (or acpi_fadt)
...

cpu (intel's software developer manuals)
mtrr -> cpumtrr (or cpu_mtrr)
msr -> cpumsr (or cpu_msr)
nx -> cpunx (or cpu_nx)
...

pcie (pci and pcie specifications)
aspm -> pcieaspm (or pcie_aspm)
maxreadfreq -> pciemaxreadfreq (or pcie_maxreadfreq)
...

and so on


We can also go a little further and implement a group-test feature:

  - "fwts acpi*" to test for acpi tests
  - "fwts cpu*" to test all cpu features
  - so on...

Any feedbacks are mostly appreciated.

Cheers,
Alex Hung


On 04/26/2013 03:22 AM, Colin Ian King wrote:
> On 25/04/13 03:43, Alex Hung wrote:
>> The name "checksum" is too generic. The rename can help a user to
>> understand
>> that the test targets ACPI tables and verifies the table checksum.
>>
>> Signed-off-by: Alex Hung <alex.hung@canonical.com>
>> ---
>>   src/acpi/checksum/checksum.c |   19 ++++++++++---------
>>   1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/acpi/checksum/checksum.c b/src/acpi/checksum/checksum.c
>> index e56ca66..a8ec15c 100644
>> --- a/src/acpi/checksum/checksum.c
>> +++ b/src/acpi/checksum/checksum.c
>> @@ -26,7 +26,7 @@
>>
>>   #include "fwts.h"
>>
>> -static void checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info
>> *table)
>> +static void acpi_checksum_rsdp(fwts_framework *fw,
>> fwts_acpi_table_info *table)
>>   {
>>       uint8_t checksum;
>>       fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp*)table->data;
>> @@ -89,7 +89,7 @@ static void checksum_rsdp(fwts_framework *fw,
>> fwts_acpi_table_info *table)
>>
>>   }
>>
>> -static int checksum_scan_tables(fwts_framework *fw)
>> +static int acpi_checksum_scan_tables(fwts_framework *fw)
>>   {
>>       int i;
>>
>> @@ -108,7 +108,7 @@ static int checksum_scan_tables(fwts_framework *fw)
>>           hdr = (fwts_acpi_table_header*)table->data;
>>
>>           if (strcmp("RSDP", table->name) == 0) {
>> -            checksum_rsdp(fw, table);
>> +            acpi_checksum_rsdp(fw, table);
>>               continue;
>>           }
>>
>> @@ -140,19 +140,20 @@ static int checksum_scan_tables(fwts_framework *fw)
>>       return FWTS_OK;
>>   }
>>
>> -static int checksum_test1(fwts_framework *fw)
>> +static int acpi_checksum_test1(fwts_framework *fw)
>>   {
>> -    return checksum_scan_tables(fw);
>> +    return acpi_checksum_scan_tables(fw);
>>   }
>>
>> -static fwts_framework_minor_test checksum_tests[] = {
>> -    { checksum_test1, "Check ACPI table checksums." },
>> +static fwts_framework_minor_test acpi_checksum_tests[] = {
>> +    { acpi_checksum_test1, "Check ACPI table checksums." },
>>       { NULL, NULL }
>>   };
>>
>>   static fwts_framework_ops checksum_ops = {
>>       .description = "Check ACPI table checksum.",
>> -    .minor_tests = checksum_tests
>> +    .minor_tests = acpi_checksum_tests
>>   };
>>
>> -FWTS_REGISTER("checksum", &checksum_ops, FWTS_TEST_ANYTIME,
>> FWTS_FLAG_BATCH);
>> +FWTS_REGISTER("acpichecksum", &checksum_ops,
>> +        FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH);
>>
>
> I've been thinking about this all day.  Indeed it is a good idea for us
> to name the tests in a meaningful way but we also need to consider
> ensuring we don't cause too much pain for users who are used to the
> current test naming scheme.
>
> I think that we should re-examine all the current tests and see if any
> more require renaming and *then* apply these changes in one go rather
> than making the changes in a piecemeal fashion.
>
> +1 for renaming this test, but also:
> 1)  We need the fwts-test renamed
> 2)  Can this wait until we have re-examined all the current test names.
>
> Colin
>

Patch

diff --git a/src/acpi/checksum/checksum.c b/src/acpi/checksum/checksum.c
index e56ca66..a8ec15c 100644
--- a/src/acpi/checksum/checksum.c
+++ b/src/acpi/checksum/checksum.c
@@ -26,7 +26,7 @@ 
 
 #include "fwts.h"
 
-static void checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info *table)
+static void acpi_checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info *table)
 {
 	uint8_t checksum;
 	fwts_acpi_table_rsdp *rsdp = (fwts_acpi_table_rsdp*)table->data;
@@ -89,7 +89,7 @@  static void checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info *table)
 
 }
 
-static int checksum_scan_tables(fwts_framework *fw)
+static int acpi_checksum_scan_tables(fwts_framework *fw)
 {
 	int i;
 
@@ -108,7 +108,7 @@  static int checksum_scan_tables(fwts_framework *fw)
 		hdr = (fwts_acpi_table_header*)table->data;
 
 		if (strcmp("RSDP", table->name) == 0) {
-			checksum_rsdp(fw, table);
+			acpi_checksum_rsdp(fw, table);
 			continue;
 		}
 
@@ -140,19 +140,20 @@  static int checksum_scan_tables(fwts_framework *fw)
 	return FWTS_OK;
 }
 
-static int checksum_test1(fwts_framework *fw)
+static int acpi_checksum_test1(fwts_framework *fw)
 {
-	return checksum_scan_tables(fw);
+	return acpi_checksum_scan_tables(fw);
 }
 
-static fwts_framework_minor_test checksum_tests[] = {
-	{ checksum_test1, "Check ACPI table checksums." },
+static fwts_framework_minor_test acpi_checksum_tests[] = {
+	{ acpi_checksum_test1, "Check ACPI table checksums." },
 	{ NULL, NULL }
 };
 
 static fwts_framework_ops checksum_ops = {
 	.description = "Check ACPI table checksum.",
-	.minor_tests = checksum_tests
+	.minor_tests = acpi_checksum_tests
 };
 
-FWTS_REGISTER("checksum", &checksum_ops, FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH);
+FWTS_REGISTER("acpichecksum", &checksum_ops,
+		FWTS_TEST_ANYTIME, FWTS_FLAG_BATCH);