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(-)
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);
--
1.8.3.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);
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
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
© 2016 - 2024 Red Hat, Inc.