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

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

[Nir Soffer]

The null driver is very usefull for testing and benchmarking, but it can not
emulate an image full of zeroes or an image full of non-zero bytes. Such images
are needed for testing computing a blkhash via qemu-nbd or qemu-storage-daemon.

This change adds `read-pattern` option allowing emulution of image full of
zeroes and image for of non-zero bytes.

I used this for testing
https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg05096.html

Changes since v1:
- Make read-zeroes and read-pattern mutual exclusive [Eric, Markus]
- Narrow read-pattern type to uint8 [Eric, Markus]
- Update the doc comment to explain that read-zeroes emulates a sparsse image,
  and read-pattern emulates an allocated image. [Markus]
- Validate that read-pattern value is within range
- Update secure-coding-practices guide with read-pattern option

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg05098.html

Nir Soffer (2):
  block/null: Report DATA if not reading zeroes
  block/null: Add read-pattern option

 block/null.c                           | 38 +++++++++++++++++++++++---
 docs/devel/secure-coding-practices.rst |  3 +-
 qapi/block-core.json                   | 17 ++++++++++--
 3 files changed, 50 insertions(+), 8 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v2 1/2] block/null: Report DATA if not reading zeroes

[Nir Soffer]

If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
confuse users or other programs:

    % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
    [{ "start": 0, "length": 1073741824, "depth": 0, "present": false, "zero": false, "data": false, "compressed": false}]

    % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &

    % nbdinfo --map nbd://127.0.0.1
             0  1073741824    1  hole

With this change we report DATA in this case:

    % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
    [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": 0}]

    % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &

    % nbdinfo --map nbd://127.0.0.1
             0  1073741824    0  data

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 block/null.c         | 4 +---
 qapi/block-core.json | 5 +++--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/block/null.c b/block/null.c
index dc0b1fdbd9..7ba87bd9a9 100644
--- a/block/null.c
+++ b/block/null.c
@@ -239,9 +239,7 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
     *map = offset;
     *file = bs;
 
-    if (s->read_zeroes) {
-        ret |= BDRV_BLOCK_ZERO;
-    }
+    ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
     return ret;
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b1937780e1..7c95c9e36a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3293,8 +3293,9 @@
 #     requests.  Default to zero which completes requests immediately.
 #     (Since 2.4)
 #
-# @read-zeroes: if true, reads from the device produce zeroes; if
-#     false, the buffer is left unchanged.
+# @read-zeroes: if true, emulate a sparse image, and reads from the
+#     device produce zeroes; if false, emulate an allocated image but
+#     reads from the device leave the buffer unchanged.
 #     (default: false; since: 4.1)
 #
 # Since: 2.9
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 1/2] block/null: Report DATA if not reading zeroes

[Markus Armbruster]

Nir Soffer <nirsof@gmail.com> writes:

> If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
> BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
> confuse users or other programs:
>
>     % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>     [{ "start": 0, "length": 1073741824, "depth": 0, "present": false, "zero": false, "data": false, "compressed": false}]
>
>     % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>
>     % nbdinfo --map nbd://127.0.0.1
>              0  1073741824    1  hole
>
> With this change we report DATA in this case:
>
>     % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>     [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": 0}]
>
>     % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>
>     % nbdinfo --map nbd://127.0.0.1
>              0  1073741824    0  data
>
> Signed-off-by: Nir Soffer <nirsof@gmail.com>
> ---
>  block/null.c         | 4 +---
>  qapi/block-core.json | 5 +++--
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/block/null.c b/block/null.c
> index dc0b1fdbd9..7ba87bd9a9 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -239,9 +239,7 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
>      *map = offset;
>      *file = bs;
>  
> -    if (s->read_zeroes) {
> -        ret |= BDRV_BLOCK_ZERO;
> -    }
> +    ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
>      return ret;
>  }
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index b1937780e1..7c95c9e36a 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3293,8 +3293,9 @@
>  #     requests.  Default to zero which completes requests immediately.
>  #     (Since 2.4)
>  #
> -# @read-zeroes: if true, reads from the device produce zeroes; if
> -#     false, the buffer is left unchanged.
> +# @read-zeroes: if true, emulate a sparse image, and reads from the
> +#     device produce zeroes; if false, emulate an allocated image but
> +#     reads from the device leave the buffer unchanged.
>  #     (default: false; since: 4.1)
>  #
>  # Since: 2.9

Possibly dumb question: how is this doc change related to the code fix?

Suggest to split the sentence for easier reading:

   # @read-zeroes: If true, emulate a sparse image, and reads from the
   #     device produce zeroes.  If false, emulate an allocated image,
   #     but reads from the device leave the buffer unchanged.

