On Wed, 10 Mar 2021 at 14:51, Philippe Mathieu-Daudé <phi...@redhat.com> wrote:
> On 3/10/21 3:28 PM, Fam Zheng wrote: > > On Wed, 10 Mar 2021 at 14:24, Philippe Mathieu-Daudé <phi...@redhat.com > > <mailto:phi...@redhat.com>> wrote: > > > > On 3/10/21 3:17 PM, f...@euphon.net <mailto:f...@euphon.net> wrote: > > > From: Fam Zheng <famzh...@amazon.com <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 <f...@euphon.net <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(&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); > > > > 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
