[Qemu-devel] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Qemu-devel] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Fam Zheng]

Management and users are accustomed to "qemu-img info" to query status of
images even when they are used by guests. Since image locking was added, the -U
(--force-share) option is needed for that to work. The reason has been that due
to possible race with image header update, the output can be misleading.

But what are likely to happen after we emit the error are that, for interactive
users, '-U' will be used and the command retried; for management (nova, RHV,
etc.), the operation is broken with no knob to workaround this.

This series changes that error to a warning so that it doesn't get in the way.

Fam

Fam Zheng (2):
  qemu-img: Move img_open error reporting to callers
  qemu-img: info: try -U automatically

 qemu-img.c                 | 129 +++++++++++++++++++++++++++++----------------
 tests/qemu-iotests/043.out |   6 +--
 tests/qemu-iotests/153.out |   3 +-
 3 files changed, 88 insertions(+), 50 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/2] qemu-img: Move img_open error reporting to callers

[Fam Zheng]

In the next patch one caller will have a special error handling logic
than reporting it, add "Error **" parameters to functions and give
control back to callers, to make that possible.

Update iotests output accordingly.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.c                 | 115 +++++++++++++++++++++++++++------------------
 tests/qemu-iotests/043.out |   6 +--
 2 files changed, 73 insertions(+), 48 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 68b375f998..7d3171c20c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -268,24 +268,22 @@ static int print_block_option_help(const char *filename, const char *fmt)
 
 static BlockBackend *img_open_opts(const char *optstr,
                                    QemuOpts *opts, int flags, bool writethrough,
-                                   bool quiet, bool force_share)
+                                   bool quiet, bool force_share, Error **errp)
 {
     QDict *options;
-    Error *local_err = NULL;
     BlockBackend *blk;
     options = qemu_opts_to_qdict(opts, NULL);
     if (force_share) {
         if (qdict_haskey(options, BDRV_OPT_FORCE_SHARE)
             && !qdict_get_bool(options, BDRV_OPT_FORCE_SHARE)) {
-            error_report("--force-share/-U conflicts with image options");
+            error_setg(errp, "--force-share/-U conflicts with image options");
             QDECREF(options);
             return NULL;
         }
         qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
     }
-    blk = blk_new_open(NULL, NULL, options, flags, &local_err);
+    blk = blk_new_open(NULL, NULL, options, flags, errp);
     if (!blk) {
-        error_reportf_err(local_err, "Could not open '%s': ", optstr);
         return NULL;
     }
     blk_set_enable_write_cache(blk, !writethrough);
@@ -297,10 +295,10 @@ static BlockBackend *img_open_file(const char *filename,
                                    QDict *options,
                                    const char *fmt, int flags,
                                    bool writethrough, bool quiet,
-                                   bool force_share)
+                                   bool force_share,
+                                   Error **errp)
 {
     BlockBackend *blk;
-    Error *local_err = NULL;
 
     if (!options) {
         options = qdict_new();
@@ -312,9 +310,8 @@ static BlockBackend *img_open_file(const char *filename,
     if (force_share) {
         qdict_put_bool(options, BDRV_OPT_FORCE_SHARE, true);
     }
-    blk = blk_new_open(filename, NULL, options, flags, &local_err);
+    blk = blk_new_open(filename, NULL, options, flags, errp);
     if (!blk) {
-        error_reportf_err(local_err, "Could not open '%s': ", filename);
         return NULL;
     }
     blk_set_enable_write_cache(blk, !writethrough);
@@ -340,7 +337,8 @@ static BlockBackend *img_open_new_file(const char *filename,
                                        QemuOpts *create_opts,
                                        const char *fmt, int flags,
                                        bool writethrough, bool quiet,
-                                       bool force_share)
+                                       bool force_share,
+                                       Error **errp)
 {
     QDict *options = NULL;
 
@@ -348,32 +346,32 @@ static BlockBackend *img_open_new_file(const char *filename,
     qemu_opt_foreach(create_opts, img_add_key_secrets, options, &error_abort);
 
     return img_open_file(filename, options, fmt, flags, writethrough, quiet,
-                         force_share);
+                         force_share, errp);
 }
 
 
 static BlockBackend *img_open(bool image_opts,
                               const char *filename,
                               const char *fmt, int flags, bool writethrough,
-                              bool quiet, bool force_share)
+                              bool quiet, bool force_share, Error **errp)
 {
     BlockBackend *blk;
     if (image_opts) {
         QemuOpts *opts;
         if (fmt) {
-            error_report("--image-opts and --format are mutually exclusive");
+            error_setg(errp,
+                       "--image-opts and --format are mutually exclusive");
             return NULL;
         }
-        opts = qemu_opts_parse_noisily(qemu_find_opts("source"),
-                                       filename, true);
+        opts = qemu_opts_parse(qemu_find_opts("source"), filename, true, errp);
         if (!opts) {
             return NULL;
         }
         blk = img_open_opts(filename, opts, flags, writethrough, quiet,
-                            force_share);
+                            force_share, errp);
     } else {
         blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet,
-                            force_share);
+                            force_share, errp);
     }
     return blk;
 }
@@ -670,6 +668,7 @@ static int img_check(int argc, char **argv)
     bool quiet = false;
     bool image_opts = false;
     bool force_share = false;
+    Error *local_err = NULL;
 
     fmt = NULL;
     output = NULL;
@@ -769,8 +768,9 @@ static int img_check(int argc, char **argv)
     }
 
     blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
-                   force_share);
+                   force_share, &local_err);
     if (!blk) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename);
         return 1;
     }
     bs = blk_bs(blk);
@@ -979,8 +979,9 @@ static int img_commit(int argc, char **argv)
     }
 
     blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
-                   false);
+                   false, &local_err);
     if (!blk) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename);
         return 1;
     }
     bs = blk_bs(blk);
@@ -1251,6 +1252,7 @@ static int img_compare(int argc, char **argv)
     uint64_t progress_base;
     bool image_opts = false;
     bool force_share = false;
