Re: [Qemu-devel] [PATCH v5] [resend 2] revamp acpitable parsing and allow to specify complete (headerful) table

[Blue Swirl]

On Thu, May 12, 2011 at 5:44 PM, Michael Tokarev <address@hidden> wrote:
> Since I've got no comments/replies whatsoever, -- neither
> positive nor negative, I assume no one received this email
> (sent on Thu, 12 May 2011, and one more time on Fri, 20
> May 2011), so I'am resending it yet again.  The patch
> still applies to qemu/master.
>
> This patch almost rewrites acpi_table_add() function
> (but still leaves it using old get_param_value() interface).
> The result is that it's now possible to specify whole table
> (together with a header) in an external file, instead of just
> data portion, with a new file= parameter, but at the same time
> it's still possible to specify header fields as before.
>
> Now with the checkpatch.pl formatting fixes, thanks to
> Stefan Hajnoczi for suggestions, with changes from
> Isaku Yamahata, and with my further refinements.
>
> v5: rediffed against current qemu/master.
>
> Signed-off-by: Michael Tokarev <address@hidden>
> ---
>  hw/acpi.c       |  292 
> ++++++++++++++++++++++++++++++++-----------------------
>  qemu-options.hx |    7 +-
>  2 files changed, 175 insertions(+), 124 deletions(-)
>
> diff --git a/hw/acpi.c b/hw/acpi.c
> index ad40fb4..4316189 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -22,17 +22,29 @@
>
>  struct acpi_table_header
>  {
> -    char signature [4];    /* ACPI signature (4 ASCII characters) */
> +    uint16_t _length;         /* our length, not actual part of the hdr */
> +                              /* XXX why we have 2 length fields here? */
> +    char sig[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 */
> +    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 */
> +    char asl_compiler_id[4];  /* ASL compiler vendor ID */
>     uint32_t asl_compiler_revision; /* ASL compiler revision number */
>  } __attribute__((packed));
>
> +#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] =
> +    "\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;
>  size_t acpi_tables_len;
>
> @@ -45,158 +57,192 @@ static int acpi_checksum(const uint8_t *data, int len)
>     return (-sum) & 0xff;
>  }
>
> +/* like strncpy() but zero-fills the tail of destination */
> +static void strzcpy(char *dst, const char *src, size_t size)
> +{
> +    size_t len = strlen(src);
> +    if (len >= size) {
> +        len = size;
> +    } else {
> +      memset(dst + len, 0, size - len);
> +    }
> +    memcpy(dst, src, len);
> +}
> +
> +/* XXX fixme: this function uses obsolete argument parsing interface */
>  int acpi_table_add(const char *t)
>  {
> -    static const char *dfl_id = "QEMUQEMU";
>     char buf[1024], *p, *f;
> -    struct acpi_table_header acpi_hdr;
>     unsigned long val;
> -    uint32_t length;
> -    struct acpi_table_header *acpi_hdr_p;
> -    size_t off;
> +    size_t len, start, allen;
> +    bool has_header;
> +    int changed;
> +    int r;
> +    struct acpi_table_header hdr;
> +
> +    r = 0;
> +    r |= get_param_value(buf, sizeof(buf), "data", t) ? 1 : 0;
> +    r |= get_param_value(buf, sizeof(buf), "file", t) ? 2 : 0;
> +    switch (r) {
> +    case 0:
> +        buf[0] = '\0';
> +    case 1:
> +        has_header = false;
> +        break;
> +    case 2:
> +        has_header = true;
> +        break;
> +    default:
> +        fprintf(stderr, "acpitable: both data and file are specified\n");
> +        return -1;
> +    }
> +
> +    if (!acpi_tables) {
> +        allen = sizeof(uint16_t);
> +        acpi_tables = qemu_mallocz(allen);
> +    }
> +    else {
> +        allen = acpi_tables_len;
> +    }

ERROR: else should follow close brace '}'
#173: FILE: hw/acpi.c:104:
+    }
+    else {