Re: [PATCH v12 1/3] block: introduce compress filter driver

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2124 bytes --]

On 02.12.19 13:15, Andrey Shinkevich wrote:
> Allow writing all the data compressed through the filter driver.
> The written data will be aligned by the cluster size.
> Based on the QEMU current implementation, that data can be written to
> unallocated clusters only. May be used for a backup job.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/Makefile.objs     |   1 +
>  block/filter-compress.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json    |  10 +--
>  3 files changed, 175 insertions(+), 4 deletions(-)
>  create mode 100644 block/filter-compress.c

[...]

> diff --git a/block/filter-compress.c b/block/filter-compress.c
> new file mode 100644
> index 0000000..4d756ea
> --- /dev/null
> +++ b/block/filter-compress.c
> @@ -0,0 +1,168 @@

[...]

> +static int compress_open(BlockDriverState *bs, QDict *options, int flags,
> +                         Error **errp)
> +{
> +    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
> +                               errp);
> +    if (!bs->file) {
> +        return -EINVAL;
> +    }
> +
> +    if (!bs->file->bs->drv || !block_driver_can_compress(bs->file->bs->drv)) {
> +        error_setg(errp,
> +                   "Compression is not supported for underlying format: %s",
> +                   bdrv_get_format_name(bs->file->bs));

bdrv_get_format_name() returns NULL if bs->file->bs->drv is NULL.  I’m
sure g_strdup_vprintf() handles %s with a NULL string gracefully in
practice, but I can’t find that specified anywhere.  So even though I’m
well aware I’m being a bit stupid about a minor edge case, I’m hesitant
to accept this patch as-is.

Obviously the solution can be as simple as bdrv_get_format_name(...) ?:
"(no format)".

Well, actually, I can be a bit less stupid about it and just propose
merging that change in myself.  Would that be OK for you?

(The rest looks good to me.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]