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

[Vladimir Sementsov-Ogievskiy]

10.03.2021 17:17, fam@euphon.net wrote:
> From: Fam Zheng <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://.
>
> Signed-off-by: Fam Zheng <fam@euphon.net>
> ---
>   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(&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);
> +    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(&bdrv_null_co);
>       bdrv_register(&bdrv_null_aio);
> +    bdrv_register(&bdrv_zero_co);
> +    bdrv_register(&bdrv_zero_aio);
>   }
>   
>   block_init(bdrv_null_init);


--
Best regards,
Vladimir