Re: [Qemu-devel] [PATCH v5 for 2.0 1/3] only add qemu_tpmdev_opts when CONFIG_TPM is defined

[Markus Armbruster]

Amos Kong <address@hidden> writes:

> Signed-off-by: Amos Kong <address@hidden>
> ---
>  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.