+    Error *local_err = NULL;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
@@ -1343,15 +1345,17 @@ static int img_compare(int argc, char **argv)
     }
 
     blk1 = img_open(image_opts, filename1, fmt1, flags, writethrough, quiet,
-                    force_share);
+                    force_share, &local_err);
     if (!blk1) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename1);
         ret = 2;
         goto out3;
     }
 
     blk2 = img_open(image_opts, filename2, fmt2, flags, writethrough, quiet,
-                    force_share);
+                    force_share, &local_err);
     if (!blk2) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename2);
         ret = 2;
         goto out2;
     }
@@ -2121,8 +2125,10 @@ static int img_convert(int argc, char **argv)
     for (bs_i = 0; bs_i < s.src_num; bs_i++) {
         s.src[bs_i] = img_open(image_opts, argv[optind + bs_i],
                                fmt, src_flags, src_writethrough, quiet,
-                               force_share);
+                               force_share, &local_err);
         if (!s.src[bs_i]) {
+            error_reportf_err(local_err, "Could not open '%s': ",
+                              argv[optind + bs_i]);
             ret = -1;
             goto out;
         }
@@ -2273,7 +2279,7 @@ static int img_convert(int argc, char **argv)
 
     if (skip_create) {
         s.target = img_open(tgt_image_opts, out_filename, out_fmt,
-                            flags, writethrough, quiet, false);
+                            flags, writethrough, quiet, false, &local_err);
     } else {
         /* TODO ultimately we should allow --target-image-opts
          * to be used even when -n is not given.
@@ -2281,9 +2287,11 @@ static int img_convert(int argc, char **argv)
          * to allow filenames in option syntax
          */
         s.target = img_open_new_file(out_filename, opts, out_fmt,
-                                     flags, writethrough, quiet, false);
+                                     flags, writethrough, quiet, false,
+                                     &local_err);
     }
     if (!s.target) {
+        error_reportf_err(local_err, "Could not open '%s': ", out_filename);
         ret = -1;
         goto out;
     }
@@ -2439,12 +2447,13 @@ static gboolean str_equal_func(gconstpointer a, gconstpointer b)
 static ImageInfoList *collect_image_info_list(bool image_opts,
                                               const char *filename,
                                               const char *fmt,
-                                              bool chain, bool force_share)
+                                              bool chain, bool force_share,
+                                              Error **errp)
 {
     ImageInfoList *head = NULL;
     ImageInfoList **last = &head;
     GHashTable *filenames;
-    Error *err = NULL;
+    Error *local_err = NULL;
 
     filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
 
@@ -2455,23 +2464,24 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
         ImageInfoList *elem;
 
         if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
-            error_report("Backing file '%s' creates an infinite loop.",
-                         filename);
+            error_setg(errp,
+                       "Backing file '%s' creates an infinite loop.",
+                       filename);
             goto err;
         }
         g_hash_table_insert(filenames, (gpointer)filename, NULL);
 
         blk = img_open(image_opts, filename, fmt,
                        BDRV_O_NO_BACKING | BDRV_O_NO_IO, false, false,
-                       force_share);
+                       force_share, errp);
         if (!blk) {
             goto err;
         }
         bs = blk_bs(blk);
 
-        bdrv_query_image_info(bs, &info, &err);
-        if (err) {
-            error_report_err(err);
+        bdrv_query_image_info(bs, &info, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
             blk_unref(blk);
             goto err;
         }
@@ -2488,9 +2498,10 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
             if (info->has_full_backing_filename) {
                 filename = info->full_backing_filename;
             } else if (info->has_backing_filename) {
-                error_report("Could not determine absolute backing filename,"
-                             " but backing filename '%s' present",
-                             info->backing_filename);
+                error_setg(errp,
+                           "Could not determine absolute backing filename,"
+                           " but backing filename '%s' present",
+                           info->backing_filename);
                 goto err;
             }
             if (info->has_backing_filename_format) {
@@ -2516,6 +2527,7 @@ static int img_info(int argc, char **argv)
     ImageInfoList *list;
     bool image_opts = false;
     bool force_share = false;
+    Error *local_err = NULL;
 
     fmt = NULL;
     output = NULL;
@@ -2592,8 +2604,9 @@ static int img_info(int argc, char **argv)
     }
 
     list = collect_image_info_list(image_opts, filename, fmt, chain,
-                                   force_share);
+                                   force_share, &local_err);
     if (!list) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename);
         return 1;
     }
 
@@ -2739,6 +2752,7 @@ static int img_map(int argc, char **argv)
     int ret = 0;
     bool image_opts = false;
     bool force_share = false;
+    Error *local_err = NULL;
 
     fmt = NULL;
     output = NULL;
@@ -2810,8 +2824,10 @@ static int img_map(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open(image_opts, filename, fmt, 0, false, false, force_share);
+    blk = img_open(image_opts, filename, fmt, 0, false, false, force_share,
+                   &local_err);
     if (!blk) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename);
         return 1;
     }
     bs = blk_bs(blk);
@@ -2961,8 +2977,9 @@ static int img_snapshot(int argc, char **argv)
 
     /* Open the image */
     blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet,
-                   force_share);
+                   force_share, &err);
     if (!blk) {
+        error_reportf_err(err, "Could not open '%s': ", filename);
         return 1;
     }
     bs = blk_bs(blk);
@@ -3147,8 +3164,9 @@ static int img_rebase(int argc, char **argv)
      * the reference to a renamed or moved backing file.
      */
     blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
-                   false);
+                   false, &local_err);
     if (!blk) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename);
         ret = -1;
         goto out;
     }
@@ -3507,8 +3525,9 @@ static int img_resize(int argc, char **argv)
 
     blk = img_open(image_opts, filename, fmt,
                    BDRV_O_RDWR | BDRV_O_RESIZE, false, quiet,
-                   false);
+                   false, &err);
     if (!blk) {
+        error_reportf_err(err, "Could not open '%s': ", filename);
         ret = -1;
         goto out;
     }
