Hi,

This is an alternative approach to changing null-co driver
default 'read-zeroes' option to true:
https://www.mail-archive.com/qemu-block@nongnu.org/msg80873.html

Instead we introduce yet another block driver with an explicit
name: 'zeroes-co'. We then clarify in secure-coding-practices.rst
that security reports have to be sent using this new driver.

The 2nd patch is RFC because I won't spend time converting the
tests until the first patch is discussed, as I already spent enough
time doing that in the previous mentioned series.

Regards,

Phil.

Philippe Mathieu-Daudé (3):
  block: Introduce the 'zeroes-co' driver
  tests/test-blockjob: Use zeroes-co instead of null-co,read-zeroes=on
  docs/secure-coding-practices: Describe null-co/zeroes-co block drivers

 docs/devel/secure-coding-practices.rst |   7 +
 block/zeroes.c                         | 306 +++++++++++++++++++++++++
 tests/test-blockjob.c                  |   4 +-
 block/meson.build                      |   1 +
 4 files changed, 315 insertions(+), 3 deletions(-)
 create mode 100644 block/zeroes.c

-- 
2.26.2

On Wed, Mar 10, 2021 at 12:43:11PM +0100, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> This is an alternative approach to changing null-co driver
> default 'read-zeroes' option to true:
> https://www.mail-archive.com/qemu-block@nongnu.org/msg80873.html
> 
> Instead we introduce yet another block driver with an explicit
> name: 'zeroes-co'. We then clarify in secure-coding-practices.rst
> that security reports have to be sent using this new driver.

IMHO introducing a new block driver, when this can be solved by
simply adding a property to the existing driver, just feels mad
Your previous series made much more sense, and despite the long
thread, I didn't see anyone suggest a real world blocker with
making it read zeros by default.

I think Max's last mail sums it up pretty well

  https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg07173.html

[quote]
In cases where we have a test that just wants a simple block node that
doesn’t use disk space, the memset() can’t be noticeable. But it’s just
a test, so do we even need the memset()? Strictly speaking, perhaps not,
but if someone is to run it via Valgrind or something, they may get
false positives, so just doing the memset() is the right thing to do.

For performance tests, it must be possible to set read-zeroes=off,
because even though “that memset() isn’t noticeable in a functional
test”, in a hard-core performance test, it will be.

So we need a switch. It should default to memset(), because (1) making
tools like Valgrind happy seems like a reasonable objective to me, and
(2) in the majority of cases, the memset() cannot have a noticeable
impact.
[/quote]


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 3/10/21 12:55 PM, Daniel P. Berrangé wrote:
> On Wed, Mar 10, 2021 at 12:43:11PM +0100, Philippe Mathieu-Daudé wrote:
>> Hi,
>>
>> This is an alternative approach to changing null-co driver
>> default 'read-zeroes' option to true:
>> https://www.mail-archive.com/qemu-block@nongnu.org/msg80873.html
>>
>> Instead we introduce yet another block driver with an explicit
>> name: 'zeroes-co'. We then clarify in secure-coding-practices.rst
>> that security reports have to be sent using this new driver.
> 
> IMHO introducing a new block driver, when this can be solved by
> simply adding a property to the existing driver, just feels mad
> Your previous series made much more sense, and despite the long
> thread, I didn't see anyone suggest a real world blocker with
> making it read zeros by default.
> 
> I think Max's last mail sums it up pretty well
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg07173.html
> 
> [quote]
> In cases where we have a test that just wants a simple block node that
> doesn’t use disk space, the memset() can’t be noticeable. But it’s just
> a test, so do we even need the memset()? Strictly speaking, perhaps not,
> but if someone is to run it via Valgrind or something, they may get
> false positives, so just doing the memset() is the right thing to do.
> 
> For performance tests, it must be possible to set read-zeroes=off,
> because even though “that memset() isn’t noticeable in a functional
> test”, in a hard-core performance test, it will be.
> 
> So we need a switch. It should default to memset(), because (1) making
> tools like Valgrind happy seems like a reasonable objective to me, and
> (2) in the majority of cases, the memset() cannot have a noticeable
> impact.
> [/quote]

Yes I totally agree with Max, but Fam is the maintainer.

I'll keep looking for alternatives. Maybe simply documentation.

Thanks,

Phil.

On Wed, 10 Mar 2021 at 11:44, Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Hi,
>
> This is an alternative approach to changing null-co driver
> default 'read-zeroes' option to true:
> https://www.mail-archive.com/qemu-block@nongnu.org/msg80873.html
>
> Instead we introduce yet another block driver with an explicit
> name: 'zeroes-co'. We then clarify in secure-coding-practices.rst
> that security reports have to be sent using this new driver.
>
> The 2nd patch is RFC because I won't spend time converting the
> tests until the first patch is discussed, as I already spent enough
> time doing that in the previous mentioned series.
>
> Regards,
>
> Phil.
>
> Philippe Mathieu-Daudé (3):
>   block: Introduce the 'zeroes-co' driver
>   tests/test-blockjob: Use zeroes-co instead of null-co,read-zeroes=on
>   docs/secure-coding-practices: Describe null-co/zeroes-co block drivers
>
>  docs/devel/secure-coding-practices.rst |   7 +
>  block/zeroes.c                         | 306 +++++++++++++++++++++++++
>


