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

[Markus Armbruster <armbru@redhat.com>]

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.