| Message ID | 1395925471-19841-2-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
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> 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.
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);
Signed-off-by: Amos Kong <akong@redhat.com> --- vl.c | 4 ++++ 1 file changed, 4 insertions(+)