Re: [PATCH v2 1/2] block/null: Report DATA if not reading zeroes

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Nir Soffer <nirsof@gmail.com> writes:
>
>> If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
>> BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
>> confuse users or other programs:
>>
>>     % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>>     [{ "start": 0, "length": 1073741824, "depth": 0, "present": false, "zero": false, "data": false, "compressed": false}]
>>
>>     % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>>
>>     % nbdinfo --map nbd://127.0.0.1
>>              0  1073741824    1  hole
>>
>> With this change we report DATA in this case:
>>
>>     % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>>     [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": 0}]
>>
>>     % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>>
>>     % nbdinfo --map nbd://127.0.0.1
>>              0  1073741824    0  data
>>
>> Signed-off-by: Nir Soffer <nirsof@gmail.com>
>> ---
>>  block/null.c         | 4 +---
>>  qapi/block-core.json | 5 +++--
>>  2 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/null.c b/block/null.c
>> index dc0b1fdbd9..7ba87bd9a9 100644
>> --- a/block/null.c
>> +++ b/block/null.c
>> @@ -239,9 +239,7 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
>>      *map = offset;
>>      *file = bs;
>>  
>> -    if (s->read_zeroes) {
>> -        ret |= BDRV_BLOCK_ZERO;
>> -    }
>> +    ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
>>      return ret;
>>  }
>>  
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b1937780e1..7c95c9e36a 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3293,8 +3293,9 @@
>>  #     requests.  Default to zero which completes requests immediately.
>>  #     (Since 2.4)
>>  #
>> -# @read-zeroes: if true, reads from the device produce zeroes; if
>> -#     false, the buffer is left unchanged.
>> +# @read-zeroes: if true, emulate a sparse image, and reads from the
>> +#     device produce zeroes; if false, emulate an allocated image but
>> +#     reads from the device leave the buffer unchanged.
>>  #     (default: false; since: 4.1)
>>  #
>>  # Since: 2.9
>
> Possibly dumb question: how is this doc change related to the code fix?
>
> Suggest to split the sentence for easier reading:
>
>    # @read-zeroes: If true, emulate a sparse image, and reads from the
>    #     device produce zeroes.  If false, emulate an allocated image,
>    #     but reads from the device leave the buffer unchanged.

false is a security hazard, as secure-coding-practices.rst points out.
I think it should be pointed out right here as well.  Especially since
"security hazard" is the default!

I'd do it in a separate patch, but I'm a compulsive patch splitter.

Re: [PATCH v2 1/2] block/null: Report DATA if not reading zeroes

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Nir Soffer <nirsof@gmail.com> writes:

[...]

>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index b1937780e1..7c95c9e36a 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3293,8 +3293,9 @@
     ##
     # @BlockdevOptionsNull:
     #
     # Driver specific block device options for the null backend.
     #
     # @size: size of the device in bytes.

Missing: default value.

     #
     # @latency-ns: emulated latency (in nanoseconds) in processing
>>>  #     requests.  Default to zero which completes requests immediately.
>>>  #     (Since 2.4)
>>>  #
>>> -# @read-zeroes: if true, reads from the device produce zeroes; if
>>> -#     false, the buffer is left unchanged.
>>> +# @read-zeroes: if true, emulate a sparse image, and reads from the
>>> +#     device produce zeroes; if false, emulate an allocated image but
>>> +#     reads from the device leave the buffer unchanged.
>>>  #     (default: false; since: 4.1)
>>>  #
>>>  # Since: 2.9
>>
>> Possibly dumb question: how is this doc change related to the code fix?
>>
>> Suggest to split the sentence for easier reading:
>>
>>    # @read-zeroes: If true, emulate a sparse image, and reads from the
>>    #     device produce zeroes.  If false, emulate an allocated image,
>>    #     but reads from the device leave the buffer unchanged.
>
> false is a security hazard, as secure-coding-practices.rst points out.
> I think it should be pointed out right here as well.  Especially since
> "security hazard" is the default!
>
> I'd do it in a separate patch, but I'm a compulsive patch splitter.

Re: [PATCH v2 1/2] block/null: Report DATA if not reading zeroes

[Nir Soffer]

> On 8 May 2025, at 8:20, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>>> Nir Soffer <nirsof@gmail.com> writes:
> 
> [...]
> 
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index b1937780e1..7c95c9e36a 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -3293,8 +3293,9 @@
>     ##
>     # @BlockdevOptionsNull:
>     #
>     # Driver specific block device options for the null backend.
>     #
>     # @size: size of the device in bytes.
> 
> Missing: default value.

