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.ping...@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.ping...@zte.com.cn> > To: kw...@redhat.com, mre...@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.ping...@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
signature.asc
Description: OpenPGP digital signature