Re: [Qemu-devel] [PATCH v2 1/4] qemu-img: add --shrink flag for resize

[Pavel Butsykin <pbutsykin@virtuozzo.com>]

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
>