@@ -3693,8 +3712,9 @@ static int img_amend(int argc, char **argv)
     }
 
     blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
-                   false);
+                   false, &err);
     if (!blk) {
+        error_reportf_err(err, "Could not open '%s': ", filename);
         ret = -1;
         goto out;
     }
@@ -3862,6 +3882,7 @@ static int img_bench(int argc, char **argv)
     struct timeval t1, t2;
     int i;
     bool force_share = false;
+    Error *local_err = NULL;
 
     for (;;) {
         static const struct option long_options[] = {
@@ -4018,8 +4039,9 @@ static int img_bench(int argc, char **argv)
     }
 
     blk = img_open(image_opts, filename, fmt, flags, writethrough, quiet,
-                   force_share);
+                   force_share, &local_err);
     if (!blk) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename);
         ret = -1;
         goto out;
     }
@@ -4301,9 +4323,10 @@ static int img_dd(int argc, char **argv)
     }
 
     blk1 = img_open(image_opts, in.filename, fmt, 0, false, false,
-                    force_share);
+                    force_share, &local_err);
 
     if (!blk1) {
+        error_reportf_err(local_err, "Could not open '%s': ", in.filename);
         ret = -1;
         goto out;
     }
@@ -4374,10 +4397,11 @@ static int img_dd(int argc, char **argv)
      * support image-opts style.
      */
     blk2 = img_open_file(out.filename, NULL, out_fmt, BDRV_O_RDWR,
-                         false, false, false);
+                         false, false, false, &local_err);
 
     if (!blk2) {
         ret = -1;
+        error_reportf_err(local_err, "Could not open '%s': ", out.filename);
         goto out;
     }
 
@@ -4600,8 +4624,9 @@ static int img_measure(int argc, char **argv)
 
     if (filename) {
         in_blk = img_open(image_opts, filename, fmt, 0,
-                          false, false, force_share);
+                          false, false, force_share, &local_err);
         if (!in_blk) {
+            error_reportf_err(local_err, "Could not open '%s': ", filename);
             goto out;
         }
 
diff --git a/tests/qemu-iotests/043.out b/tests/qemu-iotests/043.out
index b37d2a3807..ee428a4744 100644
--- a/tests/qemu-iotests/043.out
+++ b/tests/qemu-iotests/043.out
@@ -2,19 +2,19 @@ QA output created by 043
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 
 == backing file references self ==
-qemu-img: Backing file 'TEST_DIR/t.IMGFMT' creates an infinite loop.
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Backing file 'TEST_DIR/t.IMGFMT' creates an infinite loop.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
 
 == parent references self ==
-qemu-img: Backing file 'TEST_DIR/t.IMGFMT' creates an infinite loop.
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Backing file 'TEST_DIR/t.IMGFMT' creates an infinite loop.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.1.base
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.2.base
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.3.base
 
 == ancestor references another ancestor ==
-qemu-img: Backing file 'TEST_DIR/t.IMGFMT.2.base' creates an infinite loop.
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Backing file 'TEST_DIR/t.IMGFMT.2.base' creates an infinite loop.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.1.base
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.2.base
-- 
2.14.3

[Qemu-devel] [PATCH 2/2] qemu-img: info: try -U automatically

[Fam Zheng]

The error message we had didn't have a hint about "-U" when locking the
image failed, which is not friendly. Also it is imaginable that the
reaction to that error by the user would be a retry with '-U'.

So the reason we require "-U" for "qemu-img info" if the image is used
is to raise the awareness about what could be wrong. A warning would do
just fine, especially since it is a little more informative.

The test case reference output is updated accordingly.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 qemu-img.c                 | 14 ++++++++++++++
 tests/qemu-iotests/153.out |  3 +--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 7d3171c20c..9684937425 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2605,6 +2605,20 @@ static int img_info(int argc, char **argv)
 
     list = collect_image_info_list(image_opts, filename, fmt, chain,
                                    force_share, &local_err);
+    if (!list && !force_share) {
+        Error *local_err2 = NULL;
+        list = collect_image_info_list(image_opts, filename, fmt, chain,
+                                       true, &local_err2);
+        if (list) {
+            error_report("WARNING: --force-share (-U) is not used but it "
+                         "seems the image is attached to a running guest; "
+                         "the information may be inaccurate if it is being "
+                         "updated.");
+            error_free(local_err);
+        } else {
+            error_free(local_err2);
+        }
+    }
     if (!list) {
         error_reportf_err(local_err, "Could not open '%s': ", filename);
         return 1;
diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
index 5b917b177c..4de35184ba 100644
--- a/tests/qemu-iotests/153.out
+++ b/tests/qemu-iotests/153.out
@@ -41,8 +41,7 @@ Is another process using the image?
 no file open, try 'help open'
 
 _qemu_img_wrapper info TEST_DIR/t.qcow2
-qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to get shared "write" lock
-Is another process using the image?
+qemu-img: WARNING: --force-share (-U) is not used but it seems the image is attached to a running guest; the information may be inaccurate if it is being updated.
 
 _qemu_img_wrapper check TEST_DIR/t.qcow2
 qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to get shared "write" lock
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/2] qemu-img: Move img_open error reporting to callers

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

On 01/05/2018 12:55 AM, Fam Zheng wrote:
> In the next patch one caller will have a special error handling logic
> than reporting it, add "Error **" parameters to functions and give

s/than/rather than/
s/it, add/it.  Add/

> control back to callers, to make that possible.
> 
> Update iotests output accordingly.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-img.c                 | 115 +++++++++++++++++++++++++++------------------
>  tests/qemu-iotests/043.out |   6 +--
>  2 files changed, 73 insertions(+), 48 deletions(-)
> 

