diff mbox

[v2,12/14] cmdline: convert -acpitable to QemuOpts

Message ID 1334180081-6172-13-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini April 11, 2012, 9:34 p.m. UTC
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch_init.c   |   11 ++----
 arch_init.h   |    1 -
 hw/acpi.c     |   96 +++++++++++++++++++++++----------------------------------
 hw/pc.h       |    2 +-
 qemu-config.c |   54 ++++++++++++++++++++++++++++++++
 vl.c          |    9 +++++-
 6 files changed, 106 insertions(+), 67 deletions(-)

Comments

Anthony Liguori April 25, 2012, 9:03 p.m. UTC | #1
On 04/11/2012 04:34 PM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   arch_init.c   |   11 ++----
>   arch_init.h   |    1 -
>   hw/acpi.c     |   96 +++++++++++++++++++++++----------------------------------
>   hw/pc.h       |    2 +-
>   qemu-config.c |   54 ++++++++++++++++++++++++++++++++
>   vl.c          |    9 +++++-
>   6 files changed, 106 insertions(+), 67 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 595badf..9a081cf 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -672,15 +672,12 @@ int qemu_uuid_parse(const char *str, uint8_t *uuid)
>       return 0;
>   }
>
> -void do_acpitable_option(const char *optarg)
> +#ifndef TARGET_I386
> +int acpi_table_add(QemuOpts *opts, void *opaque)
>   {
> -#ifdef TARGET_I386
> -    if (acpi_table_add(optarg)<  0) {
> -        fprintf(stderr, "Wrong acpi table provided\n");
> -        exit(1);
> -    }
> -#endif
> +    abort();
>   }
> +#endif
>
>   void do_smbios_option(const char *optarg)
>   {
> diff --git a/arch_init.h b/arch_init.h
> index 828256c..82df01f 100644
> --- a/arch_init.h
> +++ b/arch_init.h
> @@ -23,7 +23,6 @@ enum {
>   extern const uint32_t arch_type;
>
>   void select_soundhw(const char *optarg);
> -void do_acpitable_option(const char *optarg);
>   void do_smbios_option(const char *optarg);
>   void cpudef_init(void);
>   int audio_available(void);
> diff --git a/hw/acpi.c b/hw/acpi.c
> index 5d521e5..1e8ab2b 100644
> --- a/hw/acpi.c
> +++ b/hw/acpi.c
> @@ -60,23 +60,11 @@ 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)
> +int acpi_table_add(QemuOpts *opts, void *opaque)
>   {
> -    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)
> -{
> -    char buf[1024], *p, *f;
> -    unsigned long val;
> +    const char *p = NULL;
> +    char *buf, *f;
> +    uint64_t val;
>       size_t len, start, allen;
>       bool has_header;

GCC (rightly) complains that this variable is used uninitialized after this patch.

I looked at the logic and it's a bit confusing.  It seems like either data or 
file must be set...

>       int changed;
> @@ -84,21 +72,13 @@ int acpi_table_add(const char *t)
>       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';
> -        /* fallthrough for default behavior */
> -    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 (qemu_opt_get(opts, "data") || qemu_opt_get(opts, "file")) {
> +        if (qemu_opt_get(opts, "data")&&  qemu_opt_get(opts, "file")) {
> +            fprintf(stderr, "acpitable: both data and file are specified\n");
> +            return -1;
> +        }
> +        has_header = qemu_opt_get(opts, "file") != NULL;
> +        p = qemu_opt_get(opts, has_header ? "file" : "data");
>       }

So I think you need an 'else' clause here with an fprintf().  That would 
probably make GCC happy.  I don't think defaulting has_header to false makes 
much sense here because I think if the else clause is taken, the result would be 
garbage.

Regards,

Anthony Liguori
diff mbox

Patch

diff --git a/arch_init.c b/arch_init.c
index 595badf..9a081cf 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -672,15 +672,12 @@  int qemu_uuid_parse(const char *str, uint8_t *uuid)
     return 0;
 }
 
-void do_acpitable_option(const char *optarg)
+#ifndef TARGET_I386
+int acpi_table_add(QemuOpts *opts, void *opaque)
 {
-#ifdef TARGET_I386
-    if (acpi_table_add(optarg) < 0) {
-        fprintf(stderr, "Wrong acpi table provided\n");
-        exit(1);
-    }
-#endif
+    abort();
 }
+#endif
 
 void do_smbios_option(const char *optarg)
 {
diff --git a/arch_init.h b/arch_init.h
index 828256c..82df01f 100644
--- a/arch_init.h
+++ b/arch_init.h
@@ -23,7 +23,6 @@  enum {
 extern const uint32_t arch_type;
 
 void select_soundhw(const char *optarg);
-void do_acpitable_option(const char *optarg);
 void do_smbios_option(const char *optarg);
 void cpudef_init(void);
 int audio_available(void);
diff --git a/hw/acpi.c b/hw/acpi.c
index 5d521e5..1e8ab2b 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -60,23 +60,11 @@  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)
+int acpi_table_add(QemuOpts *opts, void *opaque)
 {
-    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)
-{
-    char buf[1024], *p, *f;
-    unsigned long val;
+    const char *p = NULL;
+    char *buf, *f;
+    uint64_t val;
     size_t len, start, allen;
     bool has_header;
     int changed;
@@ -84,21 +72,13 @@  int acpi_table_add(const char *t)
     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';
-        /* fallthrough for default behavior */
-    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 (qemu_opt_get(opts, "data") || qemu_opt_get(opts, "file")) {
+        if (qemu_opt_get(opts, "data") && qemu_opt_get(opts, "file")) {
+            fprintf(stderr, "acpitable: both data and file are specified\n");
+            return -1;
+        }
+        has_header = qemu_opt_get(opts, "file") != NULL;
+        p = qemu_opt_get(opts, has_header ? "file" : "data");
     }
 
     if (!acpi_tables) {
@@ -114,6 +94,7 @@  int acpi_table_add(const char *t)
 
     /* now read in the data files, reallocating buffer as needed */
 
+    buf = g_strdup(p ? p : "");
     for (f = strtok(buf, ":"); f; f = strtok(NULL, ":")) {
         int fd = open(f, O_RDONLY);
 
@@ -141,6 +122,7 @@  int acpi_table_add(const char *t)
 
         close(fd);
     }
+    g_free(buf);
 
     /* now fill in the header fields */
 
@@ -155,8 +137,9 @@  int acpi_table_add(const char *t)
 
     hdr._length = cpu_to_le16(len);
 
-    if (get_param_value(buf, sizeof(buf), "sig", t)) {
-        strzcpy(hdr.sig, buf, sizeof(hdr.sig));
+    p = qemu_opt_get(opts, "sig");
+    if (p) {
+        strncpy(hdr.sig, p, sizeof(hdr.sig));
         ++changed;
     }
 
@@ -175,46 +158,48 @@  int acpi_table_add(const char *t)
     /* 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, 0);
-        if (val > 255 || *p) {
-            fprintf(stderr, "acpitable: \"rev=%s\" is invalid\n", buf);
+    if (qemu_opt_get(opts, "rev")) {
+        val = qemu_opt_get_number(opts, "rev", -1);
+        if (val > 255) {
+            fprintf(stderr, "acpitable: invalid revision\n");
             return -1;
         }
         hdr.revision = (uint8_t)val;
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "oem_id", t)) {
-        strzcpy(hdr.oem_id, buf, sizeof(hdr.oem_id));
+    p = qemu_opt_get(opts, "oem_id");
+    if (p) {
+        strncpy(hdr.oem_id, p, sizeof(hdr.oem_id));
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "oem_table_id", t)) {
-        strzcpy(hdr.oem_table_id, buf, sizeof(hdr.oem_table_id));
+    p = qemu_opt_get(opts, "oem_table_id");
+    if (p) {
+        strncpy(hdr.oem_table_id, p, sizeof(hdr.oem_table_id));
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "oem_rev", t)) {
-        val = strtol(buf, &p, 0);
-        if (*p) {
-            fprintf(stderr, "acpitable: \"oem_rev=%s\" is invalid\n", buf);
+    if (qemu_opt_get(opts, "oem_rev")) {
+        val = qemu_opt_get_number(opts, "oem_rev", -1);
+        if (val > 0xFFFFFFFFULL) {
+            fprintf(stderr, "acpitable: invalid oem_rev\n");
             return -1;
         }
         hdr.oem_revision = cpu_to_le32(val);
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "asl_compiler_id", t)) {
-        strzcpy(hdr.asl_compiler_id, buf, sizeof(hdr.asl_compiler_id));
+    p = qemu_opt_get(opts, "asl_compiler_id");
+    if (p) {
+        strncpy(hdr.asl_compiler_id, p, sizeof(hdr.asl_compiler_id));
         ++changed;
     }
 
-    if (get_param_value(buf, sizeof(buf), "asl_compiler_rev", t)) {
-        val = strtol(buf, &p, 0);
-        if (*p) {
-            fprintf(stderr, "acpitable: \"%s=%s\" is invalid\n",
-                    "asl_compiler_rev", buf);
+    if (qemu_opt_get(opts, "asl_compiler_rev")) {
+        val = qemu_opt_get_number(opts, "asl_compiler_rev", -1);
+        if (val > 0xFFFFFFFFULL) {
+            fprintf(stderr, "acpitable: invalid asl_compiler_rev\n");
             return -1;
         }
         hdr.asl_compiler_revision = cpu_to_le32(val);
@@ -233,11 +218,8 @@  int acpi_table_add(const char *t)
 
     /* 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);
-    }
+    ((struct acpi_table_header *)f)->checksum =
+        acpi_checksum((uint8_t *)f + ACPI_TABLE_PFX_SIZE, len);
 
     /* increase number of tables */
     (*(uint16_t *)acpi_tables) =
diff --git a/hw/pc.h b/hw/pc.h
index 74d3369..d302e60 100644
--- a/hw/pc.h
+++ b/hw/pc.h
@@ -136,7 +136,7 @@  extern char *acpi_tables;
 extern size_t acpi_tables_len;
 
 void acpi_bios_init(void);
-int acpi_table_add(const char *table_desc);
+int acpi_table_add(QemuOpts *opts, void *opaque);
 
 /* acpi_piix.c */
 
diff --git a/qemu-config.c b/qemu-config.c
index 6e97ff6..c197317 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -681,6 +681,59 @@  QemuOptsList qemu_boot_opts = {
     },
 };
 
+static QemuOptsList qemu_acpitable_opts = {
+    .name = "acpitable",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_acpitable_opts.head),
+    .desc = {
+        {
+            .name = "data",
+            .type = QEMU_OPT_STRING,
+            .help = "colon-separated list of files, excluding header",
+        },
+        {
+            .name = "file",
+            .type = QEMU_OPT_STRING,
+            .help = "colon-separated list of files, including header",
+        },
+        {
+            .name = "sig",
+            .type = QEMU_OPT_STRING,
+            .help = "ACPI header signature field",
+        },
+        {
+            .name = "rev",
+            .type = QEMU_OPT_NUMBER,
+            .help = "ACPI header revision field",
+        },
+        {
+            .name = "oem_id",
+            .type = QEMU_OPT_STRING,
+            .help = "ACPI header OEM id field",
+        },
+        {
+            .name = "oem_table_id",
+            .type = QEMU_OPT_STRING,
+            .help = "ACPI header OEM table id field",
+        },
+        {
+            .name = "oem_rev",
+            .type = QEMU_OPT_NUMBER,
+            .help = "ACPI header OEM revision field",
+        },
+        {
+            .name = "asl_compiler_id",
+            .type = QEMU_OPT_STRING,
+            .help = "ACPI header ASL compiler id field",
+        },
+        {
+            .name = "asl_compiler_rev",
+            .type = QEMU_OPT_NUMBER,
+            .help = "ACPI header ASL compiler revision field",
+        },
+        { /* end of list */ },
+    },
+};
+
 static QemuOptsList *vm_config_groups[32] = {
     &qemu_drive_opts,
     &qemu_chardev_opts,
@@ -698,6 +751,7 @@  static QemuOptsList *vm_config_groups[32] = {
     &qemu_numa_opts,
     &qemu_boot_opts,
     &qemu_iscsi_opts,
+    &qemu_acpitable_opts,
     NULL,
 };
 
diff --git a/vl.c b/vl.c
index b48850c..810d3c3 100644
--- a/vl.c
+++ b/vl.c
@@ -2930,7 +2930,7 @@  int main(int argc, char **argv, char **envp)
                 break;
             }
             case QEMU_OPTION_acpitable:
-                do_acpitable_option(optarg);
+                qemu_opts_parse(qemu_find_opts("acpitable"), optarg, 0);
                 break;
             case QEMU_OPTION_smbios:
                 do_smbios_option(optarg);
@@ -3199,6 +3199,13 @@  int main(int argc, char **argv, char **envp)
 
     current_machine = machine;
 
+    if (qemu_opts_foreach(qemu_find_opts("acpitable"), acpi_table_add, NULL, 1)
+        != 0) {
+        fprintf(stderr, "Wrong acpi table provided\n");
+        exit(1);
+    }
+
+
     /*
      * Default to max_cpus = smp_cpus, in case the user doesn't
      * specify a max_cpus value.