Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Max Reitz]

On 10.03.21 17:35, Fam Zheng wrote:



On Wed, 10 Mar 2021 at 15:02, Max Reitz > wrote:


On 10.03.21 15:17, f...@euphon.net  wrote:
 > From: Fam Zheng mailto:famzh...@amazon.com>>
 >
 > null-co:// has a read-zeroes=off default, when used to in security
 > analysis, this can cause false positives because the driver doesn't
 > write to the read buffer.
 >
 > null-co:// has the highest possible performance as a block driver, so
 > let's keep it that way. This patch introduces zero-co:// and
 > zero-aio://, largely similar with null-*://, but have
read-zeroes=on by
 > default, so it's more suitable in cases than null-co://.
 >
 > Signed-off-by: Fam Zheng mailto:f...@euphon.net>>
 > ---
 >   block/null.c | 91

 >   1 file changed, 91 insertions(+)

You’ll also need to make all tests that currently use null-{co,aio} use
zero-{co,aio}, because none of those are performance tests (as far as
I’m aware), so they all want a block driver that memset()s.

(And that’s basically my problem with this approach; nearly everyone
who
uses null-* right now probably wants read-zeroes=on, so keeping null-*
as it is means all of those users should be changed.  Sure, they were
all wrong to not specify read-zeroes=on, but that’s how it is.  So
while
technically this patch is a compatible change, in contrast to the one
making read-zeroes=on the default, in practice it absolutely isn’t.)

Another problem arising from that is I can imagine that some
distributions might have whitelisted null-co because many iotests
implicitly depend on it, so the iotests will fail if they aren’t
whitelisted.  Now they’d need to whitelist zero-co, too.  Not
impossible, sure, but it’s work that would need to be done.


My problem is this: We have a not-really problem, namely “valgrind and
other tools complain”.  Philippe (and I guess me on IRC) proposed a
simple solution whose only drawbacks (AFAIU) are:

(1) When writing performance tests, you’ll then need to explicitly
specify read-zeroes=off.  Existing performance tests using null-co will
probably wrongly show degredation.  (Are there such tests, though?)

(2) null will not quite conform to its name, strictly speaking.
Frankly, I don’t know who’d care other than people who write those
performance tests mentioned in (1).  I know I don’t care.

(Technically: (3) We should look into all qemu tests that use
null-co to
see whether they test performance.  In practice, they don’t, so we
don’t
need to.)

So AFAIU change the read-zeroes default would affect very few
people, if
any.  I see you care about (2), and you’re the maintainer, so I can’t
say that there is no problem.  But it isn’t a practical one.

So on the practical front, we get a small benefit (tools won’t
complain)
for a small drawback (having to remember read-zeroes=off for
performance
tests).


Now you propose a change that has the following drawbacks, as I see it:

(1) All non-performance tests using null-* should be changed to zero-*.
   And those are quite a few tests, so this is some work.

(2) Distributions that have whitelisted null-co now should whitelist
zero-co, too.

Not impossible, but I consider these much bigger practical drawbacks
than making read-zeroes=on the default.  It’s actual work that must be
done.  To me personally, these drawbacks far outweigh the benefit of
having valgrind and other tools not complain.


I can’t stop you from updating this patch to do (1), but it doesn’t
make
sense to me from a practical perspective.  It just doesn’t seem
worth it
to me.


But using null-co:// in tests is not wrong, the problem is the caller 
failed to initialize its buffer.


Then I don’t see why we’d need zero-co://.

Max

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Kevin Wolf]

Am 10.03.2021 um 15:17 hat f...@euphon.net geschrieben:
> From: Fam Zheng 
> 
> null-co:// has a read-zeroes=off default, when used to in security
> analysis, this can cause false positives because the driver doesn't
> write to the read buffer.
> 
> null-co:// has the highest possible performance as a block driver, so
> let's keep it that way. This patch introduces zero-co:// and
> zero-aio://, largely similar with null-*://, but have read-zeroes=on by
> default, so it's more suitable in cases than null-co://.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/null.c | 91 

If we're adding new block drivers, there should certainly be some QAPI
schema updates here.

Kevin

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Fam Zheng]

On Wed, 10 Mar 2021 at 15:02, Max Reitz  wrote:

