diff mbox series

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

Message ID 20211021101236.1144824-3-vsementsov@virtuozzo.com
State New
Headers show
Series qemu-img compare --stat | expand

Commit Message

Vladimir Sementsov-Ogievskiy Oct. 21, 2021, 10:12 a.m. UTC 
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(-)

Comments

Hanna Czenczek Oct. 26, 2021, 8:07 a.m. UTC | #1 
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:
>    *
Vladimir Sementsov-Ogievskiy Oct. 28, 2021, 10:04 a.m. UTC | #2 
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:
>>    *
>
diff mbox series

Patch

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.
 
 Parameters to convert subcommand:
 
@@ -390,7 +395,7 @@  Command description:
 
   The rate limit for the commit process is specified by ``-r``.
 
-.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat --block-size BLOCK_SIZE] FILENAME1 FILENAME2
+.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] FILENAME1 FILENAME2
 
   Check if two images have the same content. You can compare images with
   different format or settings.
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;
+
+    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 "
+                    "correspondingly. Use minimum as block-size for "
+                    "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 "
+                "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 "
+                "doesn't have one. Use %d as block-size. %s\n",
+                cluster_size1 ? fname1 : fname2, block_size,
+                cluster_size1 ? fname2 : fname1,
+                block_size, note);
+    }
+
+    return block_size;
+}
+
 /*
  * Compares two images. Exit codes:
  *
@@ -1535,14 +1590,6 @@  static int img_compare(int argc, char **argv)
         goto out;
     }
 
-    if (do_stat && !block_size) {
-        /* TODO: make block-size optional */
-        error_report("You must specify --block-size together with --stat");
-        ret = 1;
-        goto out;
-    }
-
-
     /* Initialize before goto out */
     qemu_progress_init(progress, 2.0);
 
@@ -1589,6 +1636,14 @@  static int img_compare(int argc, char **argv)
     total_size = MIN(total_size1, total_size2);
     progress_base = MAX(total_size1, total_size2);
 
+    if (do_stat && !block_size) {
+        block_size = img_compare_block_size(bs1, bs2, quiet);
+        if (block_size <= 0) {
+            ret = 4;
+            goto out;
+        }
+    }
+
     qemu_progress_print(0, 100);
 
     if (strict && total_size1 != total_size2) {
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 0b2d63ca5f..96a193eea8 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -40,9 +40,9 @@  SRST
 ERST
 
 DEF("compare", img_compare,
-    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] [-U] [--stat --block-size BLOCK_SIZE] filename1 filename2")
+    "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T src_cache] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] filename1 filename2")
 SRST
-.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat --block-size BLOCK_SIZE] FILENAME1 FILENAME2
+.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat [--block-size BLOCK_SIZE]] FILENAME1 FILENAME2
 ERST
 
 DEF("convert", img_convert,