> @@ -2455,23 +2464,24 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
>          ImageInfoList *elem;
>  
>          if (g_hash_table_lookup_extended(filenames, filename, NULL, NULL)) {
> -            error_report("Backing file '%s' creates an infinite loop.",
> -                         filename);
> +            error_setg(errp,
> +                       "Backing file '%s' creates an infinite loop.",

error_setg() should not end in '.'; you can fix that while touching this...

> +++ b/tests/qemu-iotests/043.out
> @@ -2,19 +2,19 @@ QA output created by 043
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>  
>  == backing file references self ==
> -qemu-img: Backing file 'TEST_DIR/t.IMGFMT' creates an infinite loop.
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Backing file 'TEST_DIR/t.IMGFMT' creates an infinite loop.
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/t.IMGFMT.base
>  
>  == parent references self ==
> -qemu-img: Backing file 'TEST_DIR/t.IMGFMT' creates an infinite loop.
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Backing file 'TEST_DIR/t.IMGFMT' creates an infinite loop.

...which is another tweak here.

Those tweaks are minor, so
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/2] qemu-img: info: try -U automatically

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2890 bytes --]

On 01/05/2018 12:55 AM, Fam Zheng wrote:
> The error message we had didn't have a hint about "-U" when locking the
> image failed, which is not friendly. Also it is imaginable that the
> reaction to that error by the user would be a retry with '-U'.
> 
> So the reason we require "-U" for "qemu-img info" if the image is used
> is to raise the awareness about what could be wrong. A warning would do
> just fine, especially since it is a little more informative.
> 
> The test case reference output is updated accordingly.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  qemu-img.c                 | 14 ++++++++++++++
>  tests/qemu-iotests/153.out |  3 +--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 7d3171c20c..9684937425 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2605,6 +2605,20 @@ static int img_info(int argc, char **argv)
>  
>      list = collect_image_info_list(image_opts, filename, fmt, chain,
>                                     force_share, &local_err);
> +    if (!list && !force_share) {
> +        Error *local_err2 = NULL;
> +        list = collect_image_info_list(image_opts, filename, fmt, chain,
> +                                       true, &local_err2);
> +        if (list) {
> +            error_report("WARNING: --force-share (-U) is not used but it "

We have warn_report() for use in this situation (in which case, you do
not want the leading "WARNING:" in your message).

> +                         "seems the image is attached to a running guest; "
> +                         "the information may be inaccurate if it is being "
> +                         "updated.");

For consistency, I'd prefer no trailing dot.

> +            error_free(local_err);
> +        } else {
> +            error_free(local_err2);
> +        }
> +    }
>      if (!list) {
>          error_reportf_err(local_err, "Could not open '%s': ", filename);
>          return 1;
> diff --git a/tests/qemu-iotests/153.out b/tests/qemu-iotests/153.out
> index 5b917b177c..4de35184ba 100644
> --- a/tests/qemu-iotests/153.out
> +++ b/tests/qemu-iotests/153.out
> @@ -41,8 +41,7 @@ Is another process using the image?
>  no file open, try 'help open'
>  
>  _qemu_img_wrapper info TEST_DIR/t.qcow2
> -qemu-img: Could not open 'TEST_DIR/t.qcow2': Failed to get shared "write" lock
> -Is another process using the image?
> +qemu-img: WARNING: --force-share (-U) is not used but it seems the image is attached to a running guest; the information may be inaccurate if it is being updated.

and those changes tweak this line.

But the idea makes sense, so with the changes,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Kevin Wolf]

Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> Management and users are accustomed to "qemu-img info" to query status of
> images even when they are used by guests. Since image locking was added, the -U
> (--force-share) option is needed for that to work. The reason has been that due
> to possible race with image header update, the output can be misleading.
> 
> But what are likely to happen after we emit the error are that, for interactive
> users, '-U' will be used and the command retried; for management (nova, RHV,
> etc.), the operation is broken with no knob to workaround this.
> 
> This series changes that error to a warning so that it doesn't get in the way.

Are management tools actually doing this? There is no good reason to
call 'qemu-img info' for an image that is in use by a VM.

If no, NACK. Automatically disabling locking because it can be
inconvenient defeats the purpose of locking.

If yes, clearly indicate that this usage is deprecated and we'll turn
this into an error again with 2.13. Then management tools can be fixed
in time.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Nir Soffer]

On Mon, Jan 8, 2018 at 4:48 PM Kevin Wolf <kwolf@redhat.com> wrote:

> Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> > Management and users are accustomed to "qemu-img info" to query status of
> > images even when they are used by guests. Since image locking was added,
> the -U
> > (--force-share) option is needed for that to work. The reason has been
> that due
> > to possible race with image header update, the output can be misleading.
> >
> > But what are likely to happen after we emit the error are that, for
> interactive
> > users, '-U' will be used and the command retried; for management (nova,
> RHV,
> > etc.), the operation is broken with no knob to workaround this.
> >
> > This series changes that error to a warning so that it doesn't get in
> the way.
>
> Are management tools actually doing this? There is no good reason to
> call 'qemu-img info' for an image that is in use by a VM.
>

Yes, ovirt/RHV is using this to get the qcow2 compat of an image since 4.1.

We asked about this here and in private mail, and there was an agreement
that
using qemu-img info on an image is safe for this purpose. We know that
accessing
image header when a guest is using is may be racy, but we control both the
guest
and the image. Nobody is modifying the image properties behind our back.

In 4.2 we will use the new flags[1], but  we cannot fix released code.
Introducing
this locking in qemu-img info will break existing installations.


> If no, NACK. Automatically disabling locking because it can be
> inconvenient defeats the purpose of locking.
>
> If yes, clearly indicate that this usage is deprecated and we'll turn
> this into an error again with 2.13. Then management tools can be fixed
> in time.


This will work for us in general, but I'm not sure that when 2.13 will be
released,
no user will run code assuming the previous behavior. It will be best to
wait with
incompatible changes like this to next major version.

[1] https://gerrit.ovirt.org/85874/

Nir

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Kevin Wolf]

