| Message ID | 20211028102441.1878668-3-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | qemu-img compare --stat | expand |
On Thu, Oct 28, 2021 at 12:24:39PM +0200, 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, 72 insertions(+), 10 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com> > + 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 have different cluster sizes: %d and %d " > + "respectively. Using minimum as block-size for " > + "accuracy: %d. %s\n", > + fname1, fname2, cluster_size1, > + cluster_size2, block_size, note); Results in a long line; I don't know if it's worth trying to wrap it (if we had a generic utility function that took arbitrary text, then outputs it wrapped to the user's current terminal column width, I'd suggest using that instead - but that's NOT something I expect you to write, and I don't know if glib has such a utility).
29.10.2021 23:32, Eric Blake wrote: > On Thu, Oct 28, 2021 at 12:24:39PM +0200, 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, 72 insertions(+), 10 deletions(-) >> > > Reviewed-by: Eric Blake <eblake@redhat.com> > >> + 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 have different cluster sizes: %d and %d " >> + "respectively. Using minimum as block-size for " >> + "accuracy: %d. %s\n", >> + fname1, fname2, cluster_size1, >> + cluster_size2, block_size, note); > > Results in a long line; I don't know if it's worth trying to wrap it > (if we had a generic utility function that took arbitrary text, then > outputs it wrapped to the user's current terminal column width, I'd > suggest using that instead - but that's NOT something I expect you to > write, and I don't know if glib has such a utility). > Hmm. But long lines printed to the terminal are wrapped by terminal automatically, so we don't need to wrap to terminal width by hand..
On Mon, Nov 01, 2021 at 11:03:22AM +0300, Vladimir Sementsov-Ogievskiy wrote: > 29.10.2021 23:32, Eric Blake wrote: > > On Thu, Oct 28, 2021 at 12:24:39PM +0200, 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, 72 insertions(+), 10 deletions(-) > > > > > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > > > + 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 have different cluster sizes: %d and %d " > > > + "respectively. Using minimum as block-size for " > > > + "accuracy: %d. %s\n", > > > + fname1, fname2, cluster_size1, > > > + cluster_size2, block_size, note); > > > > Results in a long line; I don't know if it's worth trying to wrap it > > (if we had a generic utility function that took arbitrary text, then > > outputs it wrapped to the user's current terminal column width, I'd > > suggest using that instead - but that's NOT something I expect you to > > write, and I don't know if glib has such a utility). > > > > Hmm. But long lines printed to the terminal are wrapped by terminal automatically, so we don't need to wrap to terminal width by hand.. /me using a short line length in the next paragraph on purpose... There's a difference between the ter minal wrapping when you hit the maxi mum length, and in intelligent wrapping the prefers natural word breaks. There are programs out there that do intelligent wrapping (for example, the mutt mailer), but I'm not sure if those wrapping routines are available through glib. At any rate, I do NOT want to reimplement intelligent wrapping from scratch (Unicode specifies a rather complex algorithm for getting sane wrapping in the presence of various Unicode characters, and it is not trivial https://unicode.org/reports/tr14/). So unless someone knows of an easy-to-use library that does wrapping, you are right that leaving long lines for the terminal to output, even when that output has awkward mid-word breaks, is tolerable.
On 28.10.21 12:24, 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, 72 insertions(+), 10 deletions(-) Reviewed-by: Hanna Reitz <hreitz@redhat.com>
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index 8b6b799dd4..9bfdd93d6c 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 an exact size for comparing chunks, but it does guarantee that those chunks will never cross a block-size-aligned 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 specifies cluster-size, just use that. 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 0cb7cebe91..905150671f 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1407,6 +1407,62 @@ 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 have different cluster sizes: %d and %d " + "respectively. Using 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 have an explicit cluster size. " + "Using default of %d bytes. %s\n", fname1, fname2, block_size, + note); + } else { + block_size = MAX(cluster_size1, cluster_size2); + qprintf(quiet, "%s has an explicit cluster size of %d and %s " + "doesn't have one. Using %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: * @@ -1534,13 +1590,6 @@ static int img_compare(int argc, char **argv) goto out3; } - if (stat && !block_size) { - /* TODO: make block-size optional */ - error_report("You must specify --block-size together with --stat"); - ret = 2; - goto out3; - } - if (stat && strict) { error_report("--stat can't be used together with -s"); ret = 2; @@ -1593,6 +1642,14 @@ static int img_compare(int argc, char **argv) total_size = MIN(total_size1, total_size2); progress_base = MAX(total_size1, total_size2); + if (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, 72 insertions(+), 10 deletions(-)