> On 10.03.21 15:17, f...@euphon.net wrote:
> > From: Fam Zheng 
> >
> > null-co:// has a read-zeroes=off default, when used to in security
> > analysis, this can cause false positives because the driver doesn't
> > write to the read buffer.
> >
> > null-co:// has the highest possible performance as a block driver, so
> > let's keep it that way. This patch introduces zero-co:// and
> > zero-aio://, largely similar with null-*://, but have read-zeroes=on by
> > default, so it's more suitable in cases than null-co://.
> >
> > Signed-off-by: Fam Zheng 
> > ---
> >   block/null.c | 91 
> >   1 file changed, 91 insertions(+)
>
> You’ll also need to make all tests that currently use null-{co,aio} use
> zero-{co,aio}, because none of those are performance tests (as far as
> I’m aware), so they all want a block driver that memset()s.
>
> (And that’s basically my problem with this approach; nearly everyone who
> uses null-* right now probably wants read-zeroes=on, so keeping null-*
> as it is means all of those users should be changed.  Sure, they were
> all wrong to not specify read-zeroes=on, but that’s how it is.  So while
> technically this patch is a compatible change, in contrast to the one
> making read-zeroes=on the default, in practice it absolutely isn’t.)
>
> Another problem arising from that is I can imagine that some
> distributions might have whitelisted null-co because many iotests
> implicitly depend on it, so the iotests will fail if they aren’t
> whitelisted.  Now they’d need to whitelist zero-co, too.  Not
> impossible, sure, but it’s work that would need to be done.
>
>
> My problem is this: We have a not-really problem, namely “valgrind and
> other tools complain”.  Philippe (and I guess me on IRC) proposed a
> simple solution whose only drawbacks (AFAIU) are:
>
> (1) When writing performance tests, you’ll then need to explicitly
> specify read-zeroes=off.  Existing performance tests using null-co will
> probably wrongly show degredation.  (Are there such tests, though?)
>
> (2) null will not quite conform to its name, strictly speaking.
> Frankly, I don’t know who’d care other than people who write those
> performance tests mentioned in (1).  I know I don’t care.
>
> (Technically: (3) We should look into all qemu tests that use null-co to
> see whether they test performance.  In practice, they don’t, so we don’t
> need to.)
>
> So AFAIU change the read-zeroes default would affect very few people, if
> any.  I see you care about (2), and you’re the maintainer, so I can’t
> say that there is no problem.  But it isn’t a practical one.
>
> So on the practical front, we get a small benefit (tools won’t complain)
> for a small drawback (having to remember read-zeroes=off for performance
> tests).
>
>
> Now you propose a change that has the following drawbacks, as I see it:
>
> (1) All non-performance tests using null-* should be changed to zero-*.
>   And those are quite a few tests, so this is some work.
>
> (2) Distributions that have whitelisted null-co now should whitelist
> zero-co, too.
>
> Not impossible, but I consider these much bigger practical drawbacks
> than making read-zeroes=on the default.  It’s actual work that must be
> done.  To me personally, these drawbacks far outweigh the benefit of
> having valgrind and other tools not complain.
>
>
> I can’t stop you from updating this patch to do (1), but it doesn’t make
> sense to me from a practical perspective.  It just doesn’t seem worth it
> to me.
>

But using null-co:// in tests is not wrong, the problem is the caller
failed to initialize its buffer. IMO the valgrind issue should really be
fixed like this:

diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c
index dcbccee294..9078718445 100644
--- a/hw/block/hd-geometry.c
+++ b/hw/block/hd-geometry.c
@@ -55,7 +55,7 @@ struct partition {
static int guess_disk_lchs(BlockBackend *blk,
   int *pcylinders, int *pheads, int *psectors)
{
-uint8_t buf[BDRV_SECTOR_SIZE];
+uint8_t buf[BDRV_SECTOR_SIZE] = {};
int i, heads, sectors, cylinders;
struct partition *p;
uint32_t nr_sects;

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Max Reitz]

On 10.03.21 15:17, f...@euphon.net wrote:

From: Fam Zheng 

null-co:// has a read-zeroes=off default, when used to in security
analysis, this can cause false positives because the driver doesn't
write to the read buffer.

null-co:// has the highest possible performance as a block driver, so
let's keep it that way. This patch introduces zero-co:// and
zero-aio://, largely similar with null-*://, but have read-zeroes=on by
default, so it's more suitable in cases than null-co://.

Signed-off-by: Fam Zheng 
---
  block/null.c | 91 
  1 file changed, 91 insertions(+)


You’ll also need to make all tests that currently use null-{co,aio} use 
zero-{co,aio}, because none of those are performance tests (as far as 
I’m aware), so they all want a block driver that memset()s.


(And that’s basically my problem with this approach; nearly everyone who 
uses null-* right now probably wants read-zeroes=on, so keeping null-* 
as it is means all of those users should be changed.  Sure, they were 
all wrong to not specify read-zeroes=on, but that’s how it is.  So while 
technically this patch is a compatible change, in contrast to the one 
making read-zeroes=on the default, in practice it absolutely isn’t.)


Another problem arising from that is I can imagine that some 
distributions might have whitelisted null-co because many iotests 
implicitly depend on it, so the iotests will fail if they aren’t 
whitelisted.  Now they’d need to whitelist zero-co, too.  Not 
impossible, sure, but it’s work that would need to be done.



My problem is this: We have a not-really problem, namely “valgrind and 
other tools complain”.  Philippe (and I guess me on IRC) proposed a 
simple solution whose only drawbacks (AFAIU) are:


(1) When writing performance tests, you’ll then need to explicitly 
specify read-zeroes=off.  Existing performance tests using null-co will 
probably wrongly show degredation.  (Are there such tests, though?)


(2) null will not quite conform to its name, strictly speaking. 
Frankly, I don’t know who’d care other than people who write those 
performance tests mentioned in (1).  I know I don’t care.


(Technically: (3) We should look into all qemu tests that use null-co to 
see whether they test performance.  In practice, they don’t, so we don’t 
need to.)


So AFAIU change the read-zeroes default would affect very few people, if 
any.  I see you care about (2), and you’re the maintainer, so I can’t 
say that there is no problem.  But it isn’t a practical one.


So on the practical front, we get a small benefit (tools won’t complain) 
for a small drawback (having to remember read-zeroes=off for performance 
tests).



Now you propose a change that has the following drawbacks, as I see it:

(1) All non-performance tests using null-* should be changed to zero-*. 
 And those are quite a few tests, so this is some work.


(2) Distributions that have whitelisted null-co now should whitelist 
zero-co, too.


Not impossible, but I consider these much bigger practical drawbacks 
than making read-zeroes=on the default.  It’s actual work that must be 
done.  To me personally, these drawbacks far outweigh the benefit of 
having valgrind and other tools not complain.



I can’t stop you from updating this patch to do (1), but it doesn’t make 
sense to me from a practical perspective.  It just doesn’t seem worth it 
to me.


Max

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Fam Zheng]

On Wed, 10 Mar 2021 at 14:51, Philippe Mathieu-Daudé 
wrote:

> On 3/10/21 3:28 PM, Fam Zheng wrote:
> > On Wed, 10 Mar 2021 at 14:24, Philippe Mathieu-Daudé  > > wrote:
> >
> > On 3/10/21 3:17 PM, f...@euphon.net  wrote:
> > > From: Fam Zheng mailto:famzh...@amazon.com>>
> > >
> > > null-co:// has a read-zeroes=off default, when used to in security
> > > analysis, this can cause false positives because the driver doesn't
> > > write to the read buffer.
> > >
> > > null-co:// has the highest possible performance as a block driver,
> so
> > > let's keep it that way. This patch introduces zero-co:// and
> > > zero-aio://, largely similar with null-*://, but have
> > read-zeroes=on by
> > > default, so it's more suitable in cases than null-co://.
> >
> > Thanks!
> >
> > >
> > > Signed-off-by: Fam Zheng mailto:f...@euphon.net>>
> > > ---
> > >  block/null.c | 91
> > 
> > >  1 file changed, 91 insertions(+)
> >
> > > +static int zero_file_open(BlockDriverState *bs, QDict *options,
> > int flags,
> > > +  Error **errp)
> > > +{
> > > +QemuOpts *opts;
> > > +BDRVNullState *s = bs->opaque;
> > > +int ret = 0;
> > > +
> > > +opts = qemu_opts_create(_opts, NULL, 0, _abort);
> > > +qemu_opts_absorb_qdict(opts, options, _abort);
> > > +s->length =
> > > +qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
> >
> > Please do not use this magic default value, let's always explicit the
> > size.
> >
> > s->length = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> > if (s->length < 0) {
> > error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE);
> > ret = -EINVAL;
> > }
> >
> >
> > Doesn't that result in a bare
> >
> >   -blockdev zero-co://
> >
> > would have 0 byte in size?
>
> Yes, this will display an error.
>
> Maybe better something like:
>
> if (s->length == 0) {
> error_append_hint(errp, "Usage: zero-co://,size=NUM");
> error_setg(errp, "size must be specified");
> ret = -EINVAL;
> } else if (s->length < 0) {
> error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE);
> ret = -EINVAL;
> }
>
> >
> > I'm copying what null-co:// does today, and it's convenient for tests.
> > Why is a default size bad?
>
> We learned default are almost always bad because you can not get
> rid of them. Also because we have reports of errors when using
> floppy and another one (can't remember) with null-co because of
> invalid size and we have to explain "the default is 1GB, you have
> to explicit your size".
>

Yeah I think that makes sense. I'll revise.

Thanks,
Fam

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Fam Zheng]