Am 08.01.2018 um 18:07 hat Nir Soffer geschrieben:
> On Mon, Jan 8, 2018 at 4:48 PM Kevin Wolf <kwolf@redhat.com> wrote:
> 
> > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> > > Management and users are accustomed to "qemu-img info" to query status of
> > > images even when they are used by guests. Since image locking was added,
> > the -U
> > > (--force-share) option is needed for that to work. The reason has been
> > that due
> > > to possible race with image header update, the output can be misleading.
> > >
> > > But what are likely to happen after we emit the error are that, for
> > interactive
> > > users, '-U' will be used and the command retried; for management (nova,
> > RHV,
> > > etc.), the operation is broken with no knob to workaround this.
> > >
> > > This series changes that error to a warning so that it doesn't get in
> > the way.
> >
> > Are management tools actually doing this? There is no good reason to
> > call 'qemu-img info' for an image that is in use by a VM.
> >
> 
> Yes, ovirt/RHV is using this to get the qcow2 compat of an image since 4.1.
> 
> We asked about this here and in private mail, and there was an
> agreement that using qemu-img info on an image is safe for this
> purpose. We know that accessing image header when a guest is using is
> may be racy, but we control both the guest and the image. Nobody is
> modifying the image properties behind our back.

Yes, it's probably safe enough, though it's clearly a case for -U rather
than allowing it unconditionally.

> In 4.2 we will use the new flags[1], but  we cannot fix released code.
> Introducing this locking in qemu-img info will break existing
> installations.

We should have the proper deprecation process and printed a warning for
two releases. Anyway, it's too late for this and now we've already had
two releases where this was a hard error.

I'm not sure if going back to the old behaviour for a while now would be
helpful, you'd just end up with an even more confusing set of qemu
versions, for example:

    <= 2.9          - works without a warning
    2.10 and 2.11   - errors out
    2.12            - prints a warning, but works
    >= 2.13         - errors out again

Is it expected that you use old oVirt versions with newer qemu versions?
If so, why didn't we already notice this with the 2.10 release?
Basically I think it's reasonable to expect that new qemu versions may
require new management versions, too. We try to stay compatible, but
sometimes that just doesn't quite work out as we'd like.

> > If no, NACK. Automatically disabling locking because it can be
> > inconvenient defeats the purpose of locking.
> >
> > If yes, clearly indicate that this usage is deprecated and we'll turn
> > this into an error again with 2.13. Then management tools can be fixed
> > in time.
> 
> 
> This will work for us in general, but I'm not sure that when 2.13 will
> be released, no user will run code assuming the previous behavior. It
> will be best to wait with incompatible changes like this to next major
> version.

qemu doesn't currently use a versioning schema that works like that. Our
deprecation policy is to print a warning for two releases before the
feature is removed or changed incompatibly.

We messed this up this time, but the two releases after the change are
already over, so chances are that nobody would have noticed the problem
in time even if we hadn't messed up.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Fam Zheng]

On Mon, 01/08 18:57, Kevin Wolf wrote:
> I'm not sure if going back to the old behaviour for a while now would be
> helpful, you'd just end up with an even more confusing set of qemu
> versions, for example:
> 
>     <= 2.9          - works without a warning
>     2.10 and 2.11   - errors out
>     2.12            - prints a warning, but works
>     >= 2.13         - errors out again

What I had in mind is settle on warning for good. QEMU (including qemu-img) is a
low level tool that can be used in many ways that it isn't supposed to, this one
is not more harmful than others (e.g. "qemu-img snapshot ..." on iscsi:// qcow2
image) we allow siliently.

I know this is debatable but I think the #1 purpose of image locking is to
prevent data corruption; #2 IMO is to reduce confusion and misinformation.
While inconsistent output of "qemu-img info" is misinformation, it not working
as before is actually confusion. Though the current behavior is indeed ideal,
the proposed patch is a bit more pragmatical.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Kevin Wolf]

Am 09.01.2018 um 07:24 hat Fam Zheng geschrieben:
> On Mon, 01/08 18:57, Kevin Wolf wrote:
> > I'm not sure if going back to the old behaviour for a while now would be
> > helpful, you'd just end up with an even more confusing set of qemu
> > versions, for example:
> > 
> >     <= 2.9          - works without a warning
> >     2.10 and 2.11   - errors out
> >     2.12            - prints a warning, but works
> >     >= 2.13         - errors out again
> 
> What I had in mind is settle on warning for good. QEMU (including qemu-img) is a
> low level tool that can be used in many ways that it isn't supposed to, this one
> is not more harmful than others (e.g. "qemu-img snapshot ..." on iscsi:// qcow2
> image) we allow siliently.
> 
> I know this is debatable but I think the #1 purpose of image locking is to
> prevent data corruption;

Isn't potentially wrong output of 'qemu-img info' a form of data
corruption? Not data as in disk content, but metadata is data, too.

> #2 IMO is to reduce confusion and misinformation.
> While inconsistent output of "qemu-img info" is misinformation, it not working
> as before is actually confusion. Though the current behavior is indeed ideal,
> the proposed patch is a bit more pragmatical.

To be clear: If we want to minimise confusing by change, we have to
disable locking by default, not only here but everywhere else. And
therefore make it completely useless.

The whole point of locking is to change something in order to make
things safer. Yes, this may inconvenience users who want to do something
unsafe. Tough luck. The only other option is not making things safer.

We already successfully made a point that tools (libvirt with shared
images, or libguestfs) need to update their qemu invocation for qemu
proper. They didn't like it, but they could see the point, and it has
been this way for two releases already. So I don't see a compelling
reason why now we should give up again some of the safety we achieved
long-term.

If we were designing qemu-img from scratch, it would be an error. So
that's what it should be in the long term. Tools should preferably use
'query-block' and friends rather 'qemu-img info' if the image is in use.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Ala Hino]

On Tue, Jan 9, 2018 at 11:58 AM, Kevin Wolf <kwolf@redhat.com> wrote:

