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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3995 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]