On Wed, 10 Mar 2021 at 14:41, Vladimir Sementsov-Ogievskiy <
vsement...@virtuozzo.com> wrote:

> 10.03.2021 17:17, f...@euphon.net wrote:
> > From: Fam Zheng 
> >
> > null-co:// has a read-zeroes=off default, when used to in security
> > analysis, this can cause false positives because the driver doesn't
> > write to the read buffer.
> >
> > null-co:// has the highest possible performance as a block driver, so
> > let's keep it that way. This patch introduces zero-co:// and
> > zero-aio://, largely similar with null-*://, but have read-zeroes=on by
> > default, so it's more suitable in cases than null-co://.
> >
> > Signed-off-by: Fam Zheng 
> > ---
> >   block/null.c | 91 
> >   1 file changed, 91 insertions(+)
> >
> > diff --git a/block/null.c b/block/null.c
> > index cc9b1d4ea7..5de97e8fda 100644
> > --- a/block/null.c
> > +++ b/block/null.c
> > @@ -76,6 +76,30 @@ static void null_aio_parse_filename(const char
> *filename, QDict *options,
> >   }
> >   }
> >
> > +static void zero_co_parse_filename(const char *filename, QDict *options,
> > +   Error **errp)
> > +{
> > +/* This functions only exists so that a zero-co:// filename is
> accepted
> > + * with the zero-co driver. */
> > +if (strcmp(filename, "zero-co://")) {
> > +error_setg(errp, "The only allowed filename for this driver is "
> > + "'zero-co://'");
> > +return;
> > +}
> > +}
> > +
> > +static void zero_aio_parse_filename(const char *filename, QDict
> *options,
> > +Error **errp)
> > +{
> > +/* This functions only exists so that a zero-aio:// filename is
> accepted
> > + * with the zero-aio driver. */
> > +if (strcmp(filename, "zero-aio://")) {
> > +error_setg(errp, "The only allowed filename for this driver is "
> > + "'zero-aio://'");
> > +return;
> > +}
> > +}
> > +
> >   static int null_file_open(BlockDriverState *bs, QDict *options, int
> flags,
> > Error **errp)
> >   {
> > @@ -99,6 +123,29 @@ static int null_file_open(BlockDriverState *bs,
> QDict *options, int flags,
> >   return ret;
> >   }
> >
> > +static int zero_file_open(BlockDriverState *bs, QDict *options, int
> flags,
> > +  Error **errp)
> > +{
> > +QemuOpts *opts;
> > +BDRVNullState *s = bs->opaque;
> > +int ret = 0;
> > +
> > +opts = qemu_opts_create(_opts, NULL, 0, _abort);
> > +qemu_opts_absorb_qdict(opts, options, _abort);
> > +s->length =
> > +qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
> > +s->latency_ns =
> > +qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0);
> > +if (s->latency_ns < 0) {
> > +error_setg(errp, "latency-ns is invalid");
> > +ret = -EINVAL;
> > +}
> > +s->read_zeroes = true;
> > +qemu_opts_del(opts);
> > +bs->supported_write_flags = BDRV_REQ_FUA;
> > +return ret;
> > +}
> > +
> >   static int64_t null_getlength(BlockDriverState *bs)
> >   {
> >   BDRVNullState *s = bs->opaque;
> > @@ -316,10 +363,54 @@ static BlockDriver bdrv_null_aio = {
> >   .strong_runtime_opts= null_strong_runtime_opts,
> >   };
> >
> > +static BlockDriver bdrv_zero_co = {
> > +.format_name= "zero-co",
> > +.protocol_name  = "zero-co",
> > +.instance_size  = sizeof(BDRVNullState),
> > +
> > +.bdrv_file_open = zero_file_open,
> > +.bdrv_parse_filename= zero_co_parse_filename,
> > +.bdrv_getlength = null_getlength,
> > +.bdrv_get_allocated_file_size = null_allocated_file_size,
> > +
> > +.bdrv_co_preadv = null_co_preadv,
> > +.bdrv_co_pwritev= null_co_pwritev,
> > +.bdrv_co_flush_to_disk  = null_co_flush,
> > +.bdrv_reopen_prepare= null_reopen_prepare,
> > +
> > +.bdrv_co_block_status   = null_co_block_status,
> > +
> > +.bdrv_refresh_filename  = null_refresh_filename,
> > +.strong_runtime_opts= null_strong_runtime_opts,
> > +};
> > +
> > +static BlockDriver bdrv_zero_aio = {
> > +.format_name= "zero-aio",
> > +.protocol_name  = "zero-aio",
> > +.instance_size  = sizeof(BDRVNullState),
> > +
> > +.bdrv_file_open = zero_file_open,
> > +.bdrv_parse_filename= zero_aio_parse_filename,
> > +.bdrv_getlength = null_getlength,
> > +.bdrv_get_allocated_file_size = null_allocated_file_size,
> > +
> > +.bdrv_aio_preadv= null_aio_preadv,
> > +.bdrv_aio_pwritev   = null_aio_pwritev,
> > +.bdrv_aio_flush = null_aio_flush,
> > +.bdrv_reopen_prepare= null_reopen_prepare,
> > +
> > +.bdrv_co_block_status   = null_co_block_status,
> > +
> > +.bdrv_refresh_filename  = null_refresh_filename,
> > +.strong_runtime_opts= null_strong_runtime_opts,
> > +};
>
> I don't 

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Philippe Mathieu-Daudé]