> Am 09.01.2018 um 07:24 hat Fam Zheng geschrieben:
> > On Mon, 01/08 18:57, Kevin Wolf wrote:
> > > I'm not sure if going back to the old behaviour for a while now would
> be
> > > helpful, you'd just end up with an even more confusing set of qemu
> > > versions, for example:
> > >
> > >     <= 2.9          - works without a warning
> > >     2.10 and 2.11   - errors out
> > >     2.12            - prints a warning, but works
> > >     >= 2.13         - errors out again
> >
> > What I had in mind is settle on warning for good. QEMU (including
> qemu-img) is a
> > low level tool that can be used in many ways that it isn't supposed to,
> this one
> > is not more harmful than others (e.g. "qemu-img snapshot ..." on
> iscsi:// qcow2
> > image) we allow siliently.
> >
> > I know this is debatable but I think the #1 purpose of image locking is
> to
> > prevent data corruption;
>
> Isn't potentially wrong output of 'qemu-img info' a form of data
> corruption? Not data as in disk content, but metadata is data, too.
>
> > #2 IMO is to reduce confusion and misinformation.
> > While inconsistent output of "qemu-img info" is misinformation, it not
> working
> > as before is actually confusion. Though the current behavior is indeed
> ideal,
> > the proposed patch is a bit more pragmatical.
>
> To be clear: If we want to minimise confusing by change, we have to
> disable locking by default, not only here but everywhere else. And
> therefore make it completely useless.
>
> The whole point of locking is to change something in order to make
> things safer. Yes, this may inconvenience users who want to do something
> unsafe. Tough luck. The only other option is not making things safer.
>

Safer is better for sure.
It is not about doing something unsafe, it is about breaking a released
version -
RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will not
be able to create snapshots.
I see two options:
1. As mentioned by Fam, settle on warning for good
2. Conflict qemu with vdsm < 4.2

>
> We already successfully made a point that tools (libvirt with shared
> images, or libguestfs) need to update their qemu invocation for qemu
> proper. They didn't like it, but they could see the point, and it has
> been this way for two releases already. So I don't see a compelling
> reason why now we should give up again some of the safety we achieved
> long-term.
>
> If we were designing qemu-img from scratch, it would be an error. So
> that's what it should be in the long term. Tools should preferably use
> 'query-block' and friends rather 'qemu-img info' if the image is in use.
>
> Kevin
>

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

On 01/09/2018 01:58 PM, Ala Hino wrote:

>>> I know this is debatable but I think the #1 purpose of image locking is
>> to
>>> prevent data corruption;
>>
>> Isn't potentially wrong output of 'qemu-img info' a form of data
>> corruption? Not data as in disk content, but metadata is data, too.
>>
>>> #2 IMO is to reduce confusion and misinformation.
>>> While inconsistent output of "qemu-img info" is misinformation, it not
>> working
>>> as before is actually confusion. Though the current behavior is indeed
>> ideal,
>>> the proposed patch is a bit more pragmatical.
>>
>> To be clear: If we want to minimise confusing by change, we have to
>> disable locking by default, not only here but everywhere else. And
>> therefore make it completely useless.
>>
>> The whole point of locking is to change something in order to make
>> things safer. Yes, this may inconvenience users who want to do something
>> unsafe. Tough luck. The only other option is not making things safer.
>>
> 
> Safer is better for sure.
> It is not about doing something unsafe, it is about breaking a released
> version -
> RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will not
> be able to create snapshots.
> I see two options:
> 1. As mentioned by Fam, settle on warning for good

Which is a downgrade in qemu behavior, since we've already had releases
where it was an error (and if you have to worry about THOSE qemu
releases, then the warning doesn't buy you anything).

> 2. Conflict qemu with vdsm < 4.2

If I'm understanding this option correctly, you are proposing that the
distribution is set up so that you have to upgrade vdsm prior to being
able to upgrade to newer qemu that enables locking (so you can use old
vdsm, old qemu; new vdsm, old qemu; new vdsm, new qemu; but you get
rejected on attempts to use old vdsm, new qemu).  That's outside the
scope of qemu proper and falls instead into the distro's packaging
scheme, but sounds like a better alternative than trying to fix new qemu
to work around an issue that released qemu already has, all on behalf of
vdsm where old vdsm plays nice with old qemu, and where new vdsm already
knows how to play nice with both flavors of qemu.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Ala Hino]

On Tue, Jan 9, 2018 at 10:11 PM, Eric Blake <eblake@redhat.com> wrote:

> On 01/09/2018 01:58 PM, Ala Hino wrote:
>
> >>> I know this is debatable but I think the #1 purpose of image locking is
> >> to
> >>> prevent data corruption;
> >>
> >> Isn't potentially wrong output of 'qemu-img info' a form of data
> >> corruption? Not data as in disk content, but metadata is data, too.
> >>
> >>> #2 IMO is to reduce confusion and misinformation.
> >>> While inconsistent output of "qemu-img info" is misinformation, it not
> >> working
> >>> as before is actually confusion. Though the current behavior is indeed
> >> ideal,
> >>> the proposed patch is a bit more pragmatical.
> >>
> >> To be clear: If we want to minimise confusing by change, we have to
> >> disable locking by default, not only here but everywhere else. And
> >> therefore make it completely useless.
> >>
> >> The whole point of locking is to change something in order to make
> >> things safer. Yes, this may inconvenience users who want to do something
> >> unsafe. Tough luck. The only other option is not making things safer.
> >>
> >
> > Safer is better for sure.
> > It is not about doing something unsafe, it is about breaking a released
> > version -
> > RHV 4.1 is already out and when customers upgrade to RHEL 7.5, they will
> not
> > be able to create snapshots.
> > I see two options:
> > 1. As mentioned by Fam, settle on warning for good
>
> Which is a downgrade in qemu behavior, since we've already had releases
> where it was an error (and if you have to worry about THOSE qemu
> releases, then the warning doesn't buy you anything).
>

We test our code on RHEL, and currently require qemu-kvm-rhev >= 2.6.0.
On RHEL 7.4 qemu-kvm-rhev version is 2.9.0 - and everything works good.
However, on RHEL 7.5 the version is 2.10.0, which breaks RHV 4.1.
If we got qemu-kvm-rhev 2.10.0 in RHEL 7.4 repositories, we could detect the
failure earlier and maybe we would be able to adapt the code before
releasing 4.1.

>
> > 2. Conflict qemu with vdsm < 4.2
>
> If I'm understanding this option correctly, you are proposing that the
> distribution is set up so that you have to upgrade vdsm prior to being
> able to upgrade to newer qemu that enables locking (so you can use old
> vdsm, old qemu; new vdsm, old qemu; new vdsm, new qemu; but you get
> rejected on attempts to use old vdsm, new qemu).  That's outside the
> scope of qemu proper and falls instead into the distro's packaging
> scheme, but sounds like a better alternative than trying to fix new qemu
> to work around an issue that released qemu already has, all on behalf of
> vdsm where old vdsm plays nice with old qemu, and where new vdsm already
> knows how to play nice with both flavors of qemu.
>

I agree that this might be the better option.

>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>

Re: [Qemu-devel] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Daniel P. Berrange]