This was not added by my change, but I can add another commit to specify the default size.

But it would be better to make this required, default size does not make sense for a block device, expect maybe 0, but this is not very useful default.

Can we break compatibility by requiring a size?

Re: [PATCH v2 1/2] block/null: Report DATA if not reading zeroes

[Nir Soffer]

> On 8 May 2025, at 8:03, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Nir Soffer <nirsof@gmail.com> writes:
>> 
>>> If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
>>> BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
>>> confuse users or other programs:
>>> 
>>>    % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>>>    [{ "start": 0, "length": 1073741824, "depth": 0, "present": false, "zero": false, "data": false, "compressed": false}]
>>> 
>>>    % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>>> 
>>>    % nbdinfo --map nbd://127.0.0.1
>>>             0  1073741824    1  hole
>>> 
>>> With this change we report DATA in this case:
>>> 
>>>    % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>>>    [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": 0}]
>>> 
>>>    % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>>> 
>>>    % nbdinfo --map nbd://127.0.0.1
>>>             0  1073741824    0  data
>>> 
>>> Signed-off-by: Nir Soffer <nirsof@gmail.com>
>>> ---
>>> block/null.c         | 4 +---
>>> qapi/block-core.json | 5 +++--
>>> 2 files changed, 4 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/block/null.c b/block/null.c
>>> index dc0b1fdbd9..7ba87bd9a9 100644
>>> --- a/block/null.c
>>> +++ b/block/null.c
>>> @@ -239,9 +239,7 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
>>>     *map = offset;
>>>     *file = bs;
>>> 
>>> -    if (s->read_zeroes) {
>>> -        ret |= BDRV_BLOCK_ZERO;
>>> -    }
>>> +    ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
>>>     return ret;
>>> }
>>> 
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index b1937780e1..7c95c9e36a 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -3293,8 +3293,9 @@
>>> #     requests.  Default to zero which completes requests immediately.
>>> #     (Since 2.4)
>>> #
>>> -# @read-zeroes: if true, reads from the device produce zeroes; if
>>> -#     false, the buffer is left unchanged.
>>> +# @read-zeroes: if true, emulate a sparse image, and reads from the
>>> +#     device produce zeroes; if false, emulate an allocated image but
>>> +#     reads from the device leave the buffer unchanged.
>>> #     (default: false; since: 4.1)
>>> #
>>> # Since: 2.9
>> 
>> Possibly dumb question: how is this doc change related to the code fix?
>> 
>> Suggest to split the sentence for easier reading:
>> 
>>   # @read-zeroes: If true, emulate a sparse image, and reads from the
>>   #     device produce zeroes.  If false, emulate an allocated image,
>>   #     but reads from the device leave the buffer unchanged.
> 
> false is a security hazard, as secure-coding-practices.rst points out.
> I think it should be pointed out right here as well.  Especially since
> "security hazard" is the default!
> 
> I'd do it in a separate patch, but I'm a compulsive patch splitter.
> 

I agree, we need to warn about this unexpected behavior.

Re: [PATCH v2 1/2] block/null: Report DATA if not reading zeroes

[Nir Soffer]

> On 8 May 2025, at 7:37, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Nir Soffer <nirsof@gmail.com> writes:
> 
>> If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
>> BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
>> confuse users or other programs:
>> 
>>    % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>>    [{ "start": 0, "length": 1073741824, "depth": 0, "present": false, "zero": false, "data": false, "compressed": false}]
>> 
>>    % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>> 
>>    % nbdinfo --map nbd://127.0.0.1
>>             0  1073741824    1  hole
>> 
>> With this change we report DATA in this case:
>> 
>>    % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>>    [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": 0}]
>> 
>>    % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>> 
>>    % nbdinfo --map nbd://127.0.0.1
>>             0  1073741824    0  data
>> 
>> Signed-off-by: Nir Soffer <nirsof@gmail.com>
>> ---
>> block/null.c         | 4 +---
>> qapi/block-core.json | 5 +++--
>> 2 files changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/block/null.c b/block/null.c
>> index dc0b1fdbd9..7ba87bd9a9 100644
>> --- a/block/null.c
>> +++ b/block/null.c
>> @@ -239,9 +239,7 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
>>     *map = offset;
>>     *file = bs;
>> 
>> -    if (s->read_zeroes) {
>> -        ret |= BDRV_BLOCK_ZERO;
>> -    }
>> +    ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
>>     return ret;
>> }
>> 
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index b1937780e1..7c95c9e36a 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3293,8 +3293,9 @@
>> #     requests.  Default to zero which completes requests immediately.
>> #     (Since 2.4)
>> #
>> -# @read-zeroes: if true, reads from the device produce zeroes; if
>> -#     false, the buffer is left unchanged.
>> +# @read-zeroes: if true, emulate a sparse image, and reads from the
>> +#     device produce zeroes; if false, emulate an allocated image but
>> +#     reads from the device leave the buffer unchanged.
>> #     (default: false; since: 4.1)
>> #
>> # Since: 2.9
> 
> Possibly dumb question: how is this doc change related to the code fix?

