All of lore.kernel.org
 help / color / mirror / Atom feed
From: Blue Swirl <blauwirbel@gmail.com>
To: John Baboval <john.baboval@virtualcomputer.com>
Cc: Isaku Yamahata <yamahata@valinux.co.jp>,
	Michael Tokarev <mjt@tls.msk.ru>,
	qemu-devel@nongnu.org,
	Anthony Liguori <aliguori@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH] v6 revamp acpitable parsing and allow to specify complete (headerful) table
Date: Fri, 15 Jul 2011 19:51:43 +0300	[thread overview]
Message-ID: <CAAu8pHteXwLpDBEXTLpMFna3uK4j=bFhbcbJ+MtY9tdzqMLyYA@mail.gmail.com> (raw)
In-Reply-To: <4E205A57.2040309@virtualcomputer.com>

On Fri, Jul 15, 2011 at 6:18 PM, John Baboval
<john.baboval@virtualcomputer.com> 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<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,
>>>
>>>
>
>

  reply	other threads:[~2011-07-15 16:52 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-06-06  8:35 [Qemu-devel] [PATCH] v6 revamp acpitable parsing and allow to specify complete (headerful) table Michael Tokarev
2011-06-10 22:29 ` Michael Tokarev
2011-06-13  3:38   ` Isaku Yamahata
2011-06-15 18:19   ` Blue Swirl
2011-07-06 22:41     ` John Baboval
2011-07-15 15:18     ` John Baboval
2011-07-15 16:51       ` Blue Swirl [this message]
2011-07-29  1:49         ` Isaku Yamahata
2011-07-29  5:51           ` Michael Tokarev
2011-07-30  9:40           ` Blue Swirl
2011-07-30 16:37             ` John Baboval
2011-06-06  8:37 Michael Tokarev
2013-01-17  9:50 ` TeLeMan
2013-01-17 10:49   ` Michael Tokarev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAu8pHteXwLpDBEXTLpMFna3uK4j=bFhbcbJ+MtY9tdzqMLyYA@mail.gmail.com' \
    --to=blauwirbel@gmail.com \
    --cc=aliguori@linux.vnet.ibm.com \
    --cc=john.baboval@virtualcomputer.com \
    --cc=mjt@tls.msk.ru \
    --cc=qemu-devel@nongnu.org \
    --cc=yamahata@valinux.co.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.