Block layer core: Fix qemu-img 'amend' subcommand failure of adjusting backing file in different path

Message ID 1492814072-4276-1-git-send-email-li.ping288@zte.com.cn
State New
Headers show

Commit Message

Ping Li April 21, 2017, 10:34 p.m.
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.

Signed-off-by: Li Ping<li.ping288@zte.com.cn>
---
 qemu-img.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Eric Blake April 21, 2017, 1:53 p.m. | #1
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);
>
Max Reitz April 22, 2017, 4:52 p.m. | #2
On 22.04.2017 00:34, Ping Li wrote:
> 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.
> 
> Signed-off-by: Li Ping<li.ping288@zte.com.cn>
> ---
>  qemu-img.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)

So why can't you just use rebase -u?

I'm not completely opposed to adding this functionality to amend, but
I'd actually rather remove the ability to change the backing file from
amend than to add functionality that may turn out to be rather
complicated to implement...

As Eric has proposed, I also think we could turn on BDRV_O_NO_BACKING
automatically if the option list consists of nothing but backing_file
and backing_fmt. But then we'd have to think about the case where the
user gives both backing_file and some other option at the same time; it
may be unexpected that we do open the backing file in that case. The
best might actually to error out so the user is forced to do both
changes separately -- but at that point the logic gets so complicated
that I think we should at most add a note to qemu-img's man page that
qemu-img amend will open the backing file and that users should use
rebase -u if that is not desired...

Max

Patch

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"
+           "       For amending, it doesn't open backing file(useful for moving images)\n"
            "  '-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;
+            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);