Re: [Qemu-devel] [PATCH 1/2] qmp: Add query-qemu-features

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 3/29/19 8:52 AM, Markus Armbruster wrote:
>
>> The basic idea is simple.  Let me explain with an example.
>> 
>> Instead of what I proposed above:
>> 
>>      { 'union': 'BlockdevOptions',
>>        'base': { 'driver': 'BlockdevDriver',
>>                  '*node-name': 'str',
>>                  '*discard': 'BlockdevDiscardOptions',
>>                  '*cache': 'BlockdevCacheOptions',
>>                  '*read-only': 'bool',
>>                  '*auto-read-only': 'bool',
>>     +            '*dynamic-auto-read-only': 'bool',
>>                  '*force-share': 'bool',
>>                  '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
>>     [...]
>>     }
>
> If I'm summarizing correctly, this is the minimum we could pull off to
> get a syntactic witness of the change, in time for 4.0, while leaving:
>
>> 
>> permit something like
>> 
>>      { 'union': 'BlockdevOptions',
>>     +  'features': { 'dynamic-auto-read-only': true },
>>     [...]
>>      }
> ...
>
> as future work.

Yes.

>> Subjective summary:
>> 
>> * For the known use cases, query-qemu-features is merely a crutch for
>>   getting information into the QAPI QMP schema.  Such crutches are ugly,
>>   but in absence of better ideas, ugly wins by default.
>
> That is, this series qualifies as the crutch (if we can't come up with
> anything closer in time for 4.0)

Yes.

Of course, crutchiness is no excuse for not addressing the need in 4.0.
Either we come up with something we like better, or we accept the
crutch.

>                                  - but your arguments say we may have
> something closer:
>
>> 
>> * I think I'd prefer adding @dynamic-auto-read-only to
>>   BlockdevOptionsFile, or perhaps to BlockdevOptions.  Takes precedence
>>   over @auto-read-only.  Consider deprecating @auto-read-only.
>
> This is interesting. The existing flag keeps its semantics:
>
> Open the file with automatic fallback to read-only, may or may not be
> dynamic.
>
> And the new flag is documented as:
>
> Overrides auto-read-only, open the file with automatic fallback to
> read-only, guaranteed to be dynamic (or fails if the guarantee cannot be
> provided).

Yes, adding @dynamic-auto-read-only is also an opportunity to specify
the semantics libvirt actually wants to rely on.  Not knowing enough
about that, I'd approximate by spelling out current actual behavior,
like this:

# @dynamic-auto-read-only: if true, QEMU opens the file read-only
#                  regardless of @read-only.
#                  If @read-only is false, QEMU reopens the file read/write
#                  when a writing user attaches to the node, and
#                  reopens it read-only again when the last writin user
#                  detaches from the node.
#                  (default: false, since 4.0)

We can either add it to BlockdevOptionsFile or to BlockdevOptions (next
to @auto-read-only).  If we choose the latter, the doc comment needs to
spell out that @dynamic-auto-read-only applies only to drivers "file",
"host_device" and "host_cdrom".

Regardless of where we put it, we should guard it with CONFIG_POSIX.

> I don't know if we need the deprecation or not (especially if our plans
> over time are to allow the dynamic behavior in more places than just
> file-posix, where a guaranteed failure if not dynamic vs. a sane
> fallback to only-on-initial-open may still be useful to some callers),

@auto-read-only as specified feels too weak to be useful.  But it's hard
to predict the future.

> but the new parameter with the guaranteed semantics is definitely
> introspectible, so it is closer to the problem than creating
> query-qemu-features.  It also seems like something we could pull
> together in a short amount of time.

Agree.

>> * We need command line introspection (old news).
>> 
>> * A general method to make semantic changes visible syntactically would
>>   be useful.  The "augment the QAPI schema with feature flags" idea we
>>   discussed in last KVM Forum's hallway track could be a starting point.
>
> These are not 4.0 goals.

True!

>> Comments?  Opinions?
>
> Thanks for working on a nice summary, and forcing us to think about the
> long-term before taking a short-term hack that we may regret.

You're welcome :)