qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] v2 revamp acpitable parsing and allow to specify comple


From: Isaku Yamahata
Subject: Re: [Qemu-devel] v2 revamp acpitable parsing and allow to specify complete (headerful) table
Date: Thu, 17 Mar 2011 14:19:40 +0900
User-agent: Mutt/1.5.19 (2009-01-05)

Ouch. You already clean it up.

Here is my diff from your patch. Can you please merged into the patch?
changes are
- eliminate unaligned access
- error report
- replace magic number with symbolic constants
- unconverted strtol(base=10)

Signed-off-by: Isaku Yamahata <address@hidden>

--- qemu-acpi-load-other-0/hw/acpi.c    2011-03-17 14:02:07.000000000 +0900
+++ qemu-acpi-load-0/hw/acpi.c  2011-03-17 14:14:39.000000000 +0900
@@ -19,8 +19,42 @@
 #include "pc.h"
 #include "acpi.h"
 
+struct acpi_table_header
+{
+    char signature [4];    /* ACPI signature (4 ASCII characters) */
+    uint32_t length;          /* Length of table, in bytes, including header */
+    uint8_t revision;         /* ACPI Specification minor version # */
+    uint8_t checksum;         /* To make sum of entire table == 0 */
+    char oem_id [6];       /* OEM identification */
+    char oem_table_id [8]; /* OEM table identification */
+    uint32_t oem_revision;    /* OEM revision number */
+    char asl_compiler_id [4]; /* ASL compiler vendor ID */
+    uint32_t asl_compiler_revision; /* ASL compiler revision number */
+} __attribute__((packed));
+
+#define ACPI_TABLE_OFF(x)       (offsetof(struct acpi_table_header, x))
+#define ACPI_TABLE_SIZE(x)      (sizeof(((struct acpi_table_header*)0)->x))
+
+#define ACPI_TABLE_SIG_OFF              ACPI_TABLE_OFF(signature)
+#define ACPI_TABLE_SIG_SIZE             ACPI_TABLE_SIZE(signature)
+#define ACPI_TABLE_LEN_OFF              ACPI_TABLE_OFF(length)
+#define ACPI_TABLE_LEN_SIZE             ACPI_TABLE_SIZE(length)
+#define ACPI_TABLE_REV_OFF              ACPI_TABLE_OFF(revision)
+#define ACPI_TABLE_REV_SIZE             ACPI_TABLE_SIZE(revision)
+#define ACPI_TABLE_CSUM_OFF             ACPI_TABLE_OFF(checksum)
+#define ACPI_TABLE_CSUM_SIZE            ACPI_TABLE_SIZE(checksum)
+#define ACPI_TABLE_OEM_ID_OFF           ACPI_TABLE_OFF(oem_id)
+#define ACPI_TABLE_OEM_ID_SIZE          ACPI_TABLE_SIZE(oem_id)
+#define ACPI_TABLE_OEM_TABLE_ID_OFF     ACPI_TABLE_OFF(oem_table_id)
+#define ACPI_TABLE_OEM_TABLE_ID_SIZE    ACPI_TABLE_SIZE(oem_table_id)
+#define ACPI_TABLE_OEM_REV_OFF          ACPI_TABLE_OFF(oem_revision)
+#define ACPI_TABLE_OEM_REV_SIZE         ACPI_TABLE_SIZE(oem_revision)
+#define ACPI_TABLE_ASL_COMPILER_ID_OFF  ACPI_TABLE_OFF(asl_compiler_id)
+#define ACPI_TABLE_ASL_COMPILER_ID_SIZE ACPI_TABLE_SIZE(asl_compiler_id)
+#define ACPI_TABLE_ASL_COMPILER_REV_OFF ACPI_TABLE_OFF(asl_compiler_revision)
+#define ACPI_TABLE_ASL_COMPILER_REV_SIZE ACPI_TABLE_SIZE(asl_compiler_revision)
 