On Mon, Jan 08, 2018 at 03:41:36PM +0100, Kevin Wolf wrote:
> Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> > Management and users are accustomed to "qemu-img info" to query status of
> > images even when they are used by guests. Since image locking was added, the -U
> > (--force-share) option is needed for that to work. The reason has been that due
> > to possible race with image header update, the output can be misleading.
> > 
> > But what are likely to happen after we emit the error are that, for interactive
> > users, '-U' will be used and the command retried; for management (nova, RHV,
> > etc.), the operation is broken with no knob to workaround this.
> > 
> > This series changes that error to a warning so that it doesn't get in the way.
> 
> Are management tools actually doing this? There is no good reason to
> call 'qemu-img info' for an image that is in use by a VM.

OpenStack will frequently call 'qemu-img info' for disks that are in use by
VMs. It is looking at the sizes to understand the relation between the current
size used by qcow2 vs the possible future usage. In this context, it does not
matter if the data is slightly outdated, as it will catch up next time it reads
it a few mins later.

It has been patched to just retry with -U to avoid this error on new
QEMU.

> If no, NACK. Automatically disabling locking because it can be
> inconvenient defeats the purpose of locking.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Daniel P. Berrange]

On Mon, Jan 08, 2018 at 06:57:29PM +0100, Kevin Wolf wrote:
> Am 08.01.2018 um 18:07 hat Nir Soffer geschrieben:
> > On Mon, Jan 8, 2018 at 4:48 PM Kevin Wolf <kwolf@redhat.com> wrote:
> > 
> > > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> > > > Management and users are accustomed to "qemu-img info" to query status of
> > > > images even when they are used by guests. Since image locking was added,
> > > the -U
> > > > (--force-share) option is needed for that to work. The reason has been
> > > that due
> > > > to possible race with image header update, the output can be misleading.
> > > >
> > > > But what are likely to happen after we emit the error are that, for
> > > interactive
> > > > users, '-U' will be used and the command retried; for management (nova,
> > > RHV,
> > > > etc.), the operation is broken with no knob to workaround this.
> > > >
> > > > This series changes that error to a warning so that it doesn't get in
> > > the way.
> > >
> > > Are management tools actually doing this? There is no good reason to
> > > call 'qemu-img info' for an image that is in use by a VM.
> > >
> > 
> > Yes, ovirt/RHV is using this to get the qcow2 compat of an image since 4.1.
> > 
> > We asked about this here and in private mail, and there was an
> > agreement that using qemu-img info on an image is safe for this
> > purpose. We know that accessing image header when a guest is using is
> > may be racy, but we control both the guest and the image. Nobody is
> > modifying the image properties behind our back.
> 
> Yes, it's probably safe enough, though it's clearly a case for -U rather
> than allowing it unconditionally.
> 
> > In 4.2 we will use the new flags[1], but  we cannot fix released code.
> > Introducing this locking in qemu-img info will break existing
> > installations.
> 
> We should have the proper deprecation process and printed a warning for
> two releases. Anyway, it's too late for this and now we've already had
> two releases where this was a hard error.
> 
> I'm not sure if going back to the old behaviour for a while now would be
> helpful, you'd just end up with an even more confusing set of qemu
> versions, for example:
> 
>     <= 2.9          - works without a warning
>     2.10 and 2.11   - errors out
>     2.12            - prints a warning, but works
>     >= 2.13         - errors out again

I think this is really undesirable to have 2.12 suddenly behave diferently
after 2 releases of having this change. Apps are pretty much forced to
take the change to use -U regardless because 2.10 & 2.11 are already in
the wild. Printing a warning would only be useful if we had done it in
2.10.  Now it is too late to be useful and will just confuse life even
more.

So IMHO just drop this patch.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Kashyap Chamarthy]

On Mon, Jan 08, 2018 at 03:41:36PM +0100, Kevin Wolf wrote:
> Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> > Management and users are accustomed to "qemu-img info" to query status of
> > images even when they are used by guests. Since image locking was added, the -U
> > (--force-share) option is needed for that to work. The reason has been that due
> > to possible race with image header update, the output can be misleading.
> > 
> > But what are likely to happen after we emit the error are that, for interactive
> > users, '-U' will be used and the command retried; for management (nova, RHV,
> > etc.), the operation is broken with no knob to workaround this.
> > 
> > This series changes that error to a warning so that it doesn't get in the way.
> 
> Are management tools actually doing this? There is no good reason to
> call 'qemu-img info' for an image that is in use by a VM.

Yes, Nova does use 'qemu-img info' in a few places (haven't audited them
all) for a running guest.  From a quick look at the code, at-least
during Nova's live snaphot (as part of libvirt's "shallow rebase") it is
used.

> If no, NACK. Automatically disabling locking because it can be
> inconvenient defeats the purpose of locking.
> 
> If yes, clearly indicate that this usage is deprecated and we'll turn
> this into an error again with 2.13. Then management tools can be fixed
> in time.

Yes, for completness' sake, Nova upstream is already patched to use the
`qemu-img` '--force-share' flag that comes with QEMU >= 2.10.

-- 
/kashyap

Re: [Qemu-devel] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Kevin Wolf]

