Re: [PATCH 3/4] qemu-img: add --shallow option for qemu-img compare --stat

[Vladimir Sementsov-Ogievskiy]

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


--
Best regards,
Vladimir