[libvirt] [PATCH v3 for 2.0] update names in option tables to match with actual command-line spelling
8:07 a.m.
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(a)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
3:57 p.m.
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(a)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(a)redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:12 a.m.
New subject: [libvirt] [Qemu-devel] [PATCH v3 for 2.0] update names in option tables to match with actual command-line spelling
Eric Blake <eblake(a)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(a)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(a)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.
9:16 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH v3 for 2.0] update names in option tables to match with actual command-line spelling
On Wed, Mar 26, 2014 at 05:12:08PM +0100, Markus Armbruster wrote:
Eric Blake <eblake(a)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(a)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(a)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.
11:43 p.m.
New subject: [libvirt] [Qemu-devel] [PATCH v3 for 2.0] update names in option tables to match with actual command-line spelling
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(a)redhat.com> writes:
...
> > Reviewed-by: Eric Blake <eblake(a)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.
3927
days inactive
3934
days old
4 comments
3 participants
participants (3)
-
Amos Kong -
Eric Blake -
Markus Armbruster