Why not add another BlockDriver struct to block/null.c and set the
read_zeroes field in the .bdrv_file_open callback? It would make the patch
much simpler.

Fam


>  tests/test-blockjob.c                  |   4 +-
>  block/meson.build                      |   1 +
>  4 files changed, 315 insertions(+), 3 deletions(-)
>  create mode 100644 block/zeroes.c
>
> --
> 2.26.2
>
>
>
>

On 3/10/21 1:32 PM, Fam Zheng wrote:
> On Wed, 10 Mar 2021 at 11:44, Philippe Mathieu-Daudé <philmd@redhat.com
> <mailto:philmd@redhat.com>> wrote:
> 
>     Hi,
> 
>     This is an alternative approach to changing null-co driver
>     default 'read-zeroes' option to true:
>     https://www.mail-archive.com/qemu-block@nongnu.org/msg80873.html
>     <https://www.mail-archive.com/qemu-block@nongnu.org/msg80873.html>
> 
>     Instead we introduce yet another block driver with an explicit
>     name: 'zeroes-co'. We then clarify in secure-coding-practices.rst
>     that security reports have to be sent using this new driver.
> 
>     The 2nd patch is RFC because I won't spend time converting the
>     tests until the first patch is discussed, as I already spent enough
>     time doing that in the previous mentioned series.
> 
>     Regards,
> 
>     Phil.
> 
>     Philippe Mathieu-Daudé (3):
>       block: Introduce the 'zeroes-co' driver
>       tests/test-blockjob: Use zeroes-co instead of null-co,read-zeroes=on
>       docs/secure-coding-practices: Describe null-co/zeroes-co block drivers
> 
>      docs/devel/secure-coding-practices.rst |   7 +
>      block/zeroes.c                         | 306 +++++++++++++++++++++++++
> 
> Why not add another BlockDriver struct to block/null.c and set the
> read_zeroes field in the .bdrv_file_open callback? It would make the
> patch much simpler.

This is the first patch I wrote but then realized you are listed as
null-co maintainer, so you might be uninterested in having this new
driver in the same file. And declaring the prototypes public to
reuse them seemed overkill.

Anyway I'll try to find a simpler outcome by simply improving
documentation.

On Wed, 10 Mar 2021 at 12:38, Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> On 3/10/21 1:32 PM, Fam Zheng wrote:
> > On Wed, 10 Mar 2021 at 11:44, Philippe Mathieu-Daudé <philmd@redhat.com
> > <mailto:philmd@redhat.com>> wrote:
> >
> >     Hi,
> >
> >     This is an alternative approach to changing null-co driver
> >     default 'read-zeroes' option to true:
> >     https://www.mail-archive.com/qemu-block@nongnu.org/msg80873.html
> >     <https://www.mail-archive.com/qemu-block@nongnu.org/msg80873.html>
> >
> >     Instead we introduce yet another block driver with an explicit
> >     name: 'zeroes-co'. We then clarify in secure-coding-practices.rst
> >     that security reports have to be sent using this new driver.
> >
> >     The 2nd patch is RFC because I won't spend time converting the
> >     tests until the first patch is discussed, as I already spent enough
> >     time doing that in the previous mentioned series.
> >
> >     Regards,
> >
> >     Phil.
> >
> >     Philippe Mathieu-Daudé (3):
> >       block: Introduce the 'zeroes-co' driver
> >       tests/test-blockjob: Use zeroes-co instead of
> null-co,read-zeroes=on
> >       docs/secure-coding-practices: Describe null-co/zeroes-co block
> drivers
> >
> >      docs/devel/secure-coding-practices.rst |   7 +
> >      block/zeroes.c                         | 306
> +++++++++++++++++++++++++
> >
> > Why not add another BlockDriver struct to block/null.c and set the
> > read_zeroes field in the .bdrv_file_open callback? It would make the
> > patch much simpler.
>
> This is the first patch I wrote but then realized you are listed as
> null-co maintainer, so you might be uninterested in having this new
> driver in the same file. And declaring the prototypes public to
> reuse them seemed overkill.
>
>
I posted a patch for block/null.c to add the same interface, just as an
alternative to the first patch.

I think we all agree that both zeroed and non-zeroed read mode will be
supported going forward, the divergence is on approach:

a) -blockdev null-co:// | -blockdev null-co://,read-zeroes=off,

if we make read-zeroes=on the default.

b) -blockdev null-co:// | -blockdev zero-co://,

if we keep the current default of null-co://, but introduce zero-co://
which has a clearer interface.

Fam