Before this change we returned BDRV_BLOCK_ZERO if read-zeroes is true, and no flags when it was false.
Now we return BDRV_BLOCK_ZERO when read-zeroes is true, and BDRV_BLOCK_DATA when false.

In other words, we read-zeroes: true emulate a spare image, and read-zeros: false an allocated image.

> 
> Suggest to split the sentence for easier reading:
> 
>   # @read-zeroes: If true, emulate a sparse image, and reads from the
>   #     device produce zeroes.  If false, emulate an allocated image,
>   #     but reads from the device leave the buffer unchanged.
> 

Sure, it looks better.

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

[Nir Soffer]

When the `read-zeroes` is set, reads produce zeroes, and block status
return BDRV_BLOCK_ZERO, emulating a sparse image.

If we don't set `read-zeros` we report BDRV_BLOCK_DATA, but image data
is undefined; posix_memalign, _aligned_malloc, valloc, or memalign do
not promise to zero allocated memory.

When computing a blkhash of an image via qemu-nbd, we want to test 3
cases:

1. Sparse image: skip reading the entire image based on block status
   result, and use a pre-computed zero block hash.
2. Image full of zeroes: read the entire image, detect block full of
   zeroes and skip block hash computation.
3. Image full of data: read the entire image and compute a hash of all
   blocks.

This change adds `read-pattern` option. If the option is set, reads
produce the specified pattern. With this option we can emulate an image
full of zeroes or full of non-zeroes.

The following examples shows how the new option can be used with blksum
(or nbdcopy --blkhash) to compute a blkhash of an image using the
null-co driver.

Sparse image - the very fast path:

    % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/sparse.sock \
        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-zeroes': true}}" &

    % time blksum 'nbd+unix:///?socket=/tmp/sparse.sock'
    300ad1efddb063822fea65ae3174cd35320939d4d0b050613628c6e1e876f8f6  nbd+unix:///?socket=/tmp/sparse.sock
    blksum 'nbd+unix:///?socket=/tmp/sparse.sock'  0.05s user 0.01s system 92% cpu 0.061 total

Image full of zeros - same hash, 268 times slower:

    % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/zero.sock \
        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-pattern': 0}}" &

    % time blksum 'nbd+unix:///?socket=/tmp/zero.sock'
    300ad1efddb063822fea65ae3174cd35320939d4d0b050613628c6e1e876f8f6  nbd+unix:///?socket=/tmp/zero.sock
    blksum 'nbd+unix:///?socket=/tmp/zero.sock'  7.45s user 22.57s system 183% cpu 16.347 total

Image full of data - difference hash, heavy cpu usage:

    % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/data.sock \
        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-pattern': 255}}" &

    % time blksum 'nbd+unix:///?socket=/tmp/data.sock'
    2c122b3ed28c83ede3c08485659fa9b56ee54ba1751db74d8ba9aa13d9866432  nbd+unix:///?socket=/tmp/data.sock
    blksum 'nbd+unix:///?socket=/tmp/data.sock'  46.05s user 14.15s system 448% cpu 13.414 total

Specifying both `read-zeroes` and `read-pattern` is an error since
`read-zeroes` implies a sparse image. Example errors:

    % ./qemu-img map --output json \
        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'read-pattern': -1}}"
    qemu-img: Could not open 'json:{...}': read_pattern is out of range (0-255)

    % ./qemu-img map --output json \
        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'read-pattern': 0, 'read-zeroes': true}}"
    qemu-img: Could not open 'json:{...}': The parameters read-zeroes and read-pattern are in conflict

Tested on top of
https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg05096.html.

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 block/null.c                           | 34 +++++++++++++++++++++++++-
 docs/devel/secure-coding-practices.rst |  3 ++-
 qapi/block-core.json                   | 14 +++++++++--
 3 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/block/null.c b/block/null.c
index 7ba87bd9a9..62c1da2b07 100644
--- a/block/null.c
+++ b/block/null.c
@@ -22,11 +22,14 @@
 
 #define NULL_OPT_LATENCY "latency-ns"
 #define NULL_OPT_ZEROES  "read-zeroes"
