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

[John Baboval <john.baboval@virtualcomputer.com>]

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.

-John

On 06/15/2011 02:19 PM, Blue Swirl wrote:
> On Sat, Jun 11, 2011 at 1:29 AM, Michael Tokarev<mjt@tls.msk.ru>  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<mjt@tls.msk.ru>
>>> ---
>>>   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
>>>   @item -acpitable [sig=@var{str}][,rev=@var{n}][,oem_id=@var{str}][,oem_table_id=@var{str}][,oem_rev=@var{n}] [,asl_compiler_id=@var{str}][,asl_compiler_rev=@var{n}][,data=@var{file1}[:@var{file2}]...]
>>>   @findex -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,
>>
>>