[libvirt] [PATCH v3 for 2.0] update names in option tables to match with actual command-line spelling
We want to establish a mapping between option name and option table, then we can search related option table by option name. This patch makes all the member name of QemuOptsList to match with actual command-line spelling(option name). [ Important Note ] The QemuOptsList member name values are ABI, changing them can break existing -readconfig configuration files. 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 have updated the release note of 2.0 http://wiki.qemu.org/ChangeLog/2.0#ABI_breaking Signed-off-by: Amos Kong <akong@redhat.com> --- V2: fix name in acpi option table V3: mention ABI break in commitlog (Markus) --- 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 282341a..3e6b048 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 02bf8ec..c8002ea 100644 --- a/vl.c +++ b/vl.c @@ -388,7 +388,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), @@ -1357,7 +1357,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), @@ -3245,7 +3245,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); } @@ -3614,7 +3614,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); } @@ -3681,7 +3681,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; @@ -4006,7 +4006,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) { @@ -4190,7 +4190,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; -- 1.8.5.3
On 03/20/2014 07:07 AM, Amos Kong wrote:
We want to establish a mapping between option name and option table, then we can search related option table by option name.
This patch makes all the member name of QemuOptsList to match with actual command-line spelling(option name).
[ Important Note ]
The QemuOptsList member name values are ABI, changing them can break existing -readconfig configuration files.
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 have updated the release note of 2.0 http://wiki.qemu.org/ChangeLog/2.0#ABI_breaking
Signed-off-by: Amos Kong <akong@redhat.com> ---
The benefit of this patch is that 'query-command-line-options' gains a fix where three bogus entries are replaced by their actual command line spelling. The drawback is that anyone that doesn't pay attention to the ABI break announcement, and expects -readconfig and friends to work while using the old spelling, is in for a surprise. But since we have prominently documented the change, and since consistency makes life nicer, I'm in favor of this patch. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> writes:
On 03/20/2014 07:07 AM, Amos Kong wrote:
We want to establish a mapping between option name and option table, then we can search related option table by option name.
This patch makes all the member name of QemuOptsList to match with actual command-line spelling(option name).
[ Important Note ]
The QemuOptsList member name values are ABI, changing them can break existing -readconfig configuration files.
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 have updated the release note of 2.0 http://wiki.qemu.org/ChangeLog/2.0#ABI_breaking
Signed-off-by: Amos Kong <akong@redhat.com> ---
The benefit of this patch is that 'query-command-line-options' gains a fix where three bogus entries are replaced by their actual command line spelling. The drawback is that anyone that doesn't pay attention to the ABI break announcement, and expects -readconfig and friends to work while using the old spelling, is in for a surprise. But since we have prominently documented the change, and since consistency makes life nicer, I'm in favor of this patch.
Reviewed-by: Eric Blake <eblake@redhat.com>
I'm not thrilled about the ABI break, but avoiding it would probably take too much code for too little gain. How can we prevent future violations of the convention "QemuOptsList member name matches the name of the (primary) option using it for its argument"? Right now, all we have is /* option name */ tacked to the member. Which is at best a reminder for those who already know. I'd ask for a test catching violations if I could think of an easy way to code it.
On Wed, Mar 26, 2014 at 05:12:08PM +0100, Markus Armbruster wrote:
Eric Blake <eblake@redhat.com> writes:
On 03/20/2014 07:07 AM, Amos Kong wrote:
We want to establish a mapping between option name and option table, then we can search related option table by option name.
This patch makes all the member name of QemuOptsList to match with actual command-line spelling(option name).
[ Important Note ]
The QemuOptsList member name values are ABI, changing them can break existing -readconfig configuration files.
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 have updated the release note of 2.0 http://wiki.qemu.org/ChangeLog/2.0#ABI_breaking
Signed-off-by: Amos Kong <akong@redhat.com> ---
The benefit of this patch is that 'query-command-line-options' gains a fix where three bogus entries are replaced by their actual command line spelling. The drawback is that anyone that doesn't pay attention to the ABI break announcement, and expects -readconfig and friends to work while using the old spelling, is in for a surprise. But since we have prominently documented the change, and since consistency makes life nicer, I'm in favor of this patch.
Reviewed-by: Eric Blake <eblake@redhat.com>
I'm not thrilled about the ABI break, but avoiding it would probably take too much code for too little gain.
We can add an 'alias_index' field to QemuOptsList, and init it to -1 in qemu_add_opts(). And we only re-set 'alias_index' to 'popt->index' in vl.c(option parsing part) then we can find opts by qemu_options[i].index. We also need to give a warning () if it's group name of added QemuOptsList doesn't match with defined option name. If someone add a new QemuOpts and the group name doesn't match, he/she will get a warning, and call a help function to re-set 'alias_index'. I can send a RFC patch for this.
How can we prevent future violations of the convention "QemuOptsList member name matches the name of the (primary) option using it for its argument"? Right now, all we have is /* option name */ tacked to the member. Which is at best a reminder for those who already know.
It's absolutely necessary!
I'd ask for a test catching violations if I could think of an easy way to code it.
Currently I prefer this option, so I will send a V3 with strict checking. -- Amos.
On Thu, Mar 27, 2014 at 10:16:44AM +0800, Amos Kong wrote:
On Wed, Mar 26, 2014 at 05:12:08PM +0100, Markus Armbruster wrote:
Eric Blake <eblake@redhat.com> writes:
...
Reviewed-by: Eric Blake <eblake@redhat.com>
I'm not thrilled about the ABI break, but avoiding it would probably take too much code for too little gain.
Hi Markus, Eric
We can add an 'alias_index' field to QemuOptsList, and init it to -1 in qemu_add_opts().
And we only re-set 'alias_index' to 'popt->index' in vl.c(option parsing part) then we can find opts by qemu_options[i].index.
We also need to give a warning () if it's group name of added QemuOptsList doesn't match with defined option name.
If someone add a new QemuOpts and the group name doesn't match, he/she will get a warning, and call a help function to re-set 'alias_index'.
I can send a RFC patch for this.
* Option 1 Attached two RFC patches, it added a new field ('alias_index') to QemuOptsList, and record QEMU_OPTION enum-index to it when group name of option table doesn't match option name. And try to find list by index before find list by option name in qmp_query_command_line_options(). This can avoid the ABI breaking, it also introduced some trouble. We also need to check if alias_index of unmatched option is set to a positive number, and report error (option name doesn't match, and alias_index isn't set) in error state. * Option 2 OR pass enum-index to qemu_add_opts(), and set enum-index for all the options. -void qemu_add_opts(QemuOptsList *list) +void qemu_add_opts(QemuOptsList *list, int index) * Option 3 break ABI Let's make a decision.
How can we prevent future violations of the convention "QemuOptsList member name matches the name of the (primary) option using it for its argument"? Right now, all we have is /* option name */ tacked to the member. Which is at best a reminder for those who already know.
It's absolutely necessary!
I'd ask for a test catching violations if I could think of an easy way to code it.
Currently I prefer this option, so I will send a V3 with strict checking.
-- Amos.
participants (3)
-
Amos Kong -
Eric Blake -
Markus Armbruster