| Message ID | 20211021101236.1144824-3-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img compare --stat | expand |
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: > *
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 --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,
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(-)