+#define NULL_OPT_PATTERN  "read-pattern"
 
 typedef struct {
     int64_t length;
     int64_t latency_ns;
     bool read_zeroes;
+    bool has_read_pattern;
+    int read_pattern;
 } BDRVNullState;
 
 static QemuOptsList runtime_opts = {
@@ -49,6 +52,11 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "return zeroes when read",
         },
+        {
+            .name = NULL_OPT_PATTERN,
+            .type = QEMU_OPT_NUMBER,
+            .help = "return pattern when read",
+        },
         { /* end of list */ }
     },
 };
@@ -85,6 +93,7 @@ static int null_open(BlockDriverState *bs, QDict *options, int flags,
     int ret = 0;
 
     opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
+
     qemu_opts_absorb_qdict(opts, options, &error_abort);
     s->length =
         qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
@@ -93,10 +102,28 @@ static int null_open(BlockDriverState *bs, QDict *options, int flags,
     if (s->latency_ns < 0) {
         error_setg(errp, "latency-ns is invalid");
         ret = -EINVAL;
+        goto out;
     }
     s->read_zeroes = qemu_opt_get_bool(opts, NULL_OPT_ZEROES, false);
-    qemu_opts_del(opts);
+    s->has_read_pattern = qemu_opt_find(opts, NULL_OPT_PATTERN) != NULL;
+    if (s->has_read_pattern) {
+        if (s->read_zeroes) {
+            error_setg(errp, "The parameters read-zeroes and read-pattern "
+                             "are in conflict");
+            ret = -EINVAL;
+            goto out;
+        }
+        s->read_pattern = qemu_opt_get_number(opts, NULL_OPT_PATTERN, 0);
+        if (s->read_pattern < 0 || s->read_pattern > UINT8_MAX) {
+            error_setg(errp, "read_pattern is out of range (0-%d)", UINT8_MAX);
+            ret = -EINVAL;
+            goto out;
+        }
+    }
     bs->supported_write_flags = BDRV_REQ_FUA;
+
+out:
+    qemu_opts_del(opts);
     return ret;
 }
 
@@ -125,6 +152,8 @@ static coroutine_fn int null_co_preadv(BlockDriverState *bs,
 
     if (s->read_zeroes) {
         qemu_iovec_memset(qiov, 0, 0, bytes);
+    } else if (s->has_read_pattern) {
+        qemu_iovec_memset(qiov, 0, s->read_pattern, bytes);
     }
 
     return null_co_common(bs);
@@ -199,6 +228,8 @@ static BlockAIOCB *null_aio_preadv(BlockDriverState *bs,
 
     if (s->read_zeroes) {
         qemu_iovec_memset(qiov, 0, 0, bytes);
+    } else if (s->has_read_pattern) {
+        qemu_iovec_memset(qiov, 0, s->read_pattern, bytes);
     }
 
     return null_aio_common(bs, cb, opaque);
@@ -272,6 +303,7 @@ null_co_get_allocated_file_size(BlockDriverState *bs)
 static const char *const null_strong_runtime_opts[] = {
     BLOCK_OPT_SIZE,
     NULL_OPT_ZEROES,
+    NULL_OPT_PATTERN,
 
     NULL
 };
diff --git a/docs/devel/secure-coding-practices.rst b/docs/devel/secure-coding-practices.rst
index 0454cc527e..73830684ea 100644
--- a/docs/devel/secure-coding-practices.rst
+++ b/docs/devel/secure-coding-practices.rst
@@ -111,5 +111,6 @@ Use of null-co block drivers
 The ``null-co`` block driver is designed for performance: its read accesses are
 not initialized by default. In case this driver has to be used for security
 research, it must be used with the ``read-zeroes=on`` option which fills read
-buffers with zeroes. Security issues reported with the default
+buffers with zeroes, or with the ``read-pattern=N`` option which fills read
+buffers with pattern. Security issues reported with the default
 (``read-zeroes=off``) will be discarded.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7c95c9e36a..2205ac9758 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3295,13 +3295,23 @@
 #
 # @read-zeroes: if true, emulate a sparse image, and reads from the
 #     device produce zeroes; if false, emulate an allocated image but
-#     reads from the device leave the buffer unchanged.
+#     reads from the device leave the buffer unchanged. Mutually
+#     exclusive with @read-pattern.
 #     (default: false; since: 4.1)
 #
+# @read-pattern: if set, emulate an allocated image, and reads from the
+#     device produce the specified byte value; if unset, reads from the
+#     device leave the buffer unchanged. Mutually exclusive with
+#     @read-zeroes.
+#     (since: 10.1)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsNull',
-  'data': { '*size': 'int', '*latency-ns': 'uint64', '*read-zeroes': 'bool' } }
+  'data': { '*size': 'int',
+            '*latency-ns': 'uint64',
+            '*read-zeroes': 'bool',
+            '*read-pattern': 'uint8' } }
 
 ##
 # @BlockdevOptionsNVMe:
-- 
2.39.5 (Apple Git-154)

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

[Markus Armbruster]

Nir Soffer <nirsof@gmail.com> writes:

> When the `read-zeroes` is set, reads produce zeroes, and block status
> return BDRV_BLOCK_ZERO, emulating a sparse image.
>
> If we don't set `read-zeros` we report BDRV_BLOCK_DATA, but image data
> is undefined; posix_memalign, _aligned_malloc, valloc, or memalign do
> not promise to zero allocated memory.
>
> When computing a blkhash of an image via qemu-nbd, we want to test 3
> cases:
>
> 1. Sparse image: skip reading the entire image based on block status
>    result, and use a pre-computed zero block hash.
> 2. Image full of zeroes: read the entire image, detect block full of
>    zeroes and skip block hash computation.
> 3. Image full of data: read the entire image and compute a hash of all
>    blocks.
>
> This change adds `read-pattern` option. If the option is set, reads
> produce the specified pattern. With this option we can emulate an image
> full of zeroes or full of non-zeroes.
>
> The following examples shows how the new option can be used with blksum
> (or nbdcopy --blkhash) to compute a blkhash of an image using the
> null-co driver.
>
> Sparse image - the very fast path:
>
>     % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/sparse.sock \
>         "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-zeroes': true}}" &
>
>     % time blksum 'nbd+unix:///?socket=/tmp/sparse.sock'
>     300ad1efddb063822fea65ae3174cd35320939d4d0b050613628c6e1e876f8f6  nbd+unix:///?socket=/tmp/sparse.sock
>     blksum 'nbd+unix:///?socket=/tmp/sparse.sock'  0.05s user 0.01s system 92% cpu 0.061 total
>
> Image full of zeros - same hash, 268 times slower:
>
>     % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/zero.sock \
>         "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-pattern': 0}}" &
>
>     % time blksum 'nbd+unix:///?socket=/tmp/zero.sock'
>     300ad1efddb063822fea65ae3174cd35320939d4d0b050613628c6e1e876f8f6  nbd+unix:///?socket=/tmp/zero.sock
>     blksum 'nbd+unix:///?socket=/tmp/zero.sock'  7.45s user 22.57s system 183% cpu 16.347 total
>
> Image full of data - difference hash, heavy cpu usage:
>
>     % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/data.sock \
>         "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-pattern': 255}}" &
>
>     % time blksum 'nbd+unix:///?socket=/tmp/data.sock'
>     2c122b3ed28c83ede3c08485659fa9b56ee54ba1751db74d8ba9aa13d9866432  nbd+unix:///?socket=/tmp/data.sock
>     blksum 'nbd+unix:///?socket=/tmp/data.sock'  46.05s user 14.15s system 448% cpu 13.414 total
>
> Specifying both `read-zeroes` and `read-pattern` is an error since
> `read-zeroes` implies a sparse image. Example errors:
>
>     % ./qemu-img map --output json \
>         "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'read-pattern': -1}}"
>     qemu-img: Could not open 'json:{...}': read_pattern is out of range (0-255)
>
>     % ./qemu-img map --output json \
>         "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'read-pattern': 0, 'read-zeroes': true}}"
>     qemu-img: Could not open 'json:{...}': The parameters read-zeroes and read-pattern are in conflict
>
> Tested on top of
> https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg05096.html.
>
> Signed-off-by: Nir Soffer <nirsof@gmail.com>

[...]

> diff --git a/docs/devel/secure-coding-practices.rst b/docs/devel/secure-coding-practices.rst
> index 0454cc527e..73830684ea 100644
> --- a/docs/devel/secure-coding-practices.rst
> +++ b/docs/devel/secure-coding-practices.rst
> @@ -111,5 +111,6 @@ Use of null-co block drivers
>  The ``null-co`` block driver is designed for performance: its read accesses are
>  not initialized by default. In case this driver has to be used for security
>  research, it must be used with the ``read-zeroes=on`` option which fills read
> -buffers with zeroes. Security issues reported with the default
> +buffers with zeroes, or with the ``read-pattern=N`` option which fills read
> +buffers with pattern. Security issues reported with the default
>  (``read-zeroes=off``) will be discarded.
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7c95c9e36a..2205ac9758 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3295,13 +3295,23 @@
>  #
>  # @read-zeroes: if true, emulate a sparse image, and reads from the
>  #     device produce zeroes; if false, emulate an allocated image but
> -#     reads from the device leave the buffer unchanged.
> +#     reads from the device leave the buffer unchanged. Mutually
> +#     exclusive with @read-pattern.
>  #     (default: false; since: 4.1)

Correct before the patch: absent behaves just like present and false.

That's no longer the case afterwards.  Hmm.

>  #
> +# @read-pattern: if set, emulate an allocated image, and reads from the
> +#     device produce the specified byte value; if unset, reads from the
> +#     device leave the buffer unchanged. Mutually exclusive with
> +#     @read-zeroes.
> +#     (since: 10.1)

Default?  The usual way to document it would be something like (default:
false), but that's again problematic.

> +#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsNull',
> -  'data': { '*size': 'int', '*latency-ns': 'uint64', '*read-zeroes': 'bool' } }
> +  'data': { '*size': 'int',
> +            '*latency-ns': 'uint64',
> +            '*read-zeroes': 'bool',
> +            '*read-pattern': 'uint8' } }
>  
>  ##
>  # @BlockdevOptionsNVMe:

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

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?

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

