Re: [PATCH v2 2/2] block/null: Add read-pattern option

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On Thu, May 08, 2025 at 07:28:26AM +0200, Markus Armbruster wrote:
>> Let's take a step back from the concrete interface and ponder what we're
>> trying to do.  We want three cases:
>> 
>> * Allocated, reads return unspecified crap (security hazard)
>> 
>> * Allocated, reads return specified data
>> 
>> * Sparse, reads return zeroes
>> 
>> How would we do this if we started on a green field?
>> 
>> Here's my try:
>> 
>>     bool sparse
>>     uint8 contents
>> 
>> so that
>> 
>> * Allocated, reads return unspecified crap (security hazard)
>> 
>>   @sparse is false, and @contents is absent
>> 
>> * Allocated, reads return specified data
>> 
>>   @sparse is false, and @contents is present
>> 
>> * Sparse, reads return zeroes
>> 
>>   @sparse is true, and @contents must absent or zero
>
> That indeed is a nice view of what we hope to test with.
>
>> 
>> I'd make @sparse either mandatory or default to true, so that we don't
>> default to security hazard.
>> 
>> Now compare this to your patch: same configuration data (bool × uint8),
>> better names, cleaner semantics, better defaults.
>> 
>> Unless we want to break compatibility, we're stuck with the name
>> @read-zeroes for the bool, and its trap-for-the-unwary default value,
>> but cleaner semantics seem possible.
>> 
>> Thoughts?
>
> What if we add @sparse as an optional bool, but mutually exclusive
> with @read-zeroes.  That would lead to 27 combinations of absent,
> present with 0 value, or present with non-zero value; but with fewer
> actual cases supported.  Something like your above table of what to do
> with sparse and contents, and with these additional rules:
>
> read-zeroes omitted, sparse omitted
>  - at present, defaults to sparse=false for back-compat
>  - may change in the future [*]
> read-zeroes present, sparse omitted
>  - behaves like explicit setting of sparse, but with old spelling
>  - may issue a deprecation warning [*]
> read-zeroes omitted, sparse present
>  - explicit spelling, no warning (use above logic for how contents acts)
> read-zeroes and sparse both present
>  - error, whether values were same or different
>
> [*] Simultaneously, we start the deprecation cycle on @read-zeroes -
> tagging it as deprecated now, and removing it in a couple of releases.
> Once it is gone, we are left with just @sparse; at that time, we can
> decide to either make it mandatory (if so, we should warn now if
> neither read-zeroes nor sparse is specified), or leave it optional but
> change it to default true (so that the security hazard of sparse:false
> and omitted contents is now opt-in instead of default).

The recommended way to stay on top of deprecations in QMP is to test
with -compat deprecated-input=reject,deprecated-output=hide or similar.
This relies on special feature 'deprecated', and is limited to syntax.

There is no way to formally deprecate defaults.

We can deprecate @read-zeroes, but this wouldn't cover behavior when
absent.  Changing that is a compatibility break.  Hard to justify.

Except when the interface in question is unstable.  Block drivers
null-aio and null-co aren't.  Should they?

Warnings are awkward with QMP.  We can only warn to stderr.