From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59287) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QhkAK-0000ff-QM for qemu-devel@nongnu.org; Fri, 15 Jul 2011 11:19:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QhkAF-0006wS-RC for qemu-devel@nongnu.org; Fri, 15 Jul 2011 11:18:56 -0400 Received: from server514d.exghost.com ([72.32.253.69]:3421 helo=server514.appriver.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QhkAF-0006wB-7K for qemu-devel@nongnu.org; Fri, 15 Jul 2011 11:18:51 -0400 Message-ID: <4E205A57.2040309@virtualcomputer.com> Date: Fri, 15 Jul 2011 11:18:47 -0400 From: John Baboval MIME-Version: 1.0 References: <20110606083447.47A88527A@gandalf.tls.msk.ru> <4DF29AD2.2@msgid.tls.msk.ru> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] v6 revamp acpitable parsing and allow to specify complete (headerful) table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: Isaku Yamahata , Michael Tokarev , qemu-devel@nongnu.org, Anthony Liguori 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 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 >>> --- >>> 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, >> >>