-#define ACPI_TABLE_HDR_SIZE (4+4+1+1+6+8+4+4+4)
+#define ACPI_TABLE_HDR_SIZE sizeof(struct acpi_table_header)
 
 char *acpi_tables;
 size_t acpi_tables_len;
@@ -34,6 +68,7 @@ static int acpi_checksum(const uint8_t *
     return (-sum) & 0xff;
 }
 
+/* XXX fixme: this function uses obsolete argument parsing interface */
 int acpi_table_add(const char *t)
 {
     static const char dfl_hdr[ACPI_TABLE_HDR_SIZE] =
@@ -43,17 +78,16 @@ int acpi_table_add(const char *t)
         ;
     char buf[1024], *p, *f;
     unsigned long val;
+    uint16_t val16;
+    uint32_t val32;
     size_t len, start;
     bool has_header;
     int changed;
 
-    /*XXX fixme: this function uses obsolete argument parsing interface */
-    /*XXX note: all 32bit accesses in there are misaligned */
-
     if (get_param_value(buf, sizeof(buf), "data", t)) {
-        has_header = 0;
+        has_header = false;
     } else if (get_param_value(buf, sizeof(buf), "file", t)) {
-        has_header = 1;
+        has_header = true;
     } else {
         has_header = 0;
         buf[0] = '\0';
@@ -80,7 +114,7 @@ int acpi_table_add(const char *t)
         int fd = open(f, O_RDONLY);
 
         if (fd < 0) {
-            /*XXX fixme: report error */
+            fprintf(stderr, "can't open file %s: %s\n", f, strerror(errno));
             goto out;
         }
 
@@ -94,7 +128,8 @@ int acpi_table_add(const char *t)
                 memcpy(acpi_tables + acpi_tables_len, data, r);
                 acpi_tables_len += r;
             } else if (errno != EINTR) {
-                /*XXX fixme: report error */
+                fprintf(stderr, "can't read file %s: %s\n",
+                        f, strerror(errno));
                 close(fd);
                 goto out;
             }
@@ -106,7 +141,8 @@ int acpi_table_add(const char *t)
     /* fill in the complete length of the table */
     len = acpi_tables_len - start - sizeof(uint16_t);
     f = acpi_tables + start;
-    *(uint16_t *)f = cpu_to_le32(len);
+    val16 = cpu_to_le16(len);
+    memcpy(f, &val16, sizeof(uint16_t));
     f += sizeof(uint16_t);
 
     /* now fill in the header fields */
@@ -114,7 +150,7 @@ int acpi_table_add(const char *t)
 
     /* 0..3, signature, string (4 bytes) */
     if (get_param_value(buf, sizeof(buf), "sig", t)) {
-        strncpy(f + 0, buf, 4);
+        strncpy(f + ACPI_TABLE_SIG_OFF, buf, ACPI_TABLE_SIG_SIZE);
         ++changed;
     }
 
@@ -124,23 +160,26 @@ int acpi_table_add(const char *t)
     if (get_param_value(buf, sizeof(buf), "rev", t)) {
         val = strtoul(buf, &p, 0);
         if (val > 255 || *p) {
-            goto out;   /*XXX fixme: report error */
+            fprintf(stderr, "invalid acpi rev.\n");
+            goto out;
         }
-        f[8] = (uint8_t)val;
+        f[ACPI_TABLE_REV_OFF] = (uint8_t)val;
         ++changed;
     }
 
-    /* 9, checksum of entire table (1 byte) */
+    /* 9, checksum of entire table (1 byte)
+       this will be processed after all the headers are modified */
 
     /* 10..15 OEM identification (6 bytes) */
     if (get_param_value(buf, sizeof(buf), "oem_id", t)) {
-        strncpy(f + 10, buf, 6);
+        strncpy(f + ACPI_TABLE_OEM_ID_OFF, buf, ACPI_TABLE_OEM_ID_SIZE);
         ++changed;
     }
 
     /* 16..23 OEM table identifiaction, 8 bytes */
     if (get_param_value(buf, sizeof(buf), "oem_table_id", t)) {
-        strncpy(f + 16, buf, 8);
+        strncpy(f + ACPI_TABLE_OEM_TABLE_ID_OFF, buf,
+                ACPI_TABLE_OEM_TABLE_ID_SIZE);
         ++changed;
     }
 
@@ -148,25 +187,31 @@ int acpi_table_add(const char *t)
     if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
         val = strtol(buf, &p, 0);
         if (*p) {
-            goto out;   /*XXX fixme: report error */
+            fprintf(stderr, "invalid acpi oem_rev.\n");
+            goto out;
         }
-        *(uint32_t *)(f + 24) = cpu_to_le32(val);
+        val32 = cpu_to_le32(val);
+        memcpy(f + ACPI_TABLE_OEM_REV_OFF, &val32, ACPI_TABLE_OEM_REV_SIZE);
         ++changed;
     }
 
     /* 28..31 ASL compiler vendor ID (4 bytes) */
     if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t)) {
-        strncpy(f + 28, buf, 4);
+        strncpy(f + ACPI_TABLE_ASL_COMPILER_ID_OFF, buf,
+                ACPI_TABLE_ASL_COMPILER_ID_SIZE);
         ++changed;
     }
 
     /* 32..35 ASL compiler revision number (4 bytes) */
     if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) {
-        val = strtol(buf, &p, 10);
+        val = strtol(buf, &p, 0);
         if (*p) {
-            goto out;   /*XXX fixme: report error */
+            fprintf(stderr, "invalid acpi asl_compiler_rev.\n");
+            goto out;
         }
-        *(uint32_t *)(f + 32) = cpu_to_le32(val);
+        val32 = cpu_to_le32(val);
+        memcpy(f + ACPI_TABLE_ASL_COMPILER_REV_OFF, &val32,
+               ACPI_TABLE_ASL_COMPILER_REV_SIZE);
         ++changed;
     }
 
