| Message ID | 20170613121639.17853-2-pbutsykin@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017-06-13 14:16, Pavel Butsykin wrote: > The flag as additional precaution of data loss. Perhaps in the future the > operation shrink without this flag will be banned, but while we need to > maintain compatibility. > > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > --- > qemu-img-cmds.hx | 4 ++-- > qemu-img.c | 15 +++++++++++++++ > qemu-img.texi | 5 ++++- > tests/qemu-iotests/102 | 4 ++-- > 4 files changed, 23 insertions(+), 5 deletions(-) > > 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 0ad698d7f1..bfe5f61b0b 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 { > @@ -3452,6 +3453,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. */ > @@ -3469,6 +3471,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", > @@ -3503,6 +3506,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) { > @@ -3562,6 +3568,15 @@ static int img_resize(int argc, char **argv) > goto out; > } > > + if (total_size < blk_getlength(blk) && !shrink) { > + qprintf(quiet, "Warning: shrinking of the image can lead to data loss. " I think this should always be printed to stderr, even if quiet is true; especially considering we (or at least I) plan to make it mandatory. > + "Before performing shrink operation you must make sure " *Before performaing a shrink operation Also, I'd rather use the imperative: "Before ... operation, make sure that the..." (English isn't my native language either, but as far as I remember "must" is generally used for external influences. But it's not like a force of nature is forcing you to confirm there's no important data; you can just ignore this advice and see all of your non-backed-up childhood pictures go to /dev/null.) > + "that the shrink part of image doesn't contain important" Hmm... I don't think "shrink part" really works. Maybe the following would work better: 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. > + " data.\n"); > + qprintf(quiet, > + "If you don't want to see this message use --shrink option.\n"); You should make a note that --shrink may (and I hope it will) become necessary in the future, like: 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..c2b694cd00 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,9 @@ 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! > > +If @code{--shrink} is specified, warning about data loss doesn't print for > +the shrink operation. > + Maybe rather: @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. The functional changes look good to me; even though I'd rather have it an error for qcow2 now (even if that means having to check the image format in img_resize(), and being inconsistent because you wouldn't need --shrink for raw, but for qcow2 you would). But, well, I'm not going to stop this series over that. Max
On 22.06.2017 01:17, Max Reitz wrote: > On 2017-06-13 14:16, Pavel Butsykin wrote: >> The flag as additional precaution of data loss. Perhaps in the future the >> operation shrink without this flag will be banned, but while we need to >> maintain compatibility. >> >> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> >> --- >> qemu-img-cmds.hx | 4 ++-- >> qemu-img.c | 15 +++++++++++++++ >> qemu-img.texi | 5 ++++- >> tests/qemu-iotests/102 | 4 ++-- >> 4 files changed, 23 insertions(+), 5 deletions(-) >> >> 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 0ad698d7f1..bfe5f61b0b 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 { >> @@ -3452,6 +3453,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. */ >> @@ -3469,6 +3471,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", >> @@ -3503,6 +3506,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) { >> @@ -3562,6 +3568,15 @@ static int img_resize(int argc, char **argv) >> goto out; >> } >> >> + if (total_size < blk_getlength(blk) && !shrink) { >> + qprintf(quiet, "Warning: shrinking of the image can lead to data loss. " > > I think this should always be printed to stderr, even if quiet is true; > especially considering we (or at least I) plan to make it mandatory. > OK. >> + "Before performing shrink operation you must make sure " > > *Before performaing a shrink operation s/performaing/performing/ > Also, I'd rather use the imperative: "Before ... operation, make sure > that the..." > > (English isn't my native language either, but as far as I remember > "must" is generally used for external influences. But it's not like a > force of nature is forcing you to confirm there's no important data; you > can just ignore this advice and see all of your non-backed-up childhood > pictures go to /dev/null.) > Yes, It should be just a recommendation. (As you might have already guessed, English isn't my native language too :) I'm glad you are understanding. Really thanks for helping to improve the text.) >> + "that the shrink part of image doesn't contain important" > > Hmm... I don't think "shrink part" really works. > > Maybe the following would work better: > > 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. > >> + " data.\n"); >> + qprintf(quiet, >> + "If you don't want to see this message use --shrink option.\n"); > > You should make a note that --shrink may (and I hope it will) become > necessary in the future, like: > > Using the --shrink option will suppress this message. Note that future > versions of qemu-img may refuse to shrink images without this option! > will fix. >> + } >> + >> 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..c2b694cd00 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,9 @@ 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! >> >> +If @code{--shrink} is specified, warning about data loss doesn't print for >> +the shrink operation. >> + > > Maybe rather: > > @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. > good. > The functional changes look good to me; even though I'd rather have it > an error for qcow2 now (even if that means having to check the image > format in img_resize(), and being inconsistent because you wouldn't need > --shrink for raw, but for qcow2 you would). But, well, I'm not going to > stop this series over that. > Why shrink for qcow2 image is dangerous, but for raw is not dangerous? I think we should provide the same behavior for all formats. When --shrink option will become necessary, it also should be the same for all image formats. > Max >
Am 22.06.2017 um 15:54 hat Pavel Butsykin geschrieben: > On 22.06.2017 01:17, Max Reitz wrote: > >On 2017-06-13 14:16, Pavel Butsykin wrote: > >>The flag as additional precaution of data loss. Perhaps in the future the > >>operation shrink without this flag will be banned, but while we need to > >>maintain compatibility. > >> > >>Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > >The functional changes look good to me; even though I'd rather have it > >an error for qcow2 now (even if that means having to check the image > >format in img_resize(), and being inconsistent because you wouldn't need > >--shrink for raw, but for qcow2 you would). But, well, I'm not going to > >stop this series over that. > > > > Why shrink for qcow2 image is dangerous, but for raw is not dangerous? I > think we should provide the same behavior for all formats. When --shrink > option will become necessary, it also should be the same for all image > formats. It is dangerous for both, but for raw we can't enforce the flag immediately without a deprecation period. With qcow2 we can (because it is new functionality), so we might as well enforce it from the start. Kevin
On 22.06.2017 17:49, Kevin Wolf wrote: > Am 22.06.2017 um 15:54 hat Pavel Butsykin geschrieben: >> On 22.06.2017 01:17, Max Reitz wrote: >>> On 2017-06-13 14:16, Pavel Butsykin wrote: >>>> The flag as additional precaution of data loss. Perhaps in the future the >>>> operation shrink without this flag will be banned, but while we need to >>>> maintain compatibility. >>>> >>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> > >>> The functional changes look good to me; even though I'd rather have it >>> an error for qcow2 now (even if that means having to check the image >>> format in img_resize(), and being inconsistent because you wouldn't need >>> --shrink for raw, but for qcow2 you would). But, well, I'm not going to >>> stop this series over that. >>> >> >> Why shrink for qcow2 image is dangerous, but for raw is not dangerous? I >> think we should provide the same behavior for all formats. When --shrink >> option will become necessary, it also should be the same for all image >> formats. > > It is dangerous for both, but for raw we can't enforce the flag > immediately without a deprecation period. With qcow2 we can (because it > is new functionality), so we might as well enforce it from the start. > Ah, exactly. I like the offer to print the warning for raw and enforce the flag for other formats. > Kevin >
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 0ad698d7f1..bfe5f61b0b 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 { @@ -3452,6 +3453,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. */ @@ -3469,6 +3471,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", @@ -3503,6 +3506,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) { @@ -3562,6 +3568,15 @@ static int img_resize(int argc, char **argv) goto out; } + if (total_size < blk_getlength(blk) && !shrink) { + qprintf(quiet, "Warning: shrinking of the image can lead to data loss. " + "Before performing shrink operation you must make sure " + "that the shrink part of image doesn't contain important" + " data.\n"); + qprintf(quiet, + "If you don't want to see this message use --shrink option.\n"); + } + 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..c2b694cd00 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,9 @@ 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! +If @code{--shrink} is specified, warning about data loss doesn't print for +the shrink operation. + 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 banned, but while we need to maintain compatibility. Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com> --- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 15 +++++++++++++++ qemu-img.texi | 5 ++++- tests/qemu-iotests/102 | 4 ++-- 4 files changed, 23 insertions(+), 5 deletions(-)