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

[Nir Soffer]

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