Am 10.01.2018 um 13:49 hat Daniel P. Berrange geschrieben:
> On Mon, Jan 08, 2018 at 03:41:36PM +0100, Kevin Wolf wrote:
> > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> > > Management and users are accustomed to "qemu-img info" to query status of
> > > images even when they are used by guests. Since image locking was added, the -U
> > > (--force-share) option is needed for that to work. The reason has been that due
> > > to possible race with image header update, the output can be misleading.
> > > 
> > > But what are likely to happen after we emit the error are that, for interactive
> > > users, '-U' will be used and the command retried; for management (nova, RHV,
> > > etc.), the operation is broken with no knob to workaround this.
> > > 
> > > This series changes that error to a warning so that it doesn't get in the way.
> > 
> > Are management tools actually doing this? There is no good reason to
> > call 'qemu-img info' for an image that is in use by a VM.
> 
> OpenStack will frequently call 'qemu-img info' for disks that are in use by
> VMs. It is looking at the sizes to understand the relation between the current
> size used by qcow2 vs the possible future usage. In this context, it does not
> matter if the data is slightly outdated, as it will catch up next time it reads
> it a few mins later.
> 
> It has been patched to just retry with -U to avoid this error on new
> QEMU.

The proper, though somewhat more intrusive fix would be to use QMP
commands for images of running VMs. You already need to do the same for
anything modifying the image (to avoid corruption), so I think doing the
same with 'query-block' instead of 'qemu-img info' for guaranteed
consistent results on running VMs only makes sense.

Kevin

Re: [Qemu-devel] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Daniel P. Berrange]

On Wed, Jan 10, 2018 at 03:07:12PM +0100, Kevin Wolf wrote:
> Am 10.01.2018 um 13:49 hat Daniel P. Berrange geschrieben:
> > On Mon, Jan 08, 2018 at 03:41:36PM +0100, Kevin Wolf wrote:
> > > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> > > > Management and users are accustomed to "qemu-img info" to query status of
> > > > images even when they are used by guests. Since image locking was added, the -U
> > > > (--force-share) option is needed for that to work. The reason has been that due
> > > > to possible race with image header update, the output can be misleading.
> > > > 
> > > > But what are likely to happen after we emit the error are that, for interactive
> > > > users, '-U' will be used and the command retried; for management (nova, RHV,
> > > > etc.), the operation is broken with no knob to workaround this.
> > > > 
> > > > This series changes that error to a warning so that it doesn't get in the way.
> > > 
> > > Are management tools actually doing this? There is no good reason to
> > > call 'qemu-img info' for an image that is in use by a VM.
> > 
> > OpenStack will frequently call 'qemu-img info' for disks that are in use by
> > VMs. It is looking at the sizes to understand the relation between the current
> > size used by qcow2 vs the possible future usage. In this context, it does not
> > matter if the data is slightly outdated, as it will catch up next time it reads
> > it a few mins later.
> > 
> > It has been patched to just retry with -U to avoid this error on new
> > QEMU.
> 
> The proper, though somewhat more intrusive fix would be to use QMP
> commands for images of running VMs. You already need to do the same for
> anything modifying the image (to avoid corruption), so I think doing the
> same with 'query-block' instead of 'qemu-img info' for guaranteed
> consistent results on running VMs only makes sense.

The problem with 'query-block' is that you assume the code that is
processing this set of disk images knows which image is used where.

This code in question merely sees a directory full of images, and does
not directly know whether any of them are in use or not. So trying to
use query-block would make it significantly more complex for little
obvious benefit to OpenStack.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Nir Soffer]

On Wed, Jan 10, 2018 at 4:04 PM Kashyap Chamarthy <kchamart@redhat.com>
wrote:

> On Mon, Jan 08, 2018 at 03:41:36PM +0100, Kevin Wolf wrote:
> > Am 05.01.2018 um 07:55 hat Fam Zheng geschrieben:
> > > Management and users are accustomed to "qemu-img info" to query status
> of
> > > images even when they are used by guests. Since image locking was
> added, the -U
> > > (--force-share) option is needed for that to work. The reason has been
> that due
> > > to possible race with image header update, the output can be
> misleading.
> > >
> > > But what are likely to happen after we emit the error are that, for
> interactive
> > > users, '-U' will be used and the command retried; for management
> (nova, RHV,
> > > etc.), the operation is broken with no knob to workaround this.
> > >
> > > This series changes that error to a warning so that it doesn't get in
> the way.
> >
> > Are management tools actually doing this? There is no good reason to
> > call 'qemu-img info' for an image that is in use by a VM.
>
> Yes, Nova does use 'qemu-img info' in a few places (haven't audited them
> all) for a running guest.  From a quick look at the code, at-least
> during Nova's live snaphot (as part of libvirt's "shallow rebase") it is
> used.
>
> > If no, NACK. Automatically disabling locking because it can be
> > inconvenient defeats the purpose of locking.
> >
> > If yes, clearly indicate that this usage is deprecated and we'll turn
> > this into an error again with 2.13. Then management tools can be fixed
> > in time.
>
> Yes, for completness' sake, Nova upstream is already patched to use the
> `qemu-img` '--force-share' flag that comes with QEMU >= 2.10.
>

What abut users running released Nova, upgrading to 2.10?

Nir

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] qemu-img: Let "info" warn and go ahead without -U

[Kashyap Chamarthy]

On Wed, Jan 10, 2018 at 04:43:22PM +0000, Nir Soffer wrote:
> On Wed, Jan 10, 2018 at 4:04 PM Kashyap Chamarthy <kchamart@redhat.com>
> wrote:

[...]

> > Yes, for completness' sake, Nova upstream is already patched to use the
> > `qemu-img` '--force-share' flag that comes with QEMU >= 2.10.
> >
> 
> What abut users running released Nova, upgrading to 2.10?

There are three stable Nova branches at any time, in various phases.  

It is already backported to the current newest stable Nova branch (where
all bug fixes that meet certain criteria are accepted).

Then there are two other stable releases.  One is in "Only critical
bugfixes and security patches" phase; the other is in "Only security
patches" phase.  So this change wasn't backported there, and users would
be using QEMU 2.9 or below with those two versions of Nova.

-- 
/kashyap