Re: [Qemu-devel] [PATCH v2] update names in option tables to match with actual command-line spelling

[Markus Armbruster <armbru@redhat.com>]

[cc: Eric]

Amos Kong <akong@redhat.com> writes:

> This patch makes all names in option table to match with actual
> command-line spelling, it will be helpful when we search in option
> tables.

As discussed in [*], the QemuOptsList member name values are ABI:
changing them can break existing -readconfig configuration files.  If we
decide breaking ABI is okay here (big if!), we need to document it
prominently in the commit message.

> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
> V2: fix name in acpi option table
> ---
>  hw/acpi/core.c        |  8 ++++----
>  hw/nvram/fw_cfg.c     |  4 ++--
>  include/qemu/option.h |  2 +-
>  vl.c                  | 14 +++++++-------
>  4 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 79414b4..12e9ae8 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -54,16 +54,16 @@ static const char unsigned dfl_hdr[ACPI_TABLE_HDR_SIZE - ACPI_TABLE_PFX_SIZE] =
>  char unsigned *acpi_tables;
>  size_t acpi_tables_len;
>  
> -static QemuOptsList qemu_acpi_opts = {
> -    .name = "acpi",
> +static QemuOptsList qemu_acpitable_opts = {
> +    .name = "acpitable",
>      .implied_opt_name = "data",
> -    .head = QTAILQ_HEAD_INITIALIZER(qemu_acpi_opts.head),
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_acpitable_opts.head),
>      .desc = { { 0 } } /* validated with OptsVisitor */
>  };
>  
>  static void acpi_register_config(void)
>  {
> -    qemu_add_opts(&qemu_acpi_opts);
> +    qemu_add_opts(&qemu_acpitable_opts);
>  }
>  
>  machine_init(acpi_register_config);
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index cb36dc2..1c75507 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -125,7 +125,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>      const char *temp;
>  
>      /* get user configuration */
> -    QemuOptsList *plist = qemu_find_opts("boot-opts");
> +    QemuOptsList *plist = qemu_find_opts("boot");
>      QemuOpts *opts = QTAILQ_FIRST(&plist->head);
>      if (opts != NULL) {
>          temp = qemu_opt_get(opts, "splash");
> @@ -191,7 +191,7 @@ static void fw_cfg_reboot(FWCfgState *s)
>      const char *temp;
>  
>      /* get user configuration */
> -    QemuOptsList *plist = qemu_find_opts("boot-opts");
> +    QemuOptsList *plist = qemu_find_opts("boot");
>      QemuOpts *opts = QTAILQ_FIRST(&plist->head);
>      if (opts != NULL) {
>          temp = qemu_opt_get(opts, "reboot-timeout");
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 8c0ac34..96b7c29 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -102,7 +102,7 @@ typedef struct QemuOptDesc {
>  } QemuOptDesc;
>  
>  struct QemuOptsList {
> -    const char *name;
> +    const char *name;  /* option name */
>      const char *implied_opt_name;
>      bool merge_lists;  /* Merge multiple uses of option into a single list? */
>      QTAILQ_HEAD(, QemuOpts) head;

Your patch makes the code adhere to an convention "QemuOptsList name
must match the name of the (non-sugared) command line option using it".
Apart from the comment you add here, it's an unspoken convention.

Making such a convention stick usually takes a tests that fail when it's
violated.

> diff --git a/vl.c b/vl.c
> index 685a7a9..2bcf5fe 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -380,7 +380,7 @@ static QemuOptsList qemu_machine_opts = {
>  };
>  
>  static QemuOptsList qemu_boot_opts = {
> -    .name = "boot-opts",
> +    .name = "boot",
>      .implied_opt_name = "order",
>      .merge_lists = true,
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_boot_opts.head),
> @@ -1304,7 +1304,7 @@ static void numa_add(const char *optarg)
>  }
>  
>  static QemuOptsList qemu_smp_opts = {
> -    .name = "smp-opts",
> +    .name = "smp",
>      .implied_opt_name = "cpus",
>      .merge_lists = true,
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_smp_opts.head),
> @@ -3124,7 +3124,7 @@ int main(int argc, char **argv, char **envp)
>                  drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
>                  break;
>              case QEMU_OPTION_boot:
> -                opts = qemu_opts_parse(qemu_find_opts("boot-opts"), optarg, 1);
> +                opts = qemu_opts_parse(qemu_find_opts("boot"), optarg, 1);
>                  if (!opts) {
>                      exit(1);
>                  }
> @@ -3493,7 +3493,7 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              }
>              case QEMU_OPTION_acpitable:
> -                opts = qemu_opts_parse(qemu_find_opts("acpi"), optarg, 1);
> +                opts = qemu_opts_parse(qemu_find_opts("acpitable"), optarg, 1);
>                  if (!opts) {
>                      exit(1);
>                  }
> @@ -3560,7 +3560,7 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_smp:
> -                if (!qemu_opts_parse(qemu_find_opts("smp-opts"), optarg, 1)) {
> +                if (!qemu_opts_parse(qemu_find_opts("smp"), optarg, 1)) {
>                      exit(1);
>                  }
>                  break;
> @@ -3896,7 +3896,7 @@ int main(int argc, char **argv, char **envp)
>          data_dir[data_dir_idx++] = CONFIG_QEMU_DATADIR;
>      }
>  
> -    smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL));
> +    smp_parse(qemu_opts_find(qemu_find_opts("smp"), NULL));
>  
>      machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */
>      if (smp_cpus > machine->max_cpus) {
> @@ -4072,7 +4072,7 @@ int main(int argc, char **argv, char **envp)
>      bios_name = qemu_opt_get(machine_opts, "firmware");
>  
>      boot_order = machine->default_boot_order;
> -    opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
> +    opts = qemu_opts_find(qemu_find_opts("boot"), NULL);
>      if (opts) {
>          char *normal_boot_order;
>          const char *order, *once;

[*] https://lists.nongnu.org/archive/html/qemu-devel/2014-03/msg01373.html