Re: [Qemu-devel] [PATCH] Block layer core: Fix qemu-img 'amend' subcommand failure of adjusting backing file in different path

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5238 bytes --]

On 04/21/2017 05:34 PM, Ping Li wrote:

Subject line is too long.  Better would be:

block: Fix qemu-img amend failure

Your system clock is WAAY off.  According to headers:

> Received: from notes_smtp.zte.com.cn ([10.30.1.239])
> 	by mse01.zte.com.cn with ESMTP id v3L6FjYn087324;
> 	Fri, 21 Apr 2017 14:15:45 +0800 (GMT-8)
> 	(envelope-from li.ping288@zte.com.cn)
> Received: from localhost.localdomain ([10.74.120.79])
> 	by szsmtp06.zte.com.cn (Lotus Domino Release 8.5.3FP6)
> 	with ESMTP id 2017042114155070-1062351 ;
> 	Fri, 21 Apr 2017 14:15:50 +0800 
> From: Ping Li <li.ping288@zte.com.cn>
> To: kwolf@redhat.com, mreitz@redhat.com
> Date: Sat, 22 Apr 2017 06:34:32 +0800

you sent this at 22:34 UTC, but the next hop in the chain received it at
6:15 UTC (16 hours earlier!).  Incorrect timestamps mess with mail
clients that like to sort threads based on time of most recent activity.


> Currently, qemu-img 'amend' subcommand would fail to adjust image's backing file
> which was moved into different path.
> For example, parent.qcow2, the backing file of leaf.qcow2, first is at /home/a/,
> then moved into /home/b/. Originally this command,
>  "qemu-img amend -f qcow2 -o backing_fmt=qcow2,backing_file=/home/b/parent.qcow2 leaf.qcow2",
>  would fail because qemu-img failed to open the old backing file of leaf.qcow2.
> Give the 'amend' subcommand a '-u' option to not open the old backing file
>  while openning leaf.qcow2.

s/openning/opening/

> 
> Signed-off-by: Li Ping<li.ping288@zte.com.cn>

Space before < is typical in git credits.

> ---
>  qemu-img.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 

General question before getting into the review - do we really need
amend -u, or should this behavior be automatic (that is, why do we need
access to the old backing image in the first place - can't we get along
with ONLY access to the new backing image always, when the amend options
include an update to the backing image)?

> diff --git a/qemu-img.c b/qemu-img.c
> index b220cf7..a32e9d6 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -145,9 +145,10 @@ static void QEMU_NORETURN help(void)
>             "  'snapshot_id_or_name' is deprecated, use 'snapshot_param'\n"
>             "    instead\n"
>             "  '-c' indicates that target image must be compressed (qcow format only)\n"
> -           "  '-u' enables unsafe rebasing. It is assumed that old and new backing file\n"
> -           "       match exactly. The image doesn't need a working backing file before\n"
> -           "       rebasing in this case (useful for renaming the backing file)\n"
> +           "  '-u' enables unsafe rebasing or amending. It is assumed that old and new\n"
> +           "       backing file match exactly. For rebasing, the image doesn't need a working\n"
> +           "       backing file before rebasing in this case(useful for renaming the backing file).\n"

Space before ( in English.

> +           "       For amending, it doesn't open backing file(useful for moving images)\n"

and again

>             "  '-h' with or without a command shows this help and lists the supported formats\n"
>             "  '-p' show progress of command (only certain commands)\n"
>             "  '-q' use Quiet mode - do not print any output (except errors)\n"
> @@ -3538,7 +3539,7 @@ static int img_amend(int argc, char **argv)
>      QemuOptsList *create_opts = NULL;
>      QemuOpts *opts = NULL;
>      const char *fmt = NULL, *filename, *cache;
> -    int flags;
> +    int flags = 0;
>      bool writethrough;
>      bool quiet = false, progress = false;
>      BlockBackend *blk = NULL;
> @@ -3553,7 +3554,7 @@ static int img_amend(int argc, char **argv)
>              {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
>              {0, 0, 0, 0}
>          };
> -        c = getopt_long(argc, argv, ":ho:f:t:pq",
> +        c = getopt_long(argc, argv, ":ho:f:t:pqu",
>                          long_options, NULL);
>          if (c == -1) {
>              break;
> @@ -3595,6 +3596,9 @@ static int img_amend(int argc, char **argv)
>          case 'q':
>              quiet = true;
>              break;
> +        case 'u':
> +            flags |= BDRV_O_NO_BACKING;

Does this really do what we want? When amending an image, our choice of
whether to expand a cluster to all zeroes depends heavily on qcow2
version AND whether there is a backing file; will this flag make us do
the wrong thing when converting a v3 image back to v2?

> +            break;
>          case OPTION_OBJECT:
>              opts = qemu_opts_parse_noisily(&qemu_object_opts,
>                                             optarg, true);
> @@ -3639,7 +3643,7 @@ static int img_amend(int argc, char **argv)
>          goto out;
>      }
>  
> -    flags = BDRV_O_RDWR;
> +    flags |= BDRV_O_RDWR;
>      ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
>      if (ret < 0) {
>          error_report("Invalid cache option: %s", cache);
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]