On Wed, Sep 29, 2021 at 7:28 PM Vladimir Sementsov-Ogievskiy
<vsement...@virtuozzo.com> wrote:
>
> 29.09.2021 19:00, Nir Soffer wrote:
> > On Wed, Sep 29, 2021 at 4:37 PM Vladimir Sementsov-Ogievskiy
> > <vsement...@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 <vsement...@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
>