On 3/10/21 3:28 PM, Fam Zheng wrote:
> On Wed, 10 Mar 2021 at 14:24, Philippe Mathieu-Daudé  > wrote:
> 
> On 3/10/21 3:17 PM, f...@euphon.net  wrote:
> > From: Fam Zheng mailto:famzh...@amazon.com>>
> >
> > null-co:// has a read-zeroes=off default, when used to in security
> > analysis, this can cause false positives because the driver doesn't
> > write to the read buffer.
> >
> > null-co:// has the highest possible performance as a block driver, so
> > let's keep it that way. This patch introduces zero-co:// and
> > zero-aio://, largely similar with null-*://, but have
> read-zeroes=on by
> > default, so it's more suitable in cases than null-co://.
> 
> Thanks!
> 
> >
> > Signed-off-by: Fam Zheng mailto:f...@euphon.net>>
> > ---
> >  block/null.c | 91
> 
> >  1 file changed, 91 insertions(+)
> 
> > +static int zero_file_open(BlockDriverState *bs, QDict *options,
> int flags,
> > +                          Error **errp)
> > +{
> > +    QemuOpts *opts;
> > +    BDRVNullState *s = bs->opaque;
> > +    int ret = 0;
> > +
> > +    opts = qemu_opts_create(_opts, NULL, 0, _abort);
> > +    qemu_opts_absorb_qdict(opts, options, _abort);
> > +    s->length =
> > +        qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
> 
> Please do not use this magic default value, let's always explicit the
> size.
> 
>     s->length = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
>     if (s->length < 0) {
>         error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE);
>         ret = -EINVAL;
>     }
> 
> 
> Doesn't that result in a bare
> 
>   -blockdev zero-co://
> 
> would have 0 byte in size?

Yes, this will display an error.

Maybe better something like:

if (s->length == 0) {
error_append_hint(errp, "Usage: zero-co://,size=NUM");
error_setg(errp, "size must be specified");
ret = -EINVAL;
} else if (s->length < 0) {
error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE);
ret = -EINVAL;
}

> 
> I'm copying what null-co:// does today, and it's convenient for tests.
> Why is a default size bad?

We learned default are almost always bad because you can not get
rid of them. Also because we have reports of errors when using
floppy and another one (can't remember) with null-co because of
invalid size and we have to explain "the default is 1GB, you have
to explicit your size".

Phil.

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Vladimir Sementsov-Ogievskiy]

10.03.2021 17:17, f...@euphon.net wrote:

From: Fam Zheng 

null-co:// has a read-zeroes=off default, when used to in security
analysis, this can cause false positives because the driver doesn't
write to the read buffer.

null-co:// has the highest possible performance as a block driver, so
let's keep it that way. This patch introduces zero-co:// and
zero-aio://, largely similar with null-*://, but have read-zeroes=on by
default, so it's more suitable in cases than null-co://.

