Patchwork [02/11] change element type from "char" to "unsigned char" in ACPI table data

login
register
mail settings
Submitter Laszlo Ersek
Date March 20, 2013, 11:23 p.m.
Message ID <1363821803-3380-3-git-send-email-lersek@redhat.com>
Download mbox | patch
Permalink /patch/229515/
State New
Headers show

Comments

Laszlo Ersek - March 20, 2013, 11:23 p.m.
The data is binary, not textual.

Also, acpi_table_add() abuses the "char *f" pointer -- which normally
points to file names to load -- to poke into the table. Introduce "char
unsigned *table_start" for that purpose.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 hw/pc.h   |    2 +-
 hw/acpi.c |   17 +++++++++--------
 2 files changed, 10 insertions(+), 9 deletions(-)
Eric Blake - March 20, 2013, 11:40 p.m.
On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
> The data is binary, not textual.
> 
> Also, acpi_table_add() abuses the "char *f" pointer -- which normally
> points to file names to load -- to poke into the table. Introduce "char
> unsigned *table_start" for that purpose.
> 
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---

> @@ -112,7 +113,7 @@ int acpi_table_add(const char *t)
>          }
>  
>          for (;;) {
> -            char data[8192];
> +            char unsigned data[8192];

Although this spelling of the type is valid C, it is not typical
convention prior to your patch:

$ git grep 'unsigned char' | wc
    508    3130   34115
$ git grep 'char unsigned' | wc
      0       0       0

> @@ -225,11 +226,11 @@ int acpi_table_add(const char *t)
>      hdr.checksum = 0;    /* for checksum calculation */
>  
>      /* put header back */
> -    memcpy(f, &hdr, sizeof(hdr));
> +    memcpy(table_start, &hdr, sizeof(hdr));
>  
>      if (changed || !has_header || 1) {
> -        ((struct acpi_table_header *)f)->checksum =
> -            acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len);
> +        ((struct acpi_table_header *)table_start)->checksum =
> +            acpi_checksum((uint8_t *)table_start + ACPI_TABLE_PFX_SIZE, len);

Now that table_start is an unsigned char *, do you still need the cast
to uint8_t*?
Laszlo Ersek - March 21, 2013, 12:18 a.m.
On 03/21/13 00:40, Eric Blake wrote:
> On 03/20/2013 05:23 PM, Laszlo Ersek wrote:
>> The data is binary, not textual.
>>
>> Also, acpi_table_add() abuses the "char *f" pointer -- which normally
>> points to file names to load -- to poke into the table. Introduce "char
>> unsigned *table_start" for that purpose.
>>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
> 
>> @@ -112,7 +113,7 @@ int acpi_table_add(const char *t)
>>          }
>>  
>>          for (;;) {
>> -            char data[8192];
>> +            char unsigned data[8192];
> 
> Although this spelling of the type is valid C, it is not typical
> convention prior to your patch:
> 
> $ git grep 'unsigned char' | wc
>     508    3130   34115
> $ git grep 'char unsigned' | wc
>       0       0       0

Will fix if a respin is required! :)

>> @@ -225,11 +226,11 @@ int acpi_table_add(const char *t)
>>      hdr.checksum = 0;    /* for checksum calculation */
>>  
>>      /* put header back */
>> -    memcpy(f, &hdr, sizeof(hdr));
>> +    memcpy(table_start, &hdr, sizeof(hdr));
>>  
>>      if (changed || !has_header || 1) {
>> -        ((struct acpi_table_header *)f)->checksum =
>> -            acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len);
>> +        ((struct acpi_table_header *)table_start)->checksum =
>> +            acpi_checksum((uint8_t *)table_start + ACPI_TABLE_PFX_SIZE, len);
> 
> Now that table_start is an unsigned char *, do you still need the cast
> to uint8_t*?

Hrmpf. Nope. Thankfully this code is going all away later on in the series.

Thanks for reviewing it!
Laszlo

Patch

diff --git a/hw/pc.h b/hw/pc.h
index dbbd8cd..fe1768b 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -109,7 +109,7 @@  void ioapic_init_gsi(GSIState *gsi_state, const char *parent_name);
 
 /* acpi.c */
 extern int acpi_enabled;
-extern char *acpi_tables;
+extern char unsigned *acpi_tables;
 extern size_t acpi_tables_len;
 
 void acpi_bios_init(void);
diff --git a/hw/acpi.c b/hw/acpi.c
index 53e47d5..c6320d5 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -41,14 +41,14 @@  struct acpi_table_header {
 #define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
 #define ACPI_TABLE_PFX_SIZE sizeof(uint16_t)  /* size of the extra prefix */
 
-static const char dfl_hdr[ACPI_TABLE_HDR_SIZE] =
+static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE] =
     "\0\0"                   /* fake _length (2) */
     "QEMU\0\0\0\0\1\0"       /* sig (4), len(4), revno (1), csum (1) */
     "QEMUQEQEMUQEMU\1\0\0\0" /* OEM id (6), table (8), revno (4) */
     "QEMU\1\0\0\0"           /* ASL compiler ID (4), version (4) */
     ;
 
-char *acpi_tables;
+char unsigned *acpi_tables;
 size_t acpi_tables_len;
 
 static int acpi_checksum(const uint8_t *data, int len)
@@ -71,6 +71,7 @@  int acpi_table_add(const char *t)
     int changed;
     int r;
     struct acpi_table_header hdr;
+    char unsigned *table_start;
 
     r = 0;
     r |= get_param_value(buf, sizeof(buf), "data", t) ? 1 : 0;
@@ -112,7 +113,7 @@  int acpi_table_add(const char *t)
         }
 
         for (;;) {
-            char data[8192];
+            char unsigned data[8192];
             r = read(fd, data, sizeof(data));
             if (r == 0) {
                 break;
@@ -133,11 +134,11 @@  int acpi_table_add(const char *t)
 
     /* now fill in the header fields */
 
-    f = acpi_tables + start;   /* start of the table */
+    table_start = acpi_tables + start;   /* start of the table */
     changed = 0;
 
     /* copy the header to temp place to align the fields */
-    memcpy(&hdr, has_header ? f : dfl_hdr, ACPI_TABLE_HDR_SIZE);
+    memcpy(&hdr, has_header ? table_start : dfl_hdr, ACPI_TABLE_HDR_SIZE);
 
     /* length of the table minus our prefix */
     len = allen - start - ACPI_TABLE_PFX_SIZE;
@@ -225,11 +226,11 @@  int acpi_table_add(const char *t)
     hdr.checksum = 0;    /* for checksum calculation */
 
     /* put header back */
-    memcpy(f, &hdr, sizeof(hdr));
+    memcpy(table_start, &hdr, sizeof(hdr));
 
     if (changed || !has_header || 1) {
-        ((struct acpi_table_header *)f)->checksum =
-            acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len);
+        ((struct acpi_table_header *)table_start)->checksum =
+            acpi_checksum((uint8_t *)table_start + ACPI_TABLE_PFX_SIZE, len);
     }
 
     /* increase number of tables */