[Eric Blake]

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).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

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

[Markus Armbruster]

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.

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

[Nir Soffer]

> On 8 May 2025, at 8:28, Markus Armbruster <armbru@redhat.com> wrote:
> 
> Nir Soffer <nirsof@gmail.com> writes:
> 
>> When the `read-zeroes` is set, reads produce zeroes, and block status
>> return BDRV_BLOCK_ZERO, emulating a sparse image.
>> 
>> If we don't set `read-zeros` we report BDRV_BLOCK_DATA, but image data
>> is undefined; posix_memalign, _aligned_malloc, valloc, or memalign do
>> not promise to zero allocated memory.
>> 
>> When computing a blkhash of an image via qemu-nbd, we want to test 3
>> cases:
>> 
>> 1. Sparse image: skip reading the entire image based on block status
>>   result, and use a pre-computed zero block hash.
>> 2. Image full of zeroes: read the entire image, detect block full of
>>   zeroes and skip block hash computation.
>> 3. Image full of data: read the entire image and compute a hash of all
>>   blocks.
>> 
>> This change adds `read-pattern` option. If the option is set, reads
>> produce the specified pattern. With this option we can emulate an image
>> full of zeroes or full of non-zeroes.
>> 
>> The following examples shows how the new option can be used with blksum
>> (or nbdcopy --blkhash) to compute a blkhash of an image using the
>> null-co driver.
>> 
>> Sparse image - the very fast path:
>> 
>>    % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/sparse.sock \
>>        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-zeroes': true}}" &
>> 
>>    % time blksum 'nbd+unix:///?socket=/tmp/sparse.sock'
>>    300ad1efddb063822fea65ae3174cd35320939d4d0b050613628c6e1e876f8f6  nbd+unix:///?socket=/tmp/sparse.sock
>>    blksum 'nbd+unix:///?socket=/tmp/sparse.sock'  0.05s user 0.01s system 92% cpu 0.061 total
>> 
>> Image full of zeros - same hash, 268 times slower:
>> 
>>    % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/zero.sock \
>>        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-pattern': 0}}" &
>> 
>>    % time blksum 'nbd+unix:///?socket=/tmp/zero.sock'
>>    300ad1efddb063822fea65ae3174cd35320939d4d0b050613628c6e1e876f8f6  nbd+unix:///?socket=/tmp/zero.sock
>>    blksum 'nbd+unix:///?socket=/tmp/zero.sock'  7.45s user 22.57s system 183% cpu 16.347 total
>> 
>> Image full of data - difference hash, heavy cpu usage:
>> 
>>    % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/data.sock \
>>        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-pattern': 255}}" &
>> 
>>    % time blksum 'nbd+unix:///?socket=/tmp/data.sock'
>>    2c122b3ed28c83ede3c08485659fa9b56ee54ba1751db74d8ba9aa13d9866432  nbd+unix:///?socket=/tmp/data.sock
>>    blksum 'nbd+unix:///?socket=/tmp/data.sock'  46.05s user 14.15s system 448% cpu 13.414 total
>> 
>> Specifying both `read-zeroes` and `read-pattern` is an error since
>> `read-zeroes` implies a sparse image. Example errors:
>> 
>>    % ./qemu-img map --output json \
>>        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'read-pattern': -1}}"
>>    qemu-img: Could not open 'json:{...}': read_pattern is out of range (0-255)
>> 
>>    % ./qemu-img map --output json \
>>        "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'read-pattern': 0, 'read-zeroes': true}}"
>>    qemu-img: Could not open 'json:{...}': The parameters read-zeroes and read-pattern are in conflict
>> 
>> Tested on top of
>> https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg05096.html.
>> 
>> Signed-off-by: Nir Soffer <nirsof@gmail.com>
> 
> [...]
> 
>> diff --git a/docs/devel/secure-coding-practices.rst b/docs/devel/secure-coding-practices.rst
>> index 0454cc527e..73830684ea 100644
>> --- a/docs/devel/secure-coding-practices.rst
>> +++ b/docs/devel/secure-coding-practices.rst
>> @@ -111,5 +111,6 @@ Use of null-co block drivers
>> The ``null-co`` block driver is designed for performance: its read accesses are
>> not initialized by default. In case this driver has to be used for security
>> research, it must be used with the ``read-zeroes=on`` option which fills read
>> -buffers with zeroes. Security issues reported with the default
>> +buffers with zeroes, or with the ``read-pattern=N`` option which fills read
>> +buffers with pattern. Security issues reported with the default
>> (``read-zeroes=off``) will be discarded.
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 7c95c9e36a..2205ac9758 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -3295,13 +3295,23 @@
>> #
>> # @read-zeroes: if true, emulate a sparse image, and reads from the
>> #     device produce zeroes; if false, emulate an allocated image but
>> -#     reads from the device leave the buffer unchanged.
>> +#     reads from the device leave the buffer unchanged. Mutually
>> +#     exclusive with @read-pattern.
>> #     (default: false; since: 4.1)
> 
> Correct before the patch: absent behaves just like present and false.
> 
> That's no longer the case afterwards.  Hmm.