Signed-off-by: Fam Zheng 
---
  block/null.c | 91 
  1 file changed, 91 insertions(+)

diff --git a/block/null.c b/block/null.c
index cc9b1d4ea7..5de97e8fda 100644
--- a/block/null.c
+++ b/block/null.c
@@ -76,6 +76,30 @@ static void null_aio_parse_filename(const char *filename, 
QDict *options,
  }
  }
  
+static void zero_co_parse_filename(const char *filename, QDict *options,

+   Error **errp)
+{
+/* This functions only exists so that a zero-co:// filename is accepted
+ * with the zero-co driver. */
+if (strcmp(filename, "zero-co://")) {
+error_setg(errp, "The only allowed filename for this driver is "
+ "'zero-co://'");
+return;
+}
+}
+
+static void zero_aio_parse_filename(const char *filename, QDict *options,
+Error **errp)
+{
+/* This functions only exists so that a zero-aio:// filename is accepted
+ * with the zero-aio driver. */
+if (strcmp(filename, "zero-aio://")) {
+error_setg(errp, "The only allowed filename for this driver is "
+ "'zero-aio://'");
+return;
+}
+}
+
  static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
  {
@@ -99,6 +123,29 @@ static int null_file_open(BlockDriverState *bs, QDict 
*options, int flags,
  return ret;
  }
  
+static int zero_file_open(BlockDriverState *bs, QDict *options, int flags,

+  Error **errp)
+{
+QemuOpts *opts;
+BDRVNullState *s = bs->opaque;
+int ret = 0;
+
+opts = qemu_opts_create(_opts, NULL, 0, _abort);
+qemu_opts_absorb_qdict(opts, options, _abort);
+s->length =
+qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
+s->latency_ns =
+qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0);
+if (s->latency_ns < 0) {
+error_setg(errp, "latency-ns is invalid");
+ret = -EINVAL;
+}
+s->read_zeroes = true;
+qemu_opts_del(opts);
+bs->supported_write_flags = BDRV_REQ_FUA;
+return ret;
+}
+
  static int64_t null_getlength(BlockDriverState *bs)
  {
  BDRVNullState *s = bs->opaque;
@@ -316,10 +363,54 @@ static BlockDriver bdrv_null_aio = {
  .strong_runtime_opts= null_strong_runtime_opts,
  };
  
+static BlockDriver bdrv_zero_co = {

+.format_name= "zero-co",
+.protocol_name  = "zero-co",
+.instance_size  = sizeof(BDRVNullState),
+
+.bdrv_file_open = zero_file_open,
+.bdrv_parse_filename= zero_co_parse_filename,
+.bdrv_getlength = null_getlength,
+.bdrv_get_allocated_file_size = null_allocated_file_size,
+
+.bdrv_co_preadv = null_co_preadv,
+.bdrv_co_pwritev= null_co_pwritev,
+.bdrv_co_flush_to_disk  = null_co_flush,
+.bdrv_reopen_prepare= null_reopen_prepare,
+
+.bdrv_co_block_status   = null_co_block_status,
+
+.bdrv_refresh_filename  = null_refresh_filename,
+.strong_runtime_opts= null_strong_runtime_opts,
+};
+
+static BlockDriver bdrv_zero_aio = {
+.format_name= "zero-aio",
+.protocol_name  = "zero-aio",
+.instance_size  = sizeof(BDRVNullState),
+
+.bdrv_file_open = zero_file_open,
+.bdrv_parse_filename= zero_aio_parse_filename,
+.bdrv_getlength = null_getlength,
+.bdrv_get_allocated_file_size = null_allocated_file_size,
+
+.bdrv_aio_preadv= null_aio_preadv,
+.bdrv_aio_pwritev   = null_aio_pwritev,
+.bdrv_aio_flush = null_aio_flush,
+.bdrv_reopen_prepare= null_reopen_prepare,
+
+.bdrv_co_block_status   = null_co_block_status,
+
+.bdrv_refresh_filename  = null_refresh_filename,
+.strong_runtime_opts= null_strong_runtime_opts,
+};


I don't think we need separate zero-aio driver. What is the actual difference?

Probably zero-co is enough, and we can call it just zero. And then, maybe add 
null driver (same as null-co, but without read_zeroes option) and deprecate 
null-co and null-aio drivers. Thus we'll get two clean well defined things: 
null and zero drivers..


+
  static void bdrv_null_init(void)
  {
  bdrv_register(_null_co);
  bdrv_register(_null_aio);
+bdrv_register(_zero_co);
+bdrv_register(_zero_aio);
  }
  
  block_init(bdrv_null_init);





--
Best regards,
Vladimir

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Fam Zheng]

