| Message ID | 20170707143028.692-2-pbutsykin@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 07/07/2017 09:30 AM, Pavel Butsykin wrote: > The flag as additional precaution of data loss. Perhaps in the future the s/as/is/ s/of/against/ > operation shrink without this flag will be blocked for all formats, but while s/while/for now/ > we need to maintain compatibility with raw. > > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > --- > qemu-img-cmds.hx | 4 ++-- > qemu-img.c | 23 +++++++++++++++++++++++ > qemu-img.texi | 7 ++++++- > tests/qemu-iotests/102 | 4 ++-- > 4 files changed, 33 insertions(+), 5 deletions(-) > > @@ -3568,6 +3574,23 @@ static int img_resize(int argc, char **argv) > goto out; > } > > + if (total_size < blk_getlength(blk) && !shrink) { > + error_report("Warning: Shrinking an image will delete all data beyond" Alistair has a pending thread to create a unified warning function; if that lands first, you'll have to tweak this (if yours lands first, it's one more place for that series to clean up). > + "the shrunken image's end. Before performing such an" > + "operation, make sure there is no important data there."); > + > + if (g_strcmp0(bdrv_get_format_name(blk_bs(blk)), "raw") != 0) { > + error_report( > + "Use the --shrink option to perform a shrink operation."); > + ret = -1; > + goto out; > + } else { > + error_report("Using the --shrink option will suppress this message." > + "Note that future versions of qemu-img may refuse to " > + "shrink images without this option!"); No need to shout at the user ('.' is better than '!') > + } > + } > + > ret = blk_truncate(blk, total_size, &err); > if (!ret) { > qprintf(quiet, "Image resized.\n"); > diff --git a/qemu-img.texi b/qemu-img.texi > index 5b925ecf41..6324abef48 100644 > --- a/qemu-img.texi > +++ b/qemu-img.texi > @@ -499,7 +499,7 @@ qemu-img rebase -b base.img diff.qcow2 > At this point, @code{modified.img} can be discarded, since > @code{base.img + diff.qcow2} contains the same information. > > -@item resize @var{filename} [+ | -]@var{size} > +@item resize [--shrink] @var{filename} [+ | -]@var{size} > > Change the disk image as if it had been created with @var{size}. > > @@ -507,6 +507,11 @@ Before using this command to shrink a disk image, you MUST use file system and > partitioning tools inside the VM to reduce allocated file systems and partition > sizes accordingly. Failure to do so will result in data loss! > > +@code{--shrink} informs qemu-img that the user is certain about wanting > +to shrink an image and is aware that any data beyond the truncated > +image's end will be lost. Trying to shrink an image without this option > +results in a warning; future versions may make it an error. You made it an error for all but raw already, but I'm not sure how wordy we want this to be. > + > After using this command to grow a disk image, you must use file system and > partitioning tools inside the VM to actually begin using the new space on the > device. > diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102 > index 87db1bb1bf..d7ad8d9840 100755 > --- a/tests/qemu-iotests/102 > +++ b/tests/qemu-iotests/102 > @@ -54,7 +54,7 @@ _make_test_img $IMG_SIZE > $QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io > # Remove data cluster from image (first cluster: image header, second: reftable, > # third: refblock, fourth: L1 table, fifth: L2 table) > -$QEMU_IMG resize -f raw "$TEST_IMG" $((5 * 64 * 1024)) > +$QEMU_IMG resize -f raw --shrink "$TEST_IMG" $((5 * 64 * 1024)) > > $QEMU_IO -c map "$TEST_IMG" > $QEMU_IMG map "$TEST_IMG" > @@ -69,7 +69,7 @@ $QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io > > qemu_comm_method=monitor _launch_qemu -drive if=none,file="$TEST_IMG",id=drv0 > > -$QEMU_IMG resize -f raw "$TEST_IMG" $((5 * 64 * 1024)) > +$QEMU_IMG resize -f raw --shrink "$TEST_IMG" $((5 * 64 * 1024)) This tests a successful shrink. Please also test the error message when attempting to shrink but --shrink was not supplied.
On 2017-07-07 23:12, Eric Blake wrote: > On 07/07/2017 09:30 AM, Pavel Butsykin wrote: >> The flag as additional precaution of data loss. Perhaps in the future the > > s/as/is/ s/of/against/ > >> operation shrink without this flag will be blocked for all formats, but while > > s/while/for now/ > >> we need to maintain compatibility with raw. >> >> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >> --- >> qemu-img-cmds.hx | 4 ++-- >> qemu-img.c | 23 +++++++++++++++++++++++ >> qemu-img.texi | 7 ++++++- >> tests/qemu-iotests/102 | 4 ++-- >> 4 files changed, 33 insertions(+), 5 deletions(-) >> > >> @@ -3568,6 +3574,23 @@ static int img_resize(int argc, char **argv) >> goto out; >> } >> >> + if (total_size < blk_getlength(blk) && !shrink) { >> + error_report("Warning: Shrinking an image will delete all data beyond" > > Alistair has a pending thread to create a unified warning function; if > that lands first, you'll have to tweak this (if yours lands first, it's > one more place for that series to clean up). > >> + "the shrunken image's end. Before performing such an" >> + "operation, make sure there is no important data there."); >> + >> + if (g_strcmp0(bdrv_get_format_name(blk_bs(blk)), "raw") != 0) { >> + error_report( >> + "Use the --shrink option to perform a shrink operation."); >> + ret = -1; >> + goto out; >> + } else { >> + error_report("Using the --shrink option will suppress this message." >> + "Note that future versions of qemu-img may refuse to " >> + "shrink images without this option!"); > > No need to shout at the user ('.' is better than '!') > >> + } >> + } >> + >> ret = blk_truncate(blk, total_size, &err); >> if (!ret) { >> qprintf(quiet, "Image resized.\n"); >> diff --git a/qemu-img.texi b/qemu-img.texi >> index 5b925ecf41..6324abef48 100644 >> --- a/qemu-img.texi >> +++ b/qemu-img.texi >> @@ -499,7 +499,7 @@ qemu-img rebase -b base.img diff.qcow2 >> At this point, @code{modified.img} can be discarded, since >> @code{base.img + diff.qcow2} contains the same information. >> >> -@item resize @var{filename} [+ | -]@var{size} >> +@item resize [--shrink] @var{filename} [+ | -]@var{size} >> >> Change the disk image as if it had been created with @var{size}. >> >> @@ -507,6 +507,11 @@ Before using this command to shrink a disk image, you MUST use file system and >> partitioning tools inside the VM to reduce allocated file systems and partition >> sizes accordingly. Failure to do so will result in data loss! >> >> +@code{--shrink} informs qemu-img that the user is certain about wanting >> +to shrink an image and is aware that any data beyond the truncated >> +image's end will be lost. Trying to shrink an image without this option >> +results in a warning; future versions may make it an error. > > You made it an error for all but raw already, but I'm not sure how wordy > we want this to be. Easy, just make it "...image's end will be lost. This option must always be specified when shrinking images." (Or even just put it up front: "When shrinking images, the @code{--shrink} option must be given. This informs qemu-img that the user acknowledges all loss of data beyond the truncated image's end.") Sure, this is not entirely correct for raw images right now, but we basically want it to be mandatory there, too. We just cannot do it for compatibility reasons(TM) yet -- it won't hurt to tell users the future-proof way already. >> + >> After using this command to grow a disk image, you must use file system and >> partitioning tools inside the VM to actually begin using the new space on the >> device. >> diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102 >> index 87db1bb1bf..d7ad8d9840 100755 >> --- a/tests/qemu-iotests/102 >> +++ b/tests/qemu-iotests/102 >> @@ -54,7 +54,7 @@ _make_test_img $IMG_SIZE >> $QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io >> # Remove data cluster from image (first cluster: image header, second: reftable, >> # third: refblock, fourth: L1 table, fifth: L2 table) >> -$QEMU_IMG resize -f raw "$TEST_IMG" $((5 * 64 * 1024)) >> +$QEMU_IMG resize -f raw --shrink "$TEST_IMG" $((5 * 64 * 1024)) >> >> $QEMU_IO -c map "$TEST_IMG" >> $QEMU_IMG map "$TEST_IMG" >> @@ -69,7 +69,7 @@ $QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io >> >> qemu_comm_method=monitor _launch_qemu -drive if=none,file="$TEST_IMG",id=drv0 >> >> -$QEMU_IMG resize -f raw "$TEST_IMG" $((5 * 64 * 1024)) >> +$QEMU_IMG resize -f raw --shrink "$TEST_IMG" $((5 * 64 * 1024)) > > This tests a successful shrink. Please also test the error message when > attempting to shrink but --shrink was not supplied. This doesn't test this, it just modifies an existing test to continue to work. The test is added in patch 4. With the spelling fixed as proposed by Eric, and the man page indeed telling users to always use --shrink: Reviewed-by: Max Reitz <mreitz@redhat.com>
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index a39fcdba71..3b2eab9d20 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -76,9 +76,9 @@ STEXI ETEXI DEF("resize", img_resize, - "resize [--object objectdef] [--image-opts] [-q] filename [+ | -]size") + "resize [--object objectdef] [--image-opts] [-q] [--shrink] filename [+ | -]size") STEXI -@item resize [--object @var{objectdef}] [--image-opts] [-q] @var{filename} [+ | -]@var{size} +@item resize [--object @var{objectdef}] [--image-opts] [-q] [--shrink] @var{filename} [+ | -]@var{size} ETEXI DEF("amend", img_amend, diff --git a/qemu-img.c b/qemu-img.c index 91ad6bebbf..9773e835e4 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -61,6 +61,7 @@ enum { OPTION_FLUSH_INTERVAL = 261, OPTION_NO_DRAIN = 262, OPTION_TARGET_IMAGE_OPTS = 263, + OPTION_SHRINK = 264, }; typedef enum OutputFormat { @@ -3458,6 +3459,7 @@ static int img_resize(int argc, char **argv) }, }; bool image_opts = false; + bool shrink = false; /* Remove size from argv manually so that negative numbers are not treated * as options by getopt. */ @@ -3475,6 +3477,7 @@ static int img_resize(int argc, char **argv) {"help", no_argument, 0, 'h'}, {"object", required_argument, 0, OPTION_OBJECT}, {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS}, + {"shrink", no_argument, 0, OPTION_SHRINK}, {0, 0, 0, 0} }; c = getopt_long(argc, argv, ":f:hq", @@ -3509,6 +3512,9 @@ static int img_resize(int argc, char **argv) case OPTION_IMAGE_OPTS: image_opts = true; break; + case OPTION_SHRINK: + shrink = true; + break; } } if (optind != argc - 1) { @@ -3568,6 +3574,23 @@ static int img_resize(int argc, char **argv) goto out; } + if (total_size < blk_getlength(blk) && !shrink) { + error_report("Warning: Shrinking an image will delete all data beyond" + "the shrunken image's end. Before performing such an" + "operation, make sure there is no important data there."); + + if (g_strcmp0(bdrv_get_format_name(blk_bs(blk)), "raw") != 0) { + error_report( + "Use the --shrink option to perform a shrink operation."); + ret = -1; + goto out; + } else { + error_report("Using the --shrink option will suppress this message." + "Note that future versions of qemu-img may refuse to " + "shrink images without this option!"); + } + } + ret = blk_truncate(blk, total_size, &err); if (!ret) { qprintf(quiet, "Image resized.\n"); diff --git a/qemu-img.texi b/qemu-img.texi index 5b925ecf41..6324abef48 100644 --- a/qemu-img.texi +++ b/qemu-img.texi @@ -499,7 +499,7 @@ qemu-img rebase -b base.img diff.qcow2 At this point, @code{modified.img} can be discarded, since @code{base.img + diff.qcow2} contains the same information. -@item resize @var{filename} [+ | -]@var{size} +@item resize [--shrink] @var{filename} [+ | -]@var{size} Change the disk image as if it had been created with @var{size}. @@ -507,6 +507,11 @@ Before using this command to shrink a disk image, you MUST use file system and partitioning tools inside the VM to reduce allocated file systems and partition sizes accordingly. Failure to do so will result in data loss! +@code{--shrink} informs qemu-img that the user is certain about wanting +to shrink an image and is aware that any data beyond the truncated +image's end will be lost. Trying to shrink an image without this option +results in a warning; future versions may make it an error. + After using this command to grow a disk image, you must use file system and partitioning tools inside the VM to actually begin using the new space on the device. diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102 index 87db1bb1bf..d7ad8d9840 100755 --- a/tests/qemu-iotests/102 +++ b/tests/qemu-iotests/102 @@ -54,7 +54,7 @@ _make_test_img $IMG_SIZE $QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io # Remove data cluster from image (first cluster: image header, second: reftable, # third: refblock, fourth: L1 table, fifth: L2 table) -$QEMU_IMG resize -f raw "$TEST_IMG" $((5 * 64 * 1024)) +$QEMU_IMG resize -f raw --shrink "$TEST_IMG" $((5 * 64 * 1024)) $QEMU_IO -c map "$TEST_IMG" $QEMU_IMG map "$TEST_IMG" @@ -69,7 +69,7 @@ $QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io qemu_comm_method=monitor _launch_qemu -drive if=none,file="$TEST_IMG",id=drv0 -$QEMU_IMG resize -f raw "$TEST_IMG" $((5 * 64 * 1024)) +$QEMU_IMG resize -f raw --shrink "$TEST_IMG" $((5 * 64 * 1024)) _send_qemu_cmd $QEMU_HANDLE 'qemu-io drv0 map' 'allocated' \ | sed -e 's/^(qemu).*qemu-io drv0 map...$/(qemu) qemu-io drv0 map/'
The flag as additional precaution of data loss. Perhaps in the future the operation shrink without this flag will be blocked for all formats, but while we need to maintain compatibility with raw. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 23 +++++++++++++++++++++++ qemu-img.texi | 7 ++++++- tests/qemu-iotests/102 | 4 ++-- 4 files changed, 33 insertions(+), 5 deletions(-)