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

[Blue Swirl]

On Fri, Jul 15, 2011 at 6:18 PM, John Baboval
<address@hidden> wrote:
> Is there something I can do to help take this patch the rest of the way?
>
> I'd hate to see it die because of a style issue and a compiler warning.

There's also suspicious missing 'break' statement. How about fixing
the issues and submitting the patch?

> -John
>
> On 06/15/2011 02:19 PM, Blue Swirl wrote:
>>
>> On Sat, Jun 11, 2011 at 1:29 AM, Michael Tokarev<address@hidden>  wrote:
>>>
>>> I've given up on this one.  Personally I don't need
>>> this stuff for my win7 guests since I can hack either
>>> bios or the O/S loader to include all the necessary
>>> verifications for the win7 activation to work.  I
>>> tried to make this process to be legal (no hacks
>>> or "cracks" needed) and easy for others, but since
>>> noone is interested in this after 6 versions and 5
>>> resends, I wont continue - what I am trying to achieve
>>> by pushing this so hard, after all?
>>>
>>> Thanks to everyone who gave (mostly code style) comments -
>>> at least I know the changes has been read by someone.
>>>
>>> Frustrated,
>>
>> Sorry about that. I get this error:
>> /src/qemu/hw/acpi.c: In function 'acpi_table_add':
>> /src/qemu/hw/acpi.c:167: error: format '%u' expects type 'unsigned
>> int', but argument 4 has type 'size_t'
>>
>>>  /mjt
>>>
>>> 12.05.2011 18:44, Michael Tokarev wrote:
>>>>
>>>> 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.
>>>> v6: fix one "} else {" coding style defect (noted by Blue Swirl)
>>>>
>>>> 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';
>>
>> Missing 'break' or comment about fall through.
>>
>>>> +    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 {
>>
>> 'else' belongs to the previous line.
>>
>>>> +        allen = acpi_tables_len;
>>>> +    }
>>>> +
>>>> +    start = allen;
>>>> +    acpi_tables = qemu_realloc(acpi_tables, start +
>>>> ACPI_TABLE_HDR_SIZE);
>>>> +    allen += has_header ? ACPI_TABLE_PFX_SIZE : ACPI_TABLE_HDR_SIZE;
>>>> +
>>>> +    /* now read in the data files, reallocating buffer as needed */
>>>> +
>>>> +    for (f = strtok(buf, ":"); f; f = strtok(NULL, ":")) {
>>>> +        int fd = open(f, O_RDONLY);
>>>> +
>>>> +        if (fd<  0) {
>>>> +            fprintf(stderr, "can't open file %s: %s\n", f,
>>>> strerror(errno));
>>>> +            return -1;
>>>> +        }
>>>> +
>>>> +        for (;;) {
>>>> +            char data[8192];
>>>> +            r = read(fd, data, sizeof(data));
>>>> +            if (r == 0) {
>>>> +                break;
>>>> +            } else if (r>  0) {
>>>> +                acpi_tables = qemu_realloc(acpi_tables, allen + r);
>>>> +                memcpy(acpi_tables + allen, data, r);
>>>> +                allen += r;
>>>> +            } else if (errno != EINTR) {
>>>> +                fprintf(stderr, "can't read file %s: %s\n",
>>>> +                        f, strerror(errno));
>>>> +                close(fd);
>>>> +                return -1;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        close(fd);
>>>> +    }
>>>> +
>>>> +    /* now fill in the header fields */
>>>> +
>>>> +    f = 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);
>>>> +
>>>> +    /* length of the table minus our prefix */
>>>> +    len = allen - start - ACPI_TABLE_PFX_SIZE;
>>>> +
>>>> +    hdr._length = cpu_to_le16(len);
>>>>
>>>> -    memset(&acpi_hdr, 0, sizeof(acpi_hdr));
>>>> -
>>>>      if (get_param_value(buf, sizeof(buf), "sig", t)) {
>>>> -        strncpy(acpi_hdr.signature, buf, 4);
>>>> -    } else {
>>>> -        strncpy(acpi_hdr.signature, dfl_id, 4);
>>>> +        strzcpy(hdr.sig, buf, sizeof(hdr.sig));
>>>> +        ++changed;
>>>>      }
>>>> +
>>>> +    /* length of the table including header, in bytes */
>>>> +    if (has_header) {
>>>> +        /* check if actual length is correct */
>>>> +        val = le32_to_cpu(hdr.length);
>>>> +        if (val != len) {
>>>> +            fprintf(stderr,
>>>> +                "warning: acpitable has wrong length,"
>>>> +                " header says %lu, actual size %u bytes\n",
>>>> +                val, len);
>>>> +            ++changed;
>>>> +        }
>>>> +    }
>>>> +    /* we may avoid putting length here if has_header is true */
>>>> +    hdr.length = cpu_to_le32(len);
>>>> +
>>>>      if (get_param_value(buf, sizeof(buf), "rev", t)) {
>>>> -        val = strtoul(buf,&p, 10);
>>>> -        if (val>  255 || *p != '\0')
>>>> -            goto out;
>>>> -    } else {
>>>> -        val = 1;
>>>> +        val = strtoul(buf,&p, 0);
>>>> +        if (val>  255 || *p) {
>>>> +            fprintf(stderr, "acpitable: \"rev=%s\" is invalid\n", buf);
>>>> +            return -1;
>>>> +        }
>>>> +        hdr.revision = (uint8_t)val;
>>>> +        ++changed;
>>>>      }
>>>> -    acpi_hdr.revision = (int8_t)val;
>>>>
>>>>      if (get_param_value(buf, sizeof(buf), "oem_id", t)) {
>>>> -        strncpy(acpi_hdr.oem_id, buf, 6);
>>>> -    } else {
>>>> -        strncpy(acpi_hdr.oem_id, dfl_id, 6);
>>>> +        strzcpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
>>>> +        ++changed;
>>>>      }
>>>>
>>>>      if (get_param_value(buf, sizeof(buf), "oem_table_id", t)) {
>>>> -        strncpy(acpi_hdr.oem_table_id, buf, 8);
>>>> -    } else {
>>>> -        strncpy(acpi_hdr.oem_table_id, dfl_id, 8);
>>>> +        strzcpy(hdr.oem_table_id, buf, sizeof(hdr.oem_table_id));
>>>> +        ++changed;
>>>>      }
>>>>
>>>>      if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
>>>> -        val = strtol(buf,&p, 10);
>>>> -        if(*p != '\0')
>>>> -            goto out;
>>>> -    } else {
>>>> -        val = 1;
>>>> +        val = strtol(buf,&p, 0);
>>>> +        if (*p) {
>>>> +            fprintf(stderr, "acpitable: \"oem_rev=%s\" is invalid\n",
>>>> buf);
>>>> +            return -1;
>>>> +        }
>>>> +        hdr.oem_revision = cpu_to_le32(val);
>>>> +        ++changed;
>>>>      }
>>>> -    acpi_hdr.oem_revision = cpu_to_le32(val);
>>>>
>>>>      if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t)) {
>>>> -        strncpy(acpi_hdr.asl_compiler_id, buf, 4);
>>>> -    } else {
>>>> -        strncpy(acpi_hdr.asl_compiler_id, dfl_id, 4);
>>>> +        strzcpy(hdr.asl_compiler_id, buf, sizeof(hdr.asl_compiler_id));
>>>> +        ++changed;
>>>>      }
>>>>
>>>>      if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) {
>>>> -        val = strtol(buf,&p, 10);
>>>> -        if(*p != '\0')
>>>> -            goto out;
>>>> -    } else {
>>>> -        val = 1;
>>>> -    }
>>>> -    acpi_hdr.asl_compiler_revision = cpu_to_le32(val);
>>>> -
>>>> -    if (!get_param_value(buf, sizeof(buf), "data", t)) {
>>>> -         buf[0] = '\0';
>>>> -    }
>>>> -
>>>> -    length = sizeof(acpi_hdr);
>>>> -
>>>> -    f = buf;
>>>> -    while (buf[0]) {
>>>> -        struct stat s;
>>>> -        char *n = strchr(f, ':');
>>>> -        if (n)
>>>> -            *n = '\0';
>>>> -        if(stat(f,&s)<  0) {
>>>> -            fprintf(stderr, "Can't stat file '%s': %s\n", f,
>>>> strerror(errno));
>>>> -            goto out;
>>>> +        val = strtol(buf,&p, 0);
>>>> +        if (*p) {
>>>> +            fprintf(stderr, "acpitable: \"%s=%s\" is invalid\n",
>>>> +                    "asl_compiler_rev", buf);
>>>> +            return -1;
>>>>          }
>>>> -        length += s.st_size;
>>>> -        if (!n)
>>>> -            break;
>>>> -        *n = ':';
>>>> -        f = n + 1;
>>>> +        hdr.asl_compiler_revision = cpu_to_le32(val);
>>>> +        ++changed;
>>>>      }
>>>>
>>>> -    if (!acpi_tables) {
>>>> -        acpi_tables_len = sizeof(uint16_t);
>>>> -        acpi_tables = qemu_mallocz(acpi_tables_len);
>>>> +    if (!has_header&&  !changed) {
>>>> +        fprintf(stderr, "warning: acpitable: no table headers are
>>>> specified\n");
>>>>      }
>>>> -    acpi_tables = qemu_realloc(acpi_tables,
>>>> -                               acpi_tables_len + sizeof(uint16_t) +
>>>> length);
>>>> -    p = acpi_tables + acpi_tables_len;
>>>> -    acpi_tables_len += sizeof(uint16_t) + length;
>>>> -
>>>> -    *(uint16_t*)p = cpu_to_le32(length);
>>>> -    p += sizeof(uint16_t);
>>>> -    memcpy(p,&acpi_hdr, sizeof(acpi_hdr));
>>>> -    off = sizeof(acpi_hdr);
>>>> -
>>>> -    f = buf;
>>>> -    while (buf[0]) {
>>>> -        struct stat s;
>>>> -        int fd;
>>>> -        char *n = strchr(f, ':');
>>>> -        if (n)
>>>> -            *n = '\0';
>>>> -        fd = open(f, O_RDONLY);
>>>> -
>>>> -        if(fd<  0)
>>>> -            goto out;
>>>> -        if(fstat(fd,&s)<  0) {
>>>> -            close(fd);
>>>> -            goto out;
>>>> -        }
>>>>
>>>> -        /* off<  length is necessary because file size can be changed
>>>> -           under our foot */
>>>> -        while(s.st_size&&  off<  length) {
>>>> -            int r;
>>>> -            r = read(fd, p + off, s.st_size);
>>>> -            if (r>  0) {
>>>> -                off += r;
>>>> -                s.st_size -= r;
>>>> -            } else if ((r<  0&&  errno != EINTR) || r == 0) {
>>>> -                close(fd);
>>>> -                goto out;
>>>> -            }
>>>> -        }
>>>>
>>>> -        close(fd);
>>>> -        if (!n)
>>>> -            break;
>>>> -        f = n + 1;
>>>> -    }
>>>> -    if (off<  length) {
>>>> -        /* don't pass random value in process to guest */
>>>> -        memset(p + off, 0, length - off);
>>>> +    /* now calculate checksum of the table, complete with the header */
>>>> +    /* 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
>>>> */
>>>> +    hdr.checksum = 0;    /* for checksum calculation */
>>>> +
>>>> +    /* put header back */
>>>> +    memcpy(f,&hdr, sizeof(hdr));
>>>> +
>>>> +    if (changed || !has_header || 1) {
>>>> +        ((struct acpi_table_header *)f)->checksum =
>>>> +            acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len);
>>>>      }
>>>>
>>>> -    acpi_hdr_p = (struct acpi_table_header*)p;
>>>> -    acpi_hdr_p->length = cpu_to_le32(length);
>>>> -    acpi_hdr_p->checksum = acpi_checksum((uint8_t*)p, length);
>>>>      /* 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_le32(le32_to_cpu(*(uint16_t *)acpi_tables) + 1);
>>>> +
>>>> +    acpi_tables_len = allen;
>>>>      return 0;
>>>> -out:
>>>> -    if (acpi_tables) {
>>>> -        qemu_free(acpi_tables);
>>>> -        acpi_tables = NULL;
>>>> -    }
>>>> -    return -1;
>>>> +
>>>>  }
>>>>
>>>>  /* ACPI PM1a EVT */
>>>> diff --git a/qemu-options.hx b/qemu-options.hx
>>>> index 82e085a..e639d5a 100644
>>>> --- a/qemu-options.hx
>>>> +++ b/qemu-options.hx
>>>> @@ -1014,12 +1014,17 @@ Enable virtio balloon device (default),
>>>> optionally with PCI address
>>>>  ETEXI
>>>>
>>>>  DEF("acpitable", HAS_ARG, QEMU_OPTION_acpitable,
>>>> -    "-acpitable
>>>> [sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compiler_id=str][,asl_compiler_rev=n][,data=file1[:file2]...]\n"
>>>> +    "-acpitable
>>>> [sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compiler_id=str][,asl_compiler_rev=n][,{data|file}=file1[:file2]...]\n"
>>>>      "                ACPI table description\n", QEMU_ARCH_I386)
>>>>  STEXI
>>>> address@hidden -acpitable
>>>> address@hidden,address@hidden,address@hidden,address@hidden,address@hidden
>>>> [,address@hidden,address@hidden,address@hidden:@var{file2}]...]
>>>> address@hidden -acpitable
>>>>  Add ACPI table with specified header fields and context from specified
>>>> files.
>>>> +For file=, take whole ACPI table from the specified files, including
>>>> all
>>>> +ACPI headers (possible overridden by other options).
>>>> +For data=, only data
>>>> +portion of the table is used, all header information is specified in
>>>> the
>>>> +command line.
>>>>  ETEXI
>>>>
>>>>  DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
>>>
>>>
>
>