| Message ID | 20210929133427.44923-4-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img compare --stat | expand |
On Wed, Sep 29, 2021 at 4:37 PM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > Allow compare only top images of backing chains. That's useful for > comparing two increments from the same chain of incremental backups. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > docs/tools/qemu-img.rst | 8 +++++++- > qemu-img.c | 14 ++++++++++++-- > qemu-img-cmds.hx | 4 ++-- > 3 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst > index 4b382ca2b0..c8ae96be6a 100644 > --- a/docs/tools/qemu-img.rst > +++ b/docs/tools/qemu-img.rst > @@ -176,6 +176,12 @@ Parameters to compare subcommand: > - If both files don't specify cluster-size, use default of 64K > - If only one file specify cluster-size, just use it. > > +.. option:: --shallow We use the same term in oVirt when we upload/download one layer from a chain. > + Only allowed with ``--stat``. This option prevents opening and comparing > + any backing files. This is useful to compare incremental images from > + the chain of incremental backups. This is useful also without --stat. Our current workaround in oVirt is to use unsafe rebase to disconnect the top image from the base image so we can compare source and destination image after backup. Here is an example of test code that could use --shallow (regardless of --stat): https://github.com/oVirt/ovirt-imageio/blob/master/daemon/test/backup_test.py#L114 Do you have any reason to limit --shallow to --stats? > + > Parameters to convert subcommand: > > .. program:: qemu-img-convert > @@ -395,7 +401,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] [--shallow]] 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 61e7f470bb..e8ae412c38 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -85,6 +85,7 @@ enum { > OPTION_SKIP_BROKEN = 277, > OPTION_STAT = 277, > OPTION_BLOCK_SIZE = 278, > + OPTION_SHALLOW = 279, > }; > > typedef enum OutputFormat { > @@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv) > int64_t block_end; > int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */ > bool progress = false, quiet = false, strict = false; > - int flags; > + int flags = 0; > bool writethrough; > int64_t total_size; > int64_t offset = 0; > @@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv) > {"force-share", no_argument, 0, 'U'}, > {"stat", no_argument, 0, OPTION_STAT}, > {"block-size", required_argument, 0, OPTION_BLOCK_SIZE}, > + {"shallow", no_argument, 0, OPTION_SHALLOW}, > {0, 0, 0, 0} > }; > c = getopt_long(argc, argv, ":hf:F:T:pqsU", > @@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv) > exit(EXIT_SUCCESS); > } > break; > + case OPTION_SHALLOW: > + flags |= BDRV_O_NO_BACKING; > + break; > } > } > > @@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv) > goto out; > } > > + if (!do_stat && (flags & BDRV_O_NO_BACKING)) { > + error_report("--shallow can be used only together with --stat"); > + ret = 1; > + goto out; > + } > + > /* Initialize before goto out */ > qemu_progress_init(progress, 2.0); > > - flags = 0; > ret = bdrv_parse_cache_mode(cache, &flags, &writethrough); > if (ret < 0) { > error_report("Invalid source cache option: %s", cache); > diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > index 96a193eea8..a295bc6860 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] [--shallow]] 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] [--shallow]] FILENAME1 FILENAME2 > ERST > > DEF("convert", img_convert, > -- > 2.29.2 > Looks good as is, we can remove the limit later without breaking users. Nir
29.09.2021 19:00, Nir Soffer wrote: > On Wed, Sep 29, 2021 at 4:37 PM Vladimir Sementsov-Ogievskiy > <vsementsov@virtuozzo.com> wrote: >> >> Allow compare only top images of backing chains. That's useful for >> comparing two increments from the same chain of incremental backups. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> docs/tools/qemu-img.rst | 8 +++++++- >> qemu-img.c | 14 ++++++++++++-- >> qemu-img-cmds.hx | 4 ++-- >> 3 files changed, 21 insertions(+), 5 deletions(-) >> >> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst >> index 4b382ca2b0..c8ae96be6a 100644 >> --- a/docs/tools/qemu-img.rst >> +++ b/docs/tools/qemu-img.rst >> @@ -176,6 +176,12 @@ Parameters to compare subcommand: >> - If both files don't specify cluster-size, use default of 64K >> - If only one file specify cluster-size, just use it. >> >> +.. option:: --shallow > > We use the same term in oVirt when we upload/download one layer from a chain. > >> + Only allowed with ``--stat``. This option prevents opening and comparing >> + any backing files. This is useful to compare incremental images from >> + the chain of incremental backups. > > This is useful also without --stat. Our current workaround in oVirt is > to use unsafe > rebase to disconnect the top image from the base image so we can compare > source and destination image after backup. > > Here is an example of test code that could use --shallow (regardless of --stat): > https://github.com/oVirt/ovirt-imageio/blob/master/daemon/test/backup_test.py#L114 > > Do you have any reason to limit --shallow to --stats? Hmm. I wrongly thought that without --stat qemu-img compare will fail on first mismatch, which will occur soon, as we don't have backing images and it's just superfluous. But actually, qemu-img will not compare "unallocated" areas. Ok, I agree, in v2 I'll allow --shallow without --stat. Another question to discuss: we already have "-u" option in qemu-img create and qemu-img rebase to not open backing files. And 'u' means 'unsafe'. I don't think that "unsafe" term is good for qemu-img compare --stat, that's why I decided to call it differently: "shallow". Still for qemu-img compare (without --stat) "unsafe" term make sense. So, it probably better to follow common notation, and call the option "-u". > >> + >> Parameters to convert subcommand: >> >> .. program:: qemu-img-convert >> @@ -395,7 +401,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] [--shallow]] 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 61e7f470bb..e8ae412c38 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -85,6 +85,7 @@ enum { >> OPTION_SKIP_BROKEN = 277, >> OPTION_STAT = 277, >> OPTION_BLOCK_SIZE = 278, >> + OPTION_SHALLOW = 279, >> }; >> >> typedef enum OutputFormat { >> @@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv) >> int64_t block_end; >> int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */ >> bool progress = false, quiet = false, strict = false; >> - int flags; >> + int flags = 0; >> bool writethrough; >> int64_t total_size; >> int64_t offset = 0; >> @@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv) >> {"force-share", no_argument, 0, 'U'}, >> {"stat", no_argument, 0, OPTION_STAT}, >> {"block-size", required_argument, 0, OPTION_BLOCK_SIZE}, >> + {"shallow", no_argument, 0, OPTION_SHALLOW}, >> {0, 0, 0, 0} >> }; >> c = getopt_long(argc, argv, ":hf:F:T:pqsU", >> @@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv) >> exit(EXIT_SUCCESS); >> } >> break; >> + case OPTION_SHALLOW: >> + flags |= BDRV_O_NO_BACKING; >> + break; >> } >> } >> >> @@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv) >> goto out; >> } >> >> + if (!do_stat && (flags & BDRV_O_NO_BACKING)) { >> + error_report("--shallow can be used only together with --stat"); >> + ret = 1; >> + goto out; >> + } >> + >> /* Initialize before goto out */ >> qemu_progress_init(progress, 2.0); >> >> - flags = 0; >> ret = bdrv_parse_cache_mode(cache, &flags, &writethrough); >> if (ret < 0) { >> error_report("Invalid source cache option: %s", cache); >> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx >> index 96a193eea8..a295bc6860 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] [--shallow]] 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] [--shallow]] FILENAME1 FILENAME2 >> ERST >> >> DEF("convert", img_convert, >> -- >> 2.29.2 >> > > Looks good as is, we can remove the limit later without breaking users. > > Nir >
On Wed, Sep 29, 2021 at 7:28 PM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > 29.09.2021 19:00, Nir Soffer wrote: > > On Wed, Sep 29, 2021 at 4:37 PM Vladimir Sementsov-Ogievskiy > > <vsementsov@virtuozzo.com> wrote: > >> > >> Allow compare only top images of backing chains. That's useful for > >> comparing two increments from the same chain of incremental backups. > >> > >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> --- > >> docs/tools/qemu-img.rst | 8 +++++++- > >> qemu-img.c | 14 ++++++++++++-- > >> qemu-img-cmds.hx | 4 ++-- > >> 3 files changed, 21 insertions(+), 5 deletions(-) > >> > >> diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst > >> index 4b382ca2b0..c8ae96be6a 100644 > >> --- a/docs/tools/qemu-img.rst > >> +++ b/docs/tools/qemu-img.rst > >> @@ -176,6 +176,12 @@ Parameters to compare subcommand: > >> - If both files don't specify cluster-size, use default of 64K > >> - If only one file specify cluster-size, just use it. > >> > >> +.. option:: --shallow > > > > We use the same term in oVirt when we upload/download one layer from a chain. > > > >> + Only allowed with ``--stat``. This option prevents opening and comparing > >> + any backing files. This is useful to compare incremental images from > >> + the chain of incremental backups. > > > > This is useful also without --stat. Our current workaround in oVirt is > > to use unsafe > > rebase to disconnect the top image from the base image so we can compare > > source and destination image after backup. > > > > Here is an example of test code that could use --shallow (regardless of --stat): > > https://github.com/oVirt/ovirt-imageio/blob/master/daemon/test/backup_test.py#L114 > > > > Do you have any reason to limit --shallow to --stats? > > > Hmm. I wrongly thought that without --stat qemu-img compare will fail on first mismatch, which will occur soon, as we don't have backing images and it's just superfluous. > > But actually, qemu-img will not compare "unallocated" areas. > > Ok, I agree, in v2 I'll allow --shallow without --stat. > > > Another question to discuss: we already have "-u" option in qemu-img create and qemu-img rebase to not open backing files. And 'u' means 'unsafe'. > I don't think that "unsafe" term is good for qemu-img compare --stat, that's why I decided to call it differently: "shallow". > Still for qemu-img compare (without --stat) "unsafe" term make sense. > > > So, it probably better to follow common notation, and call the option "-u". --shallow is better, comparing a single image from a chain is a safe operation. Replacing a backing file or creating an image on top of one without checking the backing file is not. > > > > >> + > >> Parameters to convert subcommand: > >> > >> .. program:: qemu-img-convert > >> @@ -395,7 +401,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] [--shallow]] 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 61e7f470bb..e8ae412c38 100644 > >> --- a/qemu-img.c > >> +++ b/qemu-img.c > >> @@ -85,6 +85,7 @@ enum { > >> OPTION_SKIP_BROKEN = 277, > >> OPTION_STAT = 277, > >> OPTION_BLOCK_SIZE = 278, > >> + OPTION_SHALLOW = 279, > >> }; > >> > >> typedef enum OutputFormat { > >> @@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv) > >> int64_t block_end; > >> int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */ > >> bool progress = false, quiet = false, strict = false; > >> - int flags; > >> + int flags = 0; > >> bool writethrough; > >> int64_t total_size; > >> int64_t offset = 0; > >> @@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv) > >> {"force-share", no_argument, 0, 'U'}, > >> {"stat", no_argument, 0, OPTION_STAT}, > >> {"block-size", required_argument, 0, OPTION_BLOCK_SIZE}, > >> + {"shallow", no_argument, 0, OPTION_SHALLOW}, > >> {0, 0, 0, 0} > >> }; > >> c = getopt_long(argc, argv, ":hf:F:T:pqsU", > >> @@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv) > >> exit(EXIT_SUCCESS); > >> } > >> break; > >> + case OPTION_SHALLOW: > >> + flags |= BDRV_O_NO_BACKING; > >> + break; > >> } > >> } > >> > >> @@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv) > >> goto out; > >> } > >> > >> + if (!do_stat && (flags & BDRV_O_NO_BACKING)) { > >> + error_report("--shallow can be used only together with --stat"); > >> + ret = 1; > >> + goto out; > >> + } > >> + > >> /* Initialize before goto out */ > >> qemu_progress_init(progress, 2.0); > >> > >> - flags = 0; > >> ret = bdrv_parse_cache_mode(cache, &flags, &writethrough); > >> if (ret < 0) { > >> error_report("Invalid source cache option: %s", cache); > >> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx > >> index 96a193eea8..a295bc6860 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] [--shallow]] 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] [--shallow]] FILENAME1 FILENAME2 > >> ERST > >> > >> DEF("convert", img_convert, > >> -- > >> 2.29.2 > >> > > > > Looks good as is, we can remove the limit later without breaking users. > > > > Nir > > > > > -- > Best regards, > Vladimir >
diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst index 4b382ca2b0..c8ae96be6a 100644 --- a/docs/tools/qemu-img.rst +++ b/docs/tools/qemu-img.rst @@ -176,6 +176,12 @@ Parameters to compare subcommand: - If both files don't specify cluster-size, use default of 64K - If only one file specify cluster-size, just use it. +.. option:: --shallow + + Only allowed with ``--stat``. This option prevents opening and comparing + any backing files. This is useful to compare incremental images from + the chain of incremental backups. + Parameters to convert subcommand: .. program:: qemu-img-convert @@ -395,7 +401,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] [--shallow]] 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 61e7f470bb..e8ae412c38 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -85,6 +85,7 @@ enum { OPTION_SKIP_BROKEN = 277, OPTION_STAT = 277, OPTION_BLOCK_SIZE = 278, + OPTION_SHALLOW = 279, }; typedef enum OutputFormat { @@ -1482,7 +1483,7 @@ static int img_compare(int argc, char **argv) int64_t block_end; int ret = 0; /* return value - 0 Ident, 1 Different, >1 Error */ bool progress = false, quiet = false, strict = false; - int flags; + int flags = 0; bool writethrough; int64_t total_size; int64_t offset = 0; @@ -1504,6 +1505,7 @@ static int img_compare(int argc, char **argv) {"force-share", no_argument, 0, 'U'}, {"stat", no_argument, 0, OPTION_STAT}, {"block-size", required_argument, 0, OPTION_BLOCK_SIZE}, + {"shallow", no_argument, 0, OPTION_SHALLOW}, {0, 0, 0, 0} }; c = getopt_long(argc, argv, ":hf:F:T:pqsU", @@ -1569,6 +1571,9 @@ static int img_compare(int argc, char **argv) exit(EXIT_SUCCESS); } break; + case OPTION_SHALLOW: + flags |= BDRV_O_NO_BACKING; + break; } } @@ -1590,10 +1595,15 @@ static int img_compare(int argc, char **argv) goto out; } + if (!do_stat && (flags & BDRV_O_NO_BACKING)) { + error_report("--shallow can be used only together with --stat"); + ret = 1; + goto out; + } + /* Initialize before goto out */ qemu_progress_init(progress, 2.0); - flags = 0; ret = bdrv_parse_cache_mode(cache, &flags, &writethrough); if (ret < 0) { error_report("Invalid source cache option: %s", cache); diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index 96a193eea8..a295bc6860 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] [--shallow]] 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] [--shallow]] FILENAME1 FILENAME2 ERST DEF("convert", img_convert,
Allow compare only top images of backing chains. That's useful for comparing two increments from the same chain of incremental backups. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- docs/tools/qemu-img.rst | 8 +++++++- qemu-img.c | 14 ++++++++++++-- qemu-img-cmds.hx | 4 ++-- 3 files changed, 21 insertions(+), 5 deletions(-)