[libvirt] [PATCH v5 for 2.0 0/3] ABI change: change group name of option table to match with option name
This patchset changes group names of option tables to match with option name, this breakes ABI, release note was updated. V4: fix tpmdev, add name matching test (markus) V5: adjust patch order (paolo) Amos Kong (3): only add qemu_tpmdev_opts when CONFIG_TPM is defined update names in option tables to match with actual command-line spelling abort QEMU if group name in option table doesn't match with defined option name hw/acpi/core.c | 8 ++++---- hw/nvram/fw_cfg.c | 4 ++-- include/qemu/option.h | 2 +- qemu-options.h | 12 ++++++++++++ util/qemu-config.c | 28 ++++++++++++++++++++++++++++ vl.c | 37 +++++++++++++------------------------ 6 files changed, 60 insertions(+), 31 deletions(-) -- 1.8.5.3
Signed-off-by: Amos Kong <akong@redhat.com> --- vl.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/vl.c b/vl.c index 2355227..596ecfa 100644 --- a/vl.c +++ b/vl.c @@ -449,6 +449,7 @@ static QemuOptsList qemu_object_opts = { }, }; +#ifdef CONFIG_TPM static QemuOptsList qemu_tpmdev_opts = { .name = "tpmdev", .implied_opt_name = "type", @@ -458,6 +459,7 @@ static QemuOptsList qemu_tpmdev_opts = { { /* end of list */ } }, }; +#endif static QemuOptsList qemu_realtime_opts = { .name = "realtime", @@ -2992,7 +2994,9 @@ int main(int argc, char **argv, char **envp) qemu_add_opts(&qemu_sandbox_opts); qemu_add_opts(&qemu_add_fd_opts); qemu_add_opts(&qemu_object_opts); +#ifdef CONFIG_TPM qemu_add_opts(&qemu_tpmdev_opts); +#endif qemu_add_opts(&qemu_realtime_opts); qemu_add_opts(&qemu_msg_opts); qemu_add_opts(&qemu_name_opts); -- 1.8.5.3
Amos Kong <akong@redhat.com> writes:
Signed-off-by: Amos Kong <akong@redhat.com> --- vl.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/vl.c b/vl.c index 2355227..596ecfa 100644 --- a/vl.c +++ b/vl.c @@ -449,6 +449,7 @@ static QemuOptsList qemu_object_opts = { }, };
+#ifdef CONFIG_TPM static QemuOptsList qemu_tpmdev_opts = { .name = "tpmdev", .implied_opt_name = "type", @@ -458,6 +459,7 @@ static QemuOptsList qemu_tpmdev_opts = { { /* end of list */ } }, }; +#endif
static QemuOptsList qemu_realtime_opts = { .name = "realtime", @@ -2992,7 +2994,9 @@ int main(int argc, char **argv, char **envp) qemu_add_opts(&qemu_sandbox_opts); qemu_add_opts(&qemu_add_fd_opts); qemu_add_opts(&qemu_object_opts); +#ifdef CONFIG_TPM qemu_add_opts(&qemu_tpmdev_opts); +#endif qemu_add_opts(&qemu_realtime_opts); qemu_add_opts(&qemu_msg_opts); qemu_add_opts(&qemu_name_opts);
Making this conditional permits checking whether TPM is enabled via QMP. Mentioning that in the commit message wouldn't hurt. Precedence for conditional adding: "iscsi" (CONFIG_LIBISCSI), "fsdev" (CONFIG_VIRTFS), and possibly more. But it's done differently there: we call qemu_add_opts() from block/iscsi.c, fsdev/qemu-fsdev-opts.c. Call it from tpm.c? Could be done as follow-up cleanup, if that helps getting the fix into 2.0. Related: "add-fd" is defined unconditionally, but works only #ifndef _WIN32. Should it be made conditional to permit querying via QMP? Taking a step back: quite a few command line options make sense only in certain build configurations. We deal with that in several different ways: 1. Target-specific options: qemu-options.hx declares a target mask. main() rejects options that don't apply to the target. Example: --no-acpi is only valid for QEMU_ARCH_I386. 2. Options specific to the host OS are recognized by os_parse_cmd_args(). Any of them not recognized by the host OS's os_parse_cmd_args() are silently ignored. *boggle* Example: --runas is ignored by the Windows build. 3. Options depending on configuration are handled in (at least) three ways: a. The option is only recognized #ifdef CONFIG_FOO. Which means it's silently ignored when FOO isn't enabled. *boggle again* Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI. b. The option is always recognized, but rejected when #ifndef CONFIG_FOO. Example: --curses is rejected #ifndef CONFIG_CURSES. c. The option is always recognized, but rejected when its QemuOptsList hasn't been registered. Essentially just an #if-less way to do 3b. Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is compiled in. In my opinion, silently ignoring an option is a bug until proven otherwise. Whether a non-sensical option should be recognized and rejected, or just not recognized is debatable. Regardless, telling QMP clients that an option works when it's always rejected isn't useful. We can either hide them in QMP, or add suitable "invalid" markers, possibly identifying the reason. Let's hide, unless somebody can come up with a convincing use case for the additional information. For each of the cases above, how can we hide? 1. Easy, check the target mask. 2. Turning "makes sense for host OS" from code into data we can check. 3a. Likewise. 3b. Likewise. 3c. If qemu-options.hx declares the QemuOptsList name, we can check whether the named list exists. Could also be used to factor the qemu_opt_parse() out of the option switch. We may not be able to get this wrapped in time for 2.0. I'm not opposed to a partial solution in 2.0, but let's think through the full solution, to ensure the partial solution doesn't conflict with it.
On 03/28/2014 06:04 AM, Markus Armbruster wrote:
Amos Kong <akong@redhat.com> writes:
Taking a step back: quite a few command line options make sense only in certain build configurations. We deal with that in several different ways:
1. Target-specific options: qemu-options.hx declares a target mask. main() rejects options that don't apply to the target.
Example: --no-acpi is only valid for QEMU_ARCH_I386.
Which means 'query-command-line-options' should not report 'no-acpi' except when built for i386 emulation.
2. Options specific to the host OS are recognized by os_parse_cmd_args(). Any of them not recognized by the host OS's os_parse_cmd_args() are silently ignored. *boggle*
Example: --runas is ignored by the Windows build.
Sounds like a bug. Libvirt doesn't yet run qemu on a Windows build, but ideally, 'query-command-line-options' should not report 'runas' on a qemu binary built for windows.
3. Options depending on configuration are handled in (at least) three ways:
a. The option is only recognized #ifdef CONFIG_FOO. Which means it's silently ignored when FOO isn't enabled. *boggle again*
Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI.
Eww. That's a bug - advertising a feature only to silently ignore it is not helpful.
b. The option is always recognized, but rejected when #ifndef CONFIG_FOO.
Example: --curses is rejected #ifndef CONFIG_CURSES.
Recognizing and rejecting with a nice message, vs. not recognizing and giving a generic 'unknown option' message, both have the same net effect. It's more code to give the nice message, and is helpful to humans, but if the option is omitted from 'query-command-line-options', it makes no difference to libvirt.
c. The option is always recognized, but rejected when its QemuOptsList hasn't been registered. Essentially just an #if-less way to do 3b.
Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is compiled in.
In my opinion, silently ignoring an option is a bug until proven otherwise.
Agreed.
Whether a non-sensical option should be recognized and rejected, or just not recognized is debatable.
Less code to not recognize it, you are then up to the question of whether the nicer error message warrants the extra code.
Regardless, telling QMP clients that an option works when it's always rejected isn't useful. We can either hide them in QMP, or add suitable "invalid" markers, possibly identifying the reason. Let's hide, unless somebody can come up with a convincing use case for the additional information.
Agreed - hiding is nicer than having to expose yet more QMP details to mark an option that is "recognized but will never work".
For each of the cases above, how can we hide?
1. Easy, check the target mask.
2. Turning "makes sense for host OS" from code into data we can check.
3a. Likewise.
3b. Likewise.
3c. If qemu-options.hx declares the QemuOptsList name, we can check whether the named list exists. Could also be used to factor the qemu_opt_parse() out of the option switch.
We may not be able to get this wrapped in time for 2.0. I'm not opposed to a partial solution in 2.0, but let's think through the full solution, to ensure the partial solution doesn't conflict with it.
We're no worse than we were for 1.5 when query-command-line-options was first introduced - at this point, since we're not fixing a regression and since the bug is longstanding, it may be wiser to just leave 2.0 as-is and save all the work for 2.1, rather than rushing in a partial fix for 2.0 only to have to redo it again later. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> writes:
On 03/28/2014 06:04 AM, Markus Armbruster wrote:
Amos Kong <akong@redhat.com> writes:
Taking a step back: quite a few command line options make sense only in certain build configurations. We deal with that in several different ways:
1. Target-specific options: qemu-options.hx declares a target mask. main() rejects options that don't apply to the target.
Example: --no-acpi is only valid for QEMU_ARCH_I386.
Which means 'query-command-line-options' should not report 'no-acpi' except when built for i386 emulation.
Yes.
2. Options specific to the host OS are recognized by os_parse_cmd_args(). Any of them not recognized by the host OS's os_parse_cmd_args() are silently ignored. *boggle*
Example: --runas is ignored by the Windows build.
Sounds like a bug. Libvirt doesn't yet run qemu on a Windows build, but ideally, 'query-command-line-options' should not report 'runas' on a qemu binary built for windows.
Yes.
3. Options depending on configuration are handled in (at least) three ways:
a. The option is only recognized #ifdef CONFIG_FOO. Which means it's silently ignored when FOO isn't enabled. *boggle again*
Example: --iscsi is ignored #ifndef CONFIG_LIBISCSI.
Eww. That's a bug - advertising a feature only to silently ignore it is not helpful.
Indeed.
b. The option is always recognized, but rejected when #ifndef CONFIG_FOO.
Example: --curses is rejected #ifndef CONFIG_CURSES.
Recognizing and rejecting with a nice message, vs. not recognizing and giving a generic 'unknown option' message, both have the same net effect. It's more code to give the nice message, and is helpful to humans, but if the option is omitted from 'query-command-line-options', it makes no difference to libvirt.
Yes.
c. The option is always recognized, but rejected when its QemuOptsList hasn't been registered. Essentially just an #if-less way to do 3b.
Example: --fsdev is rejected unless fsdev/qemu-fsdev-opts.c is compiled in.
In my opinion, silently ignoring an option is a bug until proven otherwise.
Agreed.
Whether a non-sensical option should be recognized and rejected, or just not recognized is debatable.
Less code to not recognize it, you are then up to the question of whether the nicer error message warrants the extra code.
Regardless, telling QMP clients that an option works when it's always rejected isn't useful. We can either hide them in QMP, or add suitable "invalid" markers, possibly identifying the reason. Let's hide, unless somebody can come up with a convincing use case for the additional information.
Agreed - hiding is nicer than having to expose yet more QMP details to mark an option that is "recognized but will never work".
Let's keep it simple until there's a clear need for fancy.
For each of the cases above, how can we hide?
1. Easy, check the target mask.
2. Turning "makes sense for host OS" from code into data we can check.
3a. Likewise.
3b. Likewise.
3c. If qemu-options.hx declares the QemuOptsList name, we can check whether the named list exists. Could also be used to factor the qemu_opt_parse() out of the option switch.
We may not be able to get this wrapped in time for 2.0. I'm not opposed to a partial solution in 2.0, but let's think through the full solution, to ensure the partial solution doesn't conflict with it.
We're no worse than we were for 1.5 when query-command-line-options was first introduced - at this point, since we're not fixing a regression and since the bug is longstanding, it may be wiser to just leave 2.0 as-is and save all the work for 2.1, rather than rushing in a partial fix for 2.0 only to have to redo it again later.
Let's aim for 2.1 then. Amos, thanks for trying so hard to get it fixed in 2.0. Us finding one can of worms after the other is not your fault.
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> Reviewed-by: Eric Blake <eblake@redhat.com> --- 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 596ecfa..0464494 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), @@ -1359,7 +1359,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), @@ -3241,7 +3241,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); } @@ -3610,7 +3610,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); } @@ -3677,7 +3677,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; @@ -4002,7 +4002,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) { @@ -4186,7 +4186,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
All the options are defined in qemu-options.hx. If we can't find a matched option definition by group name of option table, then the group name doesn't match with defined option name, it's not allowed from 2.0 Signed-off-by: Amos Kong <akong@redhat.com> --- qemu-options.h | 12 ++++++++++++ util/qemu-config.c | 28 ++++++++++++++++++++++++++++ vl.c | 19 ++----------------- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/qemu-options.h b/qemu-options.h index 89a009e..4024487 100644 --- a/qemu-options.h +++ b/qemu-options.h @@ -28,9 +28,21 @@ #ifndef _QEMU_OPTIONS_H_ #define _QEMU_OPTIONS_H_ +#include "sysemu/arch_init.h" + enum { #define QEMU_OPTIONS_GENERATE_ENUM #include "qemu-options-wrapper.h" }; +#define HAS_ARG 0x0001 + +typedef struct QEMUOption { + const char *name; + int flags; + int index; + uint32_t arch_mask; +} QEMUOption; + +extern const QEMUOption qemu_options[]; #endif diff --git a/util/qemu-config.c b/util/qemu-config.c index f610101..eba5428 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -6,6 +6,14 @@ #include "hw/qdev.h" #include "qapi/error.h" #include "qmp-commands.h" +#include "qemu-options.h" + +const QEMUOption qemu_options[] = { + { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, +#define QEMU_OPTIONS_GENERATE_OPTIONS +#include "qemu-options-wrapper.h" + { NULL }, +}; static QemuOptsList *vm_config_groups[32]; static QemuOptsList *drive_config_groups[4]; @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list) abort(); } +/* check if the option is defined in qemu-options.hx */ +static bool opt_is_defined(const char *name) +{ + int i; + + for (i = 0; qemu_options[i].name; i++) { + if (!strcmp(qemu_options[i].name, name)) { + return true; + } + } + + return false; +} + void qemu_add_opts(QemuOptsList *list) { int entries, i; @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) for (i = 0; i < entries; i++) { if (vm_config_groups[i] == NULL) { vm_config_groups[i] = list; + if (!opt_is_defined(list->name)) { + error_report("Didn't find a matched option definition, " + "group name (%s) of option table must match with " + "defined option name (Since 2.0)", list->name); + abort(); + } return; } } diff --git a/vl.c b/vl.c index 0464494..bd44c52 100644 --- a/vl.c +++ b/vl.c @@ -111,7 +111,6 @@ int main(int argc, char **argv) #include "trace/control.h" #include "qemu/queue.h" #include "sysemu/cpus.h" -#include "sysemu/arch_init.h" #include "qemu/osdep.h" #include "ui/qemu-spice.h" @@ -2082,22 +2081,6 @@ static void help(int exitcode) exit(exitcode); } -#define HAS_ARG 0x0001 - -typedef struct QEMUOption { - const char *name; - int flags; - int index; - uint32_t arch_mask; -} QEMUOption; - -static const QEMUOption qemu_options[] = { - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, -#define QEMU_OPTIONS_GENERATE_OPTIONS -#include "qemu-options-wrapper.h" - { NULL }, -}; - static bool vga_available(void) { return object_class_by_name("VGA") || object_class_by_name("isa-vga"); @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv, return popt; } +#undef HAS_ARG + static gpointer malloc_and_trace(gsize n_bytes) { void *ptr = malloc(n_bytes); -- 1.8.5.3
Hi Amos, On Thu, Mar 27, 2014 at 09:04:31PM +0800, Amos Kong wrote:
All the options are defined in qemu-options.hx. If we can't find a matched option definition by group name of option table, then the group name doesn't match with defined option name, it's not allowed from 2.0
Signed-off-by: Amos Kong <akong@redhat.com> --- qemu-options.h | 12 ++++++++++++ util/qemu-config.c | 28 ++++++++++++++++++++++++++++ vl.c | 19 ++----------------- 3 files changed, 42 insertions(+), 17 deletions(-)
diff --git a/qemu-options.h b/qemu-options.h index 89a009e..4024487 100644 --- a/qemu-options.h +++ b/qemu-options.h @@ -28,9 +28,21 @@ #ifndef _QEMU_OPTIONS_H_ #define _QEMU_OPTIONS_H_
+#include "sysemu/arch_init.h" + enum { #define QEMU_OPTIONS_GENERATE_ENUM #include "qemu-options-wrapper.h" };
+#define HAS_ARG 0x0001 + +typedef struct QEMUOption { + const char *name; + int flags; + int index; + uint32_t arch_mask; +} QEMUOption; + +extern const QEMUOption qemu_options[]; #endif diff --git a/util/qemu-config.c b/util/qemu-config.c index f610101..eba5428 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -6,6 +6,14 @@ #include "hw/qdev.h" #include "qapi/error.h" #include "qmp-commands.h" +#include "qemu-options.h" + +const QEMUOption qemu_options[] = { + { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, +#define QEMU_OPTIONS_GENERATE_OPTIONS +#include "qemu-options-wrapper.h" + { NULL }, +};
static QemuOptsList *vm_config_groups[32]; static QemuOptsList *drive_config_groups[4]; @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list) abort(); }
+/* check if the option is defined in qemu-options.hx */ +static bool opt_is_defined(const char *name) +{ + int i; + + for (i = 0; qemu_options[i].name; i++) { + if (!strcmp(qemu_options[i].name, name)) { + return true; + } + } + + return false; +} + void qemu_add_opts(QemuOptsList *list) { int entries, i; @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) for (i = 0; i < entries; i++) { if (vm_config_groups[i] == NULL) { vm_config_groups[i] = list; + if (!opt_is_defined(list->name)) { + error_report("Didn't find a matched option definition, " + "group name (%s) of option table must match with " + "defined option name (Since 2.0)", list->name);
I'm not a native English speaker but I don't fell comfortable with this message output, what about "option table's group name (%s) must match with predefined option name (Since 2.0)"? The patch looks good, I tested it and it works properly, so Reviewed-by: Leandro Dorileo <l@dorileo.org>
+ abort(); + } return; } } diff --git a/vl.c b/vl.c index 0464494..bd44c52 100644 --- a/vl.c +++ b/vl.c @@ -111,7 +111,6 @@ int main(int argc, char **argv) #include "trace/control.h" #include "qemu/queue.h" #include "sysemu/cpus.h" -#include "sysemu/arch_init.h" #include "qemu/osdep.h"
#include "ui/qemu-spice.h" @@ -2082,22 +2081,6 @@ static void help(int exitcode) exit(exitcode); }
-#define HAS_ARG 0x0001 - -typedef struct QEMUOption { - const char *name; - int flags; - int index; - uint32_t arch_mask; -} QEMUOption; - -static const QEMUOption qemu_options[] = { - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, -#define QEMU_OPTIONS_GENERATE_OPTIONS -#include "qemu-options-wrapper.h" - { NULL }, -}; - static bool vga_available(void) { return object_class_by_name("VGA") || object_class_by_name("isa-vga"); @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv, return popt; }
+#undef HAS_ARG + static gpointer malloc_and_trace(gsize n_bytes) { void *ptr = malloc(n_bytes); -- 1.8.5.3
-- Leandro Dorileo
Amos Kong <akong@redhat.com> writes:
All the options are defined in qemu-options.hx. If we can't find a matched option definition by group name of option table, then the group name doesn't match with defined option name, it's not allowed from 2.0
Signed-off-by: Amos Kong <akong@redhat.com> --- qemu-options.h | 12 ++++++++++++ util/qemu-config.c | 28 ++++++++++++++++++++++++++++ vl.c | 19 ++----------------- 3 files changed, 42 insertions(+), 17 deletions(-)
diff --git a/qemu-options.h b/qemu-options.h index 89a009e..4024487 100644 --- a/qemu-options.h +++ b/qemu-options.h @@ -28,9 +28,21 @@ #ifndef _QEMU_OPTIONS_H_ #define _QEMU_OPTIONS_H_
+#include "sysemu/arch_init.h" + enum { #define QEMU_OPTIONS_GENERATE_ENUM #include "qemu-options-wrapper.h" };
+#define HAS_ARG 0x0001 + +typedef struct QEMUOption { + const char *name; + int flags; + int index; + uint32_t arch_mask; +} QEMUOption; + +extern const QEMUOption qemu_options[]; #endif
Instead of exposing struct QEMUOption and qemu_options[] here, why not simply put opt_is_defined() in vl.c, and declare it in qemu-options.h?
diff --git a/util/qemu-config.c b/util/qemu-config.c index f610101..eba5428 100644 --- a/util/qemu-config.c +++ b/util/qemu-config.c @@ -6,6 +6,14 @@ #include "hw/qdev.h" #include "qapi/error.h" #include "qmp-commands.h" +#include "qemu-options.h" + +const QEMUOption qemu_options[] = { + { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, +#define QEMU_OPTIONS_GENERATE_OPTIONS +#include "qemu-options-wrapper.h" + { NULL }, +};
static QemuOptsList *vm_config_groups[32]; static QemuOptsList *drive_config_groups[4]; @@ -184,6 +192,20 @@ void qemu_add_drive_opts(QemuOptsList *list) abort(); }
+/* check if the option is defined in qemu-options.hx */ +static bool opt_is_defined(const char *name) +{ + int i; + + for (i = 0; qemu_options[i].name; i++) { + if (!strcmp(qemu_options[i].name, name)) { + return true; + } + } + + return false; +} +
The same loop is hidden in lookup_opt(). We could avoid the duplication by putting it in a function returning the option index. That could easily be a proper enumeration type, like this, in qemu-options.h: typedef enum QEMUOption { #define QEMU_OPTIONS_GENERATE_ENUM #include "qemu-options-wrapper.h" } QEMUOption; QEMUOption qemu_option_lookup(const char *name); plus a rename of vl.c's QEMUOption to QEMUOptionDesc or something. Can be done on top.
void qemu_add_opts(QemuOptsList *list) { int entries, i; @@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) for (i = 0; i < entries; i++) { if (vm_config_groups[i] == NULL) { vm_config_groups[i] = list; + if (!opt_is_defined(list->name)) { + error_report("Didn't find a matched option definition, " + "group name (%s) of option table must match with " + "defined option name (Since 2.0)", list->name); + abort(); + } return; } }
Simple! Wish it was my idea ;) Why not simply assert(opt_is_defined(list->name))?
diff --git a/vl.c b/vl.c index 0464494..bd44c52 100644 --- a/vl.c +++ b/vl.c @@ -111,7 +111,6 @@ int main(int argc, char **argv) #include "trace/control.h" #include "qemu/queue.h" #include "sysemu/cpus.h" -#include "sysemu/arch_init.h" #include "qemu/osdep.h"
#include "ui/qemu-spice.h" @@ -2082,22 +2081,6 @@ static void help(int exitcode) exit(exitcode); }
-#define HAS_ARG 0x0001 - -typedef struct QEMUOption { - const char *name; - int flags; - int index; - uint32_t arch_mask; -} QEMUOption; - -static const QEMUOption qemu_options[] = { - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, -#define QEMU_OPTIONS_GENERATE_OPTIONS -#include "qemu-options-wrapper.h" - { NULL }, -}; - static bool vga_available(void) { return object_class_by_name("VGA") || object_class_by_name("isa-vga"); @@ -2842,6 +2825,8 @@ static const QEMUOption *lookup_opt(int argc, char **argv, return popt; }
+#undef HAS_ARG + static gpointer malloc_and_trace(gsize n_bytes) { void *ptr = malloc(n_bytes);
Undefining HAS_ARG here, where it hasn't done any harm, while letting it pollute every other compilation unit including qemu-options.h makes no sense.
On 03/28/2014 08:55 AM, Markus Armbruster wrote:
Amos Kong <akong@redhat.com> writes:
All the options are defined in qemu-options.hx. If we can't find a matched option definition by group name of option table, then the group name doesn't match with defined option name, it's not allowed from 2.0
@@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) for (i = 0; i < entries; i++) { if (vm_config_groups[i] == NULL) { vm_config_groups[i] = list; + if (!opt_is_defined(list->name)) { + error_report("Didn't find a matched option definition, " + "group name (%s) of option table must match with " + "defined option name (Since 2.0)", list->name); + abort(); + } return; } }
Simple! Wish it was my idea ;)
Why not simply assert(opt_is_defined(list->name))?
Indeed, using assert() would also solve the problem of the error message being awkward.
-#define HAS_ARG 0x0001 -
- -static const QEMUOption qemu_options[] = { - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, -#define QEMU_OPTIONS_GENERATE_OPTIONS -#include "qemu-options-wrapper.h" - { NULL }, -};
+#undef HAS_ARG
HAS_ARG is not very namespace clean. Prior to your patch, it was used only in a single file (where we know it doesn't collide). After your patch, it is now in a header used by multiple files.
+ static gpointer malloc_and_trace(gsize n_bytes) { void *ptr = malloc(n_bytes);
Undefining HAS_ARG here, where it hasn't done any harm, while letting it pollute every other compilation unit including qemu-options.h makes no sense.
Maybe a better approach would be to create an enum in qemu-options.h of actual flag values: typedef enum { QEMU_OPT_HAS_ARG = 1, } QEMUOptionFlags; and use QEMU_OPT_HAS_ARG instead of HAS_ARG in vl.c. Additionally, you either have to s/HAS_ARG/QEMU_OPT_HAS_ARG/ throughout the .hx file, or you can take a shortcut in qemu-config.c: #define HAS_ARG QEMU_OPT_HAS_ARG const QEMUOption qemu_options[] = { { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, #define QEMU_OPTIONS_GENERATE_OPTIONS #include "qemu-options-wrapper.h" { NULL }, }; #undef HAS_ARG since that is the only place that includes the .hx file at a point where HAS_ARG has to be expanded to something useful. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake <eblake@redhat.com> writes:
On 03/28/2014 08:55 AM, Markus Armbruster wrote:
Amos Kong <akong@redhat.com> writes:
All the options are defined in qemu-options.hx. If we can't find a matched option definition by group name of option table, then the group name doesn't match with defined option name, it's not allowed from 2.0
@@ -193,6 +215,12 @@ void qemu_add_opts(QemuOptsList *list) for (i = 0; i < entries; i++) { if (vm_config_groups[i] == NULL) { vm_config_groups[i] = list; + if (!opt_is_defined(list->name)) { + error_report("Didn't find a matched option definition, " + "group name (%s) of option table must match with " + "defined option name (Since 2.0)", list->name); + abort(); + } return; } }
Simple! Wish it was my idea ;)
Why not simply assert(opt_is_defined(list->name))?
Indeed, using assert() would also solve the problem of the error message being awkward.
-#define HAS_ARG 0x0001 -
- -static const QEMUOption qemu_options[] = { - { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, -#define QEMU_OPTIONS_GENERATE_OPTIONS -#include "qemu-options-wrapper.h" - { NULL }, -};
+#undef HAS_ARG
HAS_ARG is not very namespace clean. Prior to your patch, it was used only in a single file (where we know it doesn't collide). After your patch, it is now in a header used by multiple files.
+ static gpointer malloc_and_trace(gsize n_bytes) { void *ptr = malloc(n_bytes);
Undefining HAS_ARG here, where it hasn't done any harm, while letting it pollute every other compilation unit including qemu-options.h makes no sense.
Maybe a better approach would be to create an enum in qemu-options.h of actual flag values:
typedef enum { QEMU_OPT_HAS_ARG = 1, } QEMUOptionFlags;
and use QEMU_OPT_HAS_ARG instead of HAS_ARG in vl.c. Additionally, you either have to s/HAS_ARG/QEMU_OPT_HAS_ARG/ throughout the .hx file, or you can take a shortcut in qemu-config.c:
#define HAS_ARG QEMU_OPT_HAS_ARG
const QEMUOption qemu_options[] = { { "h", 0, QEMU_OPTION_h, QEMU_ARCH_ALL }, #define QEMU_OPTIONS_GENERATE_OPTIONS #include "qemu-options-wrapper.h" { NULL }, };
#undef HAS_ARG
since that is the only place that includes the .hx file at a point where HAS_ARG has to be expanded to something useful.
Either something like this, or avoid moving this stuff to a header. I prefer the latter, and suggested how in my review.
participants (4)
-
Amos Kong -
Eric Blake -
Leandro Dorileo -
Markus Armbruster