Re: [PATCH v2 1/4] keyval: Parse help options

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

[...]
> Making help support opt-in complicates things.  Is there a genuine use
> for not supporting help?  Or is just to keep the users that don't
> support help yet (but should) working without change?  Mind, I'm not
> asking you to make them work, I'm only asking whether you can think of a
> genuine case where help should not work.
>
> What are the existing users that don't support help?  Let's see... many
> in tests/test-keyval.c (ignore), and qobject_input_visitor_new_str().
> That one's used for qemu-system-FOO -audiodev, -display, -blockdev, and
> for qemu-storage-daemon --blockdev, --export, --monitor, --nbd-server.
>
> Attempting to get help for them fails like this:
>
>     $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev help
>     qemu-storage-daemon: Invalid parameter 'help'
>     $ bld-x86/storage-daemon/qemu-storage-daemon --blockdev file,help
>     qemu-storage-daemon: Expected '=' after parameter 'help'
>
> I believe making them fail like
>
>     qemu-storage-daemon: no help available
>
> would actually be an improvement.

Potential issue: if an option's implied key may have the value "help",
then option argument "help" can be either parsed as "give me help", or
as "implied-key=help".

None of the existing options have this issue:

* audiodev: union Audiodev, implied key "driver" is enum AudiodevDriver,
   "help" is not among its values

* display: union DisplayOptions, implied key "type" is enum
   DisplayType, "help" is not among its values

* blockdev: union BlockdevOptions, implied key "driver is enum
   BlockdevDriver, "help" is not among its values

* export: union BlockExport, implied key "type" is enum BlockExportType,
  "help" is not among its values

* monitor: struct MonitorOptions, implied key "mode"is enum MonitorMode,
  "help" is not among its values

* nbd-server: struct NbdServerOptions, no implied key.

We're good.

What's the risk of not staying good?

The implied keys above are all QAPI enums.

The only existing QAPI enum with a value 'help' is QKeyCode.

The only existing C enum with a name that ends in _HELP is the one
defining Mac keycodes in hw/input/adb-keys.h.

For completeness, I double-checked non of the existing occurences of
string "help" are used as values of option parameters.

We'll almost certainly stay good.

What if we manage to mess this up against all odds?

In my opinion, consistency in getting help is more important than
consistency within a badly designed option: option argument "help"
should give you help even when that means you can't omit the implied key
when its value is "help".

[...]