On Wed, 10 Mar 2021 at 14:24, Philippe Mathieu-Daudé 
wrote:

> On 3/10/21 3:17 PM, f...@euphon.net wrote:
> > From: Fam Zheng 
> >
> > null-co:// has a read-zeroes=off default, when used to in security
> > analysis, this can cause false positives because the driver doesn't
> > write to the read buffer.
> >
> > null-co:// has the highest possible performance as a block driver, so
> > let's keep it that way. This patch introduces zero-co:// and
> > zero-aio://, largely similar with null-*://, but have read-zeroes=on by
> > default, so it's more suitable in cases than null-co://.
>
> Thanks!
>
> >
> > Signed-off-by: Fam Zheng 
> > ---
> >  block/null.c | 91 
> >  1 file changed, 91 insertions(+)
>
> > +static int zero_file_open(BlockDriverState *bs, QDict *options, int
> flags,
> > +  Error **errp)
> > +{
> > +QemuOpts *opts;
> > +BDRVNullState *s = bs->opaque;
> > +int ret = 0;
> > +
> > +opts = qemu_opts_create(_opts, NULL, 0, _abort);
> > +qemu_opts_absorb_qdict(opts, options, _abort);
> > +s->length =
> > +qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
>
> Please do not use this magic default value, let's always explicit the
> size.
>
> s->length = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> if (s->length < 0) {
> error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE);
> ret = -EINVAL;
> }


Doesn't that result in a bare

  -blockdev zero-co://

would have 0 byte in size?

I'm copying what null-co:// does today, and it's convenient for tests. Why
is a default size bad?

Fam



>
>
> +s->latency_ns =
> > +qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0);
> > +if (s->latency_ns < 0) {
> > +error_setg(errp, "latency-ns is invalid");
> > +ret = -EINVAL;
> > +}
> > +s->read_zeroes = true;
> > +qemu_opts_del(opts);
> > +bs->supported_write_flags = BDRV_REQ_FUA;
> > +return ret;
> > +}
>
> Otherwise LGTM :)
>
>
>

Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Philippe Mathieu-Daudé]

On 3/10/21 3:17 PM, f...@euphon.net wrote:
> From: Fam Zheng 
> 
> null-co:// has a read-zeroes=off default, when used to in security
> analysis, this can cause false positives because the driver doesn't
> write to the read buffer.
> 
> null-co:// has the highest possible performance as a block driver, so
> let's keep it that way. This patch introduces zero-co:// and
> zero-aio://, largely similar with null-*://, but have read-zeroes=on by
> default, so it's more suitable in cases than null-co://.

Thanks!

> 
> Signed-off-by: Fam Zheng 
> ---
>  block/null.c | 91 
>  1 file changed, 91 insertions(+)

> +static int zero_file_open(BlockDriverState *bs, QDict *options, int flags,
> +  Error **errp)
> +{
> +QemuOpts *opts;
> +BDRVNullState *s = bs->opaque;
> +int ret = 0;
> +
> +opts = qemu_opts_create(_opts, NULL, 0, _abort);
> +qemu_opts_absorb_qdict(opts, options, _abort);
> +s->length =
> +qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);

Please do not use this magic default value, let's always explicit the
size.

s->length = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
if (s->length < 0) {
error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE);
ret = -EINVAL;
}

> +s->latency_ns =
> +qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0);
> +if (s->latency_ns < 0) {
> +error_setg(errp, "latency-ns is invalid");
> +ret = -EINVAL;
> +}
> +s->read_zeroes = true;
> +qemu_opts_del(opts);
> +bs->supported_write_flags = BDRV_REQ_FUA;
> +return ret;
> +}

Otherwise LGTM :)

[PATCH] block: Introduce zero-co:// and zero-aio://

[fam]

From: Fam Zheng 

null-co:// has a read-zeroes=off default, when used to in security
analysis, this can cause false positives because the driver doesn't
write to the read buffer.

null-co:// has the highest possible performance as a block driver, so
let's keep it that way. This patch introduces zero-co:// and
zero-aio://, largely similar with null-*://, but have read-zeroes=on by
default, so it's more suitable in cases than null-co://.

