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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

26.10.2021 11:07, Hanna Reitz wrote:
> 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.

Hmm.. But it seems obvious: bdi.cluster_size should not be <0 on success of bdrv_get_info.

> 
>> +    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:
>>    *
> 


-- 
Best regards,
Vladimir