I don’t get the issue

> 
>> #
>> +# @read-pattern: if set, emulate an allocated image, and reads from the
>> +#     device produce the specified byte value; if unset, reads from the
>> +#     device leave the buffer unchanged. Mutually exclusive with
>> +#     @read-zeroes.
>> +#     (since: 10.1)
> 
> Default?  The usual way to document it would be something like (default:
> false), but that's again problematic.

This is optional, so we cannot have a default. Maybe (default absent)?

> 
>> +#
>> # Since: 2.9
>> ##
>> { 'struct': 'BlockdevOptionsNull',
>> -  'data': { '*size': 'int', '*latency-ns': 'uint64', '*read-zeroes': 'bool' } }
>> +  'data': { '*size': 'int',
>> +            '*latency-ns': 'uint64',
>> +            '*read-zeroes': 'bool',
>> +            '*read-pattern': 'uint8' } }
>> 
>> ##
>> # @BlockdevOptionsNVMe:
> 
> 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

The names are short and nice, but we need to consider the entire API. "read-zeroes" works well with "detect-zeroes". "read-pattern" works well with "read-zeroes” and the term “pattern” works well with qemu-io -c:

    read [-abCqrv] [-P pattern [-s off] [-l len]] off len -- reads a number of bytes at a specified offset
    write [-bcCfnqruz] [-P pattern | -s source_file] off len -- writes a number of bytes at a specified offset