Signed-off-by: Fam Zheng 
---
 block/null.c | 91 
 1 file changed, 91 insertions(+)

diff --git a/block/null.c b/block/null.c
index cc9b1d4ea7..5de97e8fda 100644
--- a/block/null.c
+++ b/block/null.c
@@ -76,6 +76,30 @@ static void null_aio_parse_filename(const char *filename, 
QDict *options,
 }
 }
 
+static void zero_co_parse_filename(const char *filename, QDict *options,
+   Error **errp)
+{
+/* This functions only exists so that a zero-co:// filename is accepted
+ * with the zero-co driver. */
+if (strcmp(filename, "zero-co://")) {
+error_setg(errp, "The only allowed filename for this driver is "
+ "'zero-co://'");
+return;
+}
+}
+
+static void zero_aio_parse_filename(const char *filename, QDict *options,
+Error **errp)
+{
+/* This functions only exists so that a zero-aio:// filename is accepted
+ * with the zero-aio driver. */
+if (strcmp(filename, "zero-aio://")) {
+error_setg(errp, "The only allowed filename for this driver is "
+ "'zero-aio://'");
+return;
+}
+}
+
 static int null_file_open(BlockDriverState *bs, QDict *options, int flags,
   Error **errp)
 {
@@ -99,6 +123,29 @@ static int null_file_open(BlockDriverState *bs, QDict 
*options, int flags,
 return ret;
 }
 
+static int zero_file_open(BlockDriverState *bs, QDict *options, int flags,
+  Error **errp)
+{
+QemuOpts *opts;
+BDRVNullState *s = bs->opaque;
+int ret = 0;
+
+opts = qemu_opts_create(_opts, NULL, 0, _abort);
+qemu_opts_absorb_qdict(opts, options, _abort);
+s->length =
+qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30);
+s->latency_ns =
+qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0);
+if (s->latency_ns < 0) {
+error_setg(errp, "latency-ns is invalid");
+ret = -EINVAL;
+}
+s->read_zeroes = true;
+qemu_opts_del(opts);
+bs->supported_write_flags = BDRV_REQ_FUA;
+return ret;
+}
+
 static int64_t null_getlength(BlockDriverState *bs)
 {
 BDRVNullState *s = bs->opaque;
@@ -316,10 +363,54 @@ static BlockDriver bdrv_null_aio = {
 .strong_runtime_opts= null_strong_runtime_opts,
 };
 
+static BlockDriver bdrv_zero_co = {
+.format_name= "zero-co",
+.protocol_name  = "zero-co",
+.instance_size  = sizeof(BDRVNullState),
+
+.bdrv_file_open = zero_file_open,
+.bdrv_parse_filename= zero_co_parse_filename,
+.bdrv_getlength = null_getlength,
+.bdrv_get_allocated_file_size = null_allocated_file_size,
+
+.bdrv_co_preadv = null_co_preadv,
+.bdrv_co_pwritev= null_co_pwritev,
+.bdrv_co_flush_to_disk  = null_co_flush,
+.bdrv_reopen_prepare= null_reopen_prepare,
+
+.bdrv_co_block_status   = null_co_block_status,
+
+.bdrv_refresh_filename  = null_refresh_filename,
+.strong_runtime_opts= null_strong_runtime_opts,
+};
+
+static BlockDriver bdrv_zero_aio = {
+.format_name= "zero-aio",
+.protocol_name  = "zero-aio",
+.instance_size  = sizeof(BDRVNullState),
+
+.bdrv_file_open = zero_file_open,
+.bdrv_parse_filename= zero_aio_parse_filename,
+.bdrv_getlength = null_getlength,
+.bdrv_get_allocated_file_size = null_allocated_file_size,
+
+.bdrv_aio_preadv= null_aio_preadv,
+.bdrv_aio_pwritev   = null_aio_pwritev,
+.bdrv_aio_flush = null_aio_flush,
+.bdrv_reopen_prepare= null_reopen_prepare,
+
+.bdrv_co_block_status   = null_co_block_status,
+
+.bdrv_refresh_filename  = null_refresh_filename,
+.strong_runtime_opts= null_strong_runtime_opts,
+};
+
 static void bdrv_null_init(void)
 {
 bdrv_register(_null_co);
 bdrv_register(_null_aio);
+bdrv_register(_zero_co);
+bdrv_register(_zero_aio);
 }
 
 block_init(bdrv_null_init);
-- 
2.17.1