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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 3/10/21 3:28 PM, Fam Zheng wrote:
> On Wed, 10 Mar 2021 at 14:24, Philippe Mathieu-Daudé <philmd@redhat.com
> <mailto:philmd@redhat.com>> wrote:
> 
>     On 3/10/21 3:17 PM, fam@euphon.net <mailto:fam@euphon.net> wrote:
>     > From: Fam Zheng <famzheng@amazon.com <mailto:famzheng@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 <fam@euphon.net <mailto:fam@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".

Phil.