Re: [Qemu-devel] [PATCH v2 5/6] qemu-img: Specify backing file for commit

[Fam Zheng]

On Tue, 04/08 14:50, Max Reitz wrote:
> Introduce a new parameter for qemu-img commit which may be used to
> explicitly specify the backing file unto which an image should be
> committed if the backing chain has more than a single layer.
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  qemu-img-cmds.hx |  4 ++--
>  qemu-img.c       | 22 +++++++++++++++-------
>  qemu-img.texi    |  8 +++++++-
>  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 8bc55cd..7f62f6d 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -22,9 +22,9 @@ STEXI
>  ETEXI
>  
>  DEF("commit", img_commit,
> -    "commit [-q] [-f fmt] [-t cache] [-p] filename")
> +    "commit [-q] [-f fmt] [-t cache] [-b backing_file] [-p] filename")
>  STEXI
> address@hidden commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename}
> address@hidden commit [-q] [-f @var{fmt}] [-t @var{cache}] [-b 
> @var{backing_file}] [-p] @var{filename}
>  ETEXI
>  
>  DEF("compare", img_compare,
> diff --git a/qemu-img.c b/qemu-img.c
> index 0a9eff7..9d4bdbc 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -725,15 +725,16 @@ static void run_block_job(BlockJob *job, Error **errp)
>  static int img_commit(int argc, char **argv)
>  {
>      int c, ret, flags;
> -    const char *filename, *fmt, *cache;
> +    const char *filename, *fmt, *cache, *base;
>      BlockDriverState *bs, *base_bs;
>      bool progress = false, quiet = false;
>      Error *local_err = NULL;
>  
>      fmt = NULL;
>      cache = BDRV_DEFAULT_CACHE;
> +    base = NULL;
>      for(;;) {
> -        c = getopt(argc, argv, "f:ht:qp");
> +        c = getopt(argc, argv, "f:ht:b:qp");
>          if (c == -1) {
>              break;
>          }
> @@ -748,6 +749,9 @@ static int img_commit(int argc, char **argv)
>          case 't':
>              cache = optarg;
>              break;
> +        case 'b':
> +            base = optarg;
> +            break;
>          case 'p':
>              progress = true;
>              break;
> @@ -782,12 +786,16 @@ static int img_commit(int argc, char **argv)
>      qemu_progress_init(progress, 1.f);
>      qemu_progress_print(0.f, 100);
>  
> -    /* This is different from QMP, which by default uses the deepest file in 
> the
> -     * backing chain (i.e., the very base); however, the traditional 
> behavior of
> -     * qemu-img commit is using the immediate backing file. */
> -    base_bs = bs->backing_hd;
> +    if (base) {
> +        base_bs = bdrv_find_backing_image(bs, base);
> +    } else {
> +        /* This is different from QMP, which by default uses the deepest 
> file in
> +         * the backing chain (i.e., the very base); however, the traditional
> +         * behavior of qemu-img commit is using the immediate backing file. 
> */
> +        base_bs = bs->backing_hd;
> +    }
>      if (!base_bs) {
> -        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
> +        error_set(&local_err, QERR_BASE_NOT_FOUND, base ?: "NULL");
>          goto done;
>      }
>  
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 1a9c08f..4a9f493 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -140,7 +140,7 @@ this case. @var{backing_file} will never be modified 
> unless you use the
>  The size can also be specified using the @var{size} option with @code{-o},
>  it doesn't need to be specified separately in this case.
>  
> address@hidden commit [-q] [-f @var{fmt}] [-t @var{cache}] [-p] @var{filename}
> address@hidden commit [-q] [-f @var{fmt}] [-t @var{cache}] [-b 
> @var{backing_file}] [-p] @var{filename}
>  
>  Commit the changes recorded in @var{filename} in its base image or backing 
> file.
>  If the backing file is smaller than the snapshot, then the backing file will 
> be
> @@ -149,6 +149,12 @@ the backing file, the backing file will not be 
> truncated.  If you want the
>  backing file to match the size of the smaller snapshot, you can safely 
> truncate
>  it yourself once the commit operation successfully completes.
>  
> +If the backing chain of the given image file @var{filename} has more than one
> +layer, the backing file unto which the changes shall be committed may be
> +specified as @var{backing_file} (which has to be part of @var{filename}'s
> +backing chain). If @var{filename} is not specified, the immediate backing 
> file

s/@var{filename}/@var{backing_file}/ ?

BTW how about just calling it 'base' as in qmp commands, because backing_file
has usages in (slightly) different context of create.

Other than the two questions,

Reviewed-by: Fam Zheng <address@hidden>

> +of the top image (which is @var{filename}) will be used.
> +
>  @item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} 
> @var{filename2}
>  
>  Check if two images have the same content. You can compare images with
> -- 
> 1.9.1
> 
>