Re: [Qemu-arm] [PATCH v4 1/2] ACPI: Cleanup -acpitable option code

[Zheng, Lv]

Hi, 

> From: Zheng, Lv
> Subject: [PATCH v4 1/2] ACPI: Cleanup -acpitable option code
> 
> In -acpitable options, at least/most one data/file sub-option is mandatory,
> this patch cleans up the code to reflect this in a managed manner so that
> the follow-up mandatory sub-options can be added to -acpitable.
> 
> Signed-off-by: Lv Zheng <address@hidden>
> ---
>  hw/acpi/core.c   |   32 +++++++++++++++++++++++---------
>  qapi-schema.json |   27 ++++++++-------------------
>  qemu-options.hx  |    7 +++++--
>  3 files changed, 36 insertions(+), 30 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index e890a5d..cd1f9e4 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -89,8 +89,6 @@ static int acpi_checksum(const uint8_t *data, int len)
>   * It is valid to call this function with
>   * (@blob == NULL && bloblen == 0 && !has_header).
>   *
> - * @hdrs->file and @hdrs->data are ignored.
> - *
>   * SIZE_MAX is considered "infinity" in this function.
>   *
>   * The number of tables that can be installed is not limited, but the 16-bit
> @@ -229,7 +227,8 @@ static void acpi_table_install(const char unsigned
> *blob, size_t bloblen,
>                                        ACPI_TABLE_PFX_SIZE, 
> acpi_payload_size);
>  }
> 
> -void acpi_table_add(const QemuOpts *opts, Error **errp)
> +static void acpi_table_from_file(bool has_header, const char *file,
> +                                 AcpiTableOptions *hdrs, Error **errp)
[Lv Zheng] 
It looks this patch still has problem here.
Please ignore the v3/v4 submissions.
Sorry for the noise.

Thanks
Lv

>  {
>      AcpiTableOptions *hdrs = NULL;
>      Error *err = NULL;
> @@ -249,12 +248,8 @@ void acpi_table_add(const QemuOpts *opts,
> Error **errp)
>      if (err) {
>          goto out;
>      }
> -    if (hdrs->has_file == hdrs->has_data) {
> -        error_setg(&err, "'-acpitable' requires one of 'data' or 'file'");
> -        goto out;
> -    }
> 
> -    pathnames = g_strsplit(hdrs->has_file ? hdrs->file : hdrs->data, ":", 0);
> +    pathnames = g_strsplit(file, ":", 0);
>      if (pathnames == NULL || pathnames[0] == NULL) {
>          error_setg(&err, "'-acpitable' requires at least one pathname");
>          goto out;
> @@ -291,7 +286,7 @@ void acpi_table_add(const QemuOpts *opts, Error
> **errp)
>          close(fd);
>      }
> 
> -    acpi_table_install(blob, bloblen, hdrs->has_file, hdrs, &err);
> +    acpi_table_install(blob, bloblen, has_header, hdrs, &err);
> 
>  out:
>      g_free(blob);
> @@ -301,6 +296,25 @@ out:
>      error_propagate(errp, err);
>  }
> 
> +void acpi_table_add(const QemuOpts *opts, Error **errp)
> +{
> +    const char *val;
> +
> +    val = qemu_opt_get((QemuOpts *)opts, "file");
> +    if (val) {
> +        acpi_table_from_file(true, val, hdrs, errp);
> +        return;
> +    }
> +
> +    val = qemu_opt_get((QemuOpts *)opts, "data");
> +    if (val) {
> +        acpi_table_from_file(false, val, hdrs, errp);
> +        return;
> +    }
> +
> +    error_setg(errp, "'-acpitable' requires one of 'data' or 'file'");
> +}
> +
>  static bool acpi_table_builtin = false;
> 
>  void acpi_table_add_builtin(const QemuOpts *opts, Error **errp)
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5658723..a5e219f 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3597,17 +3597,18 @@
>  ##
>  # @AcpiTableOptions
>  #
> -# Specify an ACPI table on the command line to load.
> +# Specify ACPI table options for the table loaded on the command line.
>  #
> -# At most one of @file and @data can be specified. The list of files
> specified
> -# by any one of them is loaded and concatenated in order. If both are
> omitted,
> -# @data is implied.
> +# ACPI table can be loaded via 'file' and 'data' options. At most one of
> +# 'file' and 'data' can be specified. The list of files specified by any one
> +# of them is loaded and concatenated in order. If both # are omitted,
> 'data'
> +# is implied.
>  #
>  # Other fields / optargs can be used to override fields of the generic ACPI
>  # table header; refer to the ACPI specification 5.0, section 5.2.6 System
>  # Description Table Header. If a header field is not overridden, then the
> -# corresponding value from the concatenated blob is used (in case of
> @file), or
> -# it is filled in with a hard-coded value (in case of @data).
> +# corresponding value from the concatenated blob is used (in case of
> 'file'),
> +# or it is filled in with a hard-coded value (in case of 'data').
>  #
>  # String fields are copied into the matching ACPI member from lowest
> address
>  # upwards, and silently truncated / NUL-padded to length.
> @@ -3628,16 +3629,6 @@
>  # @asl_compiler_rev: #optional revision number of the utility that
> created the
>  #                    table (4 bytes)
>  #
> -# @file: #optional colon (:) separated list of pathnames to load and
> -#        concatenate as table data. The resultant binary blob is expected to
> -#        have an ACPI table header. At least one file is required. This field
> -#        excludes @data.
> -#
> -# @data: #optional colon (:) separated list of pathnames to load and
> -#        concatenate as table data. The resultant binary blob must not have
> an
> -#        ACPI table header. At least one file is required. This field 
> excludes
> -#        @file.
> -#
>  # Since 1.5
>  ##
>  { 'struct': 'AcpiTableOptions',
> @@ -3648,9 +3639,7 @@
>      '*oem_table_id':      'str',
>      '*oem_rev':           'uint32',
>      '*asl_compiler_id':   'str',
> -    '*asl_compiler_rev':  'uint32',
> -    '*file':              'str',
> -    '*data':              'str' }}
> +    '*asl_compiler_rev':  'uint32' }}
> 
>  ##
>  # @CommandLineParameterType:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index a71aaf8..5fe7f87 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1493,10 +1493,13 @@ Disable HPET support.
>  ETEXI
> 
>  DEF("acpitable", HAS_ARG, QEMU_OPTION_acpitable,
> -    "-acpitable
> [sig=str][,rev=n][,oem_id=str][,oem_table_id=str][,oem_rev=n][,asl_compi
> ler_id=str][,asl_compiler_rev=n][,{data|file}=file1[:file2]...]\n"
> +    "-acpitable {data|file}=file1[:file2]...\n"
> +    "             [,sig=str][,rev=n]\n"
> +    "             [,oem_id=str][,oem_table_id=str][,oem_rev=n]\n"
> +    "             [,asl_compiler_id=str][,asl_compiler_rev=n]\n"
>      "                ACPI table description\n", QEMU_ARCH_I386)
>  STEXI
> address@hidden -acpitable
> address@hidden,address@hidden,address@hidden,address@hidden
> r}][,address@hidden
> [,address@hidden,address@hidden,address@hidden
> }[:@var{file2}]...]
> address@hidden -acpitable
> address@hidden:@var{file2}]...[,address@hidden,address@hidden,oem_id=@
> var{str}][,address@hidden,address@hidden
> [,address@hidden,address@hidden
>  @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
> --
> 1.7.10