| Message ID | 1395033894-21613-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
[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
Il 17/03/2014 09:23, Markus Armbruster ha scritto: > > 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. I think in some (rare) cases breaking the rule is okay. For example, the pending conversion of "-m" to QemuOpts uses "memory". However, I don't think adding "-opts" is a good thing to do. Which one is most readable? [m] size = 128M max = 512M [memory] size = 128M max = 512M [memory-opts] size = 128M max = 512M I'm for including this patch in 2.0. Paolo
[cc: Eric yet again :)] Paolo Bonzini <pbonzini@redhat.com> writes: > Il 17/03/2014 09:23, Markus Armbruster ha scritto: >> > 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. > > I think in some (rare) cases breaking the rule is okay. For example, > the pending conversion of "-m" to QemuOpts uses "memory". Breaking naming consistency rules is one thing, breaking ABI an entirely different one. While I'm not overly scared of the ABI change bugaboo, I do insist on considering the implications, and on proper documentation. > However, I don't think adding "-opts" is a good thing to do. Which > one is most readable? > > [m] > size = 128M > max = 512M > > [memory] > size = 128M > max = 512M > > [memory-opts] > size = 128M > max = 512M That's a no-brainer :) I'm all for naming configuration file sections sensibly. While I value consistency between section names and command line options, I agree with you that having sensible section names trumps consistency with the command line. In this case, consistency with command line could easily be preserved by making -m sugar for --memory. I doubt changing existing section names to make them prettier or more consistent with the command is worth the ABI breakage. Any inconsistent or ugly names that haven't been released yet should be fixed right away, of course. This patch changes: from to introduced in acpi acpitable 0c764a9 v1.5.0 boot-opts boot 3d3b830 v1.0 smp-opts smp 12b7f57 v1.6.0 All three have calcified into ABI already. > I'm for including this patch in 2.0. Not without explaining the ABI breakage in the commit message. We should also make sure to cover it in the release notes[*]. Moreover, if exceptions from the rule "QemuOptsList name must match the name of the (non-sugared) command line option using it" are or will be permissible, then Amos's comment on QemuOptsList member name needs to be clarified. [*] Release note material is being collected at http://wiki.qemu.org/ChangeLog/2.0
Il 17/03/2014 13:33, Markus Armbruster ha scritto: > This patch changes: > > from to introduced in > acpi acpitable 0c764a9 v1.5.0 > boot-opts boot 3d3b830 v1.0 > smp-opts smp 12b7f57 v1.6.0 > > All three have calcified into ABI already. What has calcified into ABI is only what has likely been used as ABI. The other is an ABI that won't be nice to break, but it is not calcified yet. There are two aspects of this: 1) QMP queries. Did any of the above conversions introduce new sub-options? Or was any suboption introduced after that point? If not, there's hardly a reason for anyone to query for acpi/boot-opts/smp-opts QemuOpts 2) readconfig. I think readconfig is not practically usable until at least memory will be part of it. For this reason, -readconfig is not a worry; not yet at least: it will be in 2.1 once -m is converted. >> > I'm for including this patch in 2.0. > Not without explaining the ABI breakage in the commit message. We > should also make sure to cover it in the release notes[*]. I agree. > Moreover, if exceptions from the rule "QemuOptsList name must match the > name of the (non-sugared) command line option using it" are or will be > permissible, then Amos's comment on QemuOptsList member name needs to be > clarified. I like your idea of -memory as a synonym for -m. Paolo
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; 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;
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. 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(-)