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 <pbutsy...@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


Reply via email to