There is also nbdkit pattern plugin in this space:
https://libguestfs.org/nbdkit-pattern-plugin.1.html

> 
> 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

“sparse" conflicts with “contents”. I would not make both possible even if contents is 0. 

> 
> I'd make @sparse either mandatory

Do you mean must be always specified? Why?

> or default to true, so that we don't
> default to security hazard.

Default sparse sounds good, but it means that we must use:

    “sparse": false, “contents”: 42

So we must use the conflicting options together.

“sparse”: false is a better default, expect the unspecified bytes, but it should not depend on spareness.

> 
> 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?

Having "sparse": true default is little better than returning unspecified data by default, but sing “sparse”: false will return unspecified bytes and is not expected - spareness should not be related to security.

If we want to make this secure by default, maybe:

   bool sparse: emulate sparse image (default false)
   uint8 contents: read return specified bytes (default 0)
   bool insecure: read return unspecified bytes, for performance testing (default false)

Another way is to accept invalid value for unspecified content:

   bool sparse: emulate sparse image (default false)
   uint8 contents: read return specified bytes. -1 to return unspecified bytes (default 0)

This may be confusing, users may assume that -1 and 255 will give the same result.

Another way is to use lower level terms:

    uint8 block-status: flags to return in block_status: (default 0x0, zero 0x1, data 0x2)
    uint8 read-pattern: return specified bytes. -1 to return unspecified bytes (default 0)

This is harder to use but allows more combinations that can be useful or testing NBD. And the default is secure.

But the important questions are:
- Do we want to keep read-zeros?
- Do we want to keep the insecure default or make insecure behavior explicit?