Re: [Qemu-devel] [PATCH v2 12/14] cmdline: convert -acpitable to QemuOpts

[Anthony Liguori]

On 04/11/2012 04:34 PM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini<address@hidden>
> ---
>   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