@@ -179,11 +224,12 @@ int acpi_table_add(const char *t)
         }
     } else {
         /* check if actual length is correct */
-        val = le32_to_cpu(*(uint32_t *)(f + 4));
+        memcpy(&val32, f + ACPI_TABLE_LEN_OFF, ACPI_TABLE_LEN_SIZE);
+        val = le32_to_cpu(val32);
         if (val != len) {
             fprintf(stderr,
                 "warning: acpi table specified with file= has wrong length,"
-                " header says %lu, actual size %u\n",
+                " header says %lu, actual size %zu\n",
                 val, len);
             ++changed;
         }
@@ -191,19 +237,20 @@ int acpi_table_add(const char *t)
 
     /* fix table length */
     /* we may avoid putting length here if has_header is true */
-    *(uint32_t *)(f + 4) = cpu_to_le32(len);
+    val32 = cpu_to_le32(len);
+    memcpy(f + ACPI_TABLE_LEN_OFF, &val32, ACPI_TABLE_LEN_SIZE);
 
     /* 9 checksum (1 byte) */
     /* we may as well leave checksum intact if has_header is true */
     /* alternatively there may be a way to set cksum to a given value */
     if (changed || !has_header || 1) {
-        f[9] = 0;
-        f[9] = acpi_checksum((uint8_t *)f, len);
+        f[ACPI_TABLE_CSUM_OFF] = 0;
+        f[ACPI_TABLE_CSUM_OFF] = acpi_checksum((uint8_t*)f, len);
     }
 
     /* increase number of tables */
-    (*(uint16_t *)acpi_tables) =
-            cpu_to_le32(le32_to_cpu(*(uint16_t *)acpi_tables) + 1);
+    (*(uint16_t*)acpi_tables) =
+        cpu_to_le16(le16_to_cpu(*(uint16_t*)acpi_tables) + 1);
     return 0;
 
 out:


-- 
yamahata



reply via email to

[Prev in Thread] Current Thread [Next in Thread]