Re: [PATCH v2 2/4] qemu-img: make --block-size optional for compare --stat

[Hanna Reitz <hreitz@redhat.com>]

On 21.10.21 12:12, Vladimir Sementsov-Ogievskiy wrote:
> Let's detect block-size automatically if not specified by user:
>
>   If both files define cluster-size, use minimum to be more precise.
>   If both files don't specify cluster-size, use default of 64K
>   If only one file specify cluster-size, just use it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   docs/tools/qemu-img.rst |  7 +++-
>   qemu-img.c              | 71 ++++++++++++++++++++++++++++++++++++-----
>   qemu-img-cmds.hx        |  4 +--
>   3 files changed, 71 insertions(+), 11 deletions(-)

Looks good, just grammar nits and a request for an assertion below.

> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
> index 21164253d4..4b382ca2b0 100644
> --- a/docs/tools/qemu-img.rst
> +++ b/docs/tools/qemu-img.rst
> @@ -170,6 +170,11 @@ Parameters to compare subcommand:
>     Block size for comparing with ``--stat``. This doesn't guarantee exact
>     size of comparing chunks, but that guarantee that data chunks being
>     compared will never cross aligned block-size boundary.
> +  When unspecified the following logic is used:
> +
> +    - If both files define cluster-size, use minimum to be more precise.
> +    - If both files don't specify cluster-size, use default of 64K
> +    - If only one file specify cluster-size, just use it.

s/specify/specifies/; and perhaps s/it/that/

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index 79a0589167..61e7f470bb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1407,6 +1407,61 @@ static void cmp_stat_print(ImgCmpStat *stat, int64_t total_bytes)
>       }
>   }
>   
> +/* Get default value for qemu-img compare --block-size option. */
> +static int img_compare_block_size(BlockDriverState *bs1,
> +                                  BlockDriverState *bs2,
> +                                  bool quiet)
> +{
> +    const int default_block_size = 64 * 1024; /* 64K */
> +
> +    int ret;
> +    BlockDriverInfo bdi;
> +    int cluster_size1, cluster_size2, block_size;
> +    const char *note = "Note: to alter it, set --block-size option.";
> +    const char *fname1 = bs1->filename;
> +    const char *fname2 = bs2->filename;
> +
> +    ret = bdrv_get_info(bs1, &bdi);
> +    if (ret < 0 && ret != -ENOTSUP) {
> +        error_report("Failed to get info of %s: %s", fname1, strerror(-ret));
> +        return ret;
> +    }
> +    cluster_size1 = ret < 0 ? 0 : bdi.cluster_size;
> +
> +    ret = bdrv_get_info(bs2, &bdi);
> +    if (ret < 0 && ret != -ENOTSUP) {
> +        error_report("Failed to get info of %s: %s", fname2, strerror(-ret));
> +        return ret;
> +    }
> +    cluster_size2 = ret < 0 ? 0 : bdi.cluster_size;
> +

I’d feel better with an `assert(cluster_size1 >= 0 && cluster_size2 >= 
0);` here.

> +    if (cluster_size1 > 0 && cluster_size2 > 0) {
> +        if (cluster_size1 == cluster_size2) {
> +            block_size = cluster_size1;
> +        } else {
> +            block_size = MIN(cluster_size1, cluster_size2);
> +            qprintf(quiet, "%s and %s has different cluster sizes: %d and %d "

s/has/have/

> +                    "correspondingly. Use minimum as block-size for "

s/correspondingly/respectively/; s/Use/Using/ (“Use” sounds like an 
imperative)

> +                    "accuracy: %d. %s\n",
> +                    fname1, fname2, cluster_size1,
> +                    cluster_size2, block_size, note);
> +        }
> +    } else if (cluster_size1 == 0 && cluster_size2 == 0) {
> +        block_size = default_block_size;
> +        qprintf(quiet, "Neither of %s and %s has explicit cluster size. Use "

s/has/have an/; s/Use/Using/

> +                "default of %d bytes. %s\n", fname1, fname2, block_size, note);
> +    } else {
> +        block_size = MAX(cluster_size1, cluster_size2);
> +        qprintf(quiet, "%s has explicit cluster size of %d and %s "

s/has/has an/

> +                "doesn't have one. Use %d as block-size. %s\n",

s/Use/Using/

Hanna

> +                cluster_size1 ? fname1 : fname2, block_size,
> +                cluster_size1 ? fname2 : fname1,
> +                block_size, note);
> +    }
> +
> +    return block_size;
> +}
> +
>   /*
>    * Compares two images. Exit codes:
>    *