[Qemu-devel] [PATCH v3 0/5] block: allow partial info-block query

[Qemu-devel] [PATCH v3 0/5] block: allow partial info-block query

[John Snow]

Allow info-block to succeed even when it cannot reconstruct filenames,
but be explicit about this failure. Utilities that rely on the current
behavior are modified to report this explicit failure to users.

v3:
 - Fix qemu-img output such that "backing file IS the absolute backing path"
   and "absolute backing path was indeterminate" are distinguishable via
   a new error message present in block/qapi.c; see patch 3.
 - Output for test 110 modified once more to expect the above error.

v2:
 - Fix qemu-img from now choking when it gets a partial response.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch block-allow-partial-query
https://github.com/jnsnow/qemu/tree/block-allow-partial-query

This version is tagged block-allow-partial-query-v3:
https://github.com/jnsnow/qemu/releases/tag/block-allow-partial-query-v3

John Snow (5):
  block/qapi: do not redundantly print "actual path"
  block/qapi: always report full_backing_filename
  block/qapi: explicitly warn if !has_full_backing_filename
  qemu-img: abort when full_backing_filename not present
  block/qapi: allow best-effort query

 block/qapi.c               | 19 ++++++++++++-------
 qemu-img.c                 |  5 ++++-
 tests/qemu-iotests/043.out |  2 ++
 tests/qemu-iotests/110.out |  5 ++++-
 4 files changed, 22 insertions(+), 9 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v3 1/5] block/qapi: do not redundantly print "actual path"

[John Snow]

If it happens to match the backing path, that was the actual path.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qapi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/qapi.c b/block/qapi.c
index 267f147..01569da 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -676,7 +676,9 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
 
     if (info->has_backing_filename) {
         func_fprintf(f, "backing file: %s", info->backing_filename);
-        if (info->has_full_backing_filename) {
+        if (info->has_full_backing_filename &&
+            (strcmp(info->backing_filename,
+                    info->full_backing_filename) != 0)) {
             func_fprintf(f, " (actual path: %s)", info->full_backing_filename);
         }
         func_fprintf(f, "\n");
-- 
2.4.3

[Qemu-devel] [PATCH v3 2/5] block/qapi: always report full_backing_filename

[John Snow]

Always report full_backing_filename, even if it's the same as
backing_filename. In the next patch, full_backing_filename may be
omitted if it cannot be generated instead of allowing e.g. drive_query
to abort if it runs into this scenario.

The presence or absence of the "full" field becomes useful information.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qapi.c               | 7 ++++---
 tests/qemu-iotests/043.out | 2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 01569da..0e6b333 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -251,9 +251,10 @@ void bdrv_query_image_info(BlockDriverState *bs,
             return;
         }
 
-        if (strcmp(backing_filename, backing_filename2) != 0) {
-            info->full_backing_filename =
-                        g_strdup(backing_filename2);
+        /* Always report the full_backing_filename if present, even if it's the
+         * same as backing_filename. That they are same is useful info. */
+        if (backing_filename2) {
+            info->full_backing_filename = g_strdup(backing_filename2);
             info->has_full_backing_filename = true;
         }
 
diff --git a/tests/qemu-iotests/043.out b/tests/qemu-iotests/043.out
index 33f8cc3..b37d2a3 100644
--- a/tests/qemu-iotests/043.out
+++ b/tests/qemu-iotests/043.out
@@ -44,6 +44,7 @@ cluster_size: 65536
         "filename": "TEST_DIR/t.IMGFMT",
         "cluster-size": 65536,
         "format": "IMGFMT",
+        "full-backing-filename": "TEST_DIR/t.IMGFMT.2.base",
         "backing-filename": "TEST_DIR/t.IMGFMT.2.base",
         "dirty-flag": false
     },
@@ -52,6 +53,7 @@ cluster_size: 65536
         "filename": "TEST_DIR/t.IMGFMT.2.base",
         "cluster-size": 65536,
         "format": "IMGFMT",
+        "full-backing-filename": "TEST_DIR/t.IMGFMT.1.base",
         "backing-filename": "TEST_DIR/t.IMGFMT.1.base",
         "dirty-flag": false
     },
-- 
2.4.3

[Qemu-devel] [PATCH v3 3/5] block/qapi: explicitly warn if !has_full_backing_filename

[John Snow]

Disambiguate "Backing filename and full backing filename" are equivalent
from "full backing filename could not be determined."

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qapi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 0e6b333..c61fb30 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -677,9 +677,10 @@ void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
 
     if (info->has_backing_filename) {
         func_fprintf(f, "backing file: %s", info->backing_filename);
-        if (info->has_full_backing_filename &&
-            (strcmp(info->backing_filename,
-                    info->full_backing_filename) != 0)) {
+        if (!info->has_full_backing_filename) {
+            func_fprintf(f, " (cannot determine actual path)");
+        } else if (strcmp(info->backing_filename,
+                          info->full_backing_filename) != 0) {
             func_fprintf(f, " (actual path: %s)", info->full_backing_filename);
         }
         func_fprintf(f, "\n");
-- 
2.4.3

[Qemu-devel] [PATCH v3 4/5] qemu-img: abort when full_backing_filename not present

[John Snow]

...But only if we have the backing_filename. It means something Scary
happened and we can't really be quite exactly sure if we can trust the
backing_filename.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 033011c..272bb0b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2040,7 +2040,10 @@ static ImageInfoList *collect_image_info_list(const char *filename,
             if (info->has_full_backing_filename) {
                 filename = info->full_backing_filename;
             } else if (info->has_backing_filename) {
-                filename = info->backing_filename;
+                error_report("Could not determine absolute backing filename,"
+                             " but backing filename '%s' present",
+                             info->backing_filename);
+                goto err;
             }
             if (info->has_backing_filename_format) {
                 fmt = info->backing_filename_format;
-- 
2.4.3

[Qemu-devel] [PATCH v3 5/5] block/qapi: allow best-effort query

[John Snow]

For more complex BDS trees that can be created under normal circumstances,
we lose the ability to issue query commands because of our inability to
re-construct the absolute filename.

Instead, omit this field when it is a problem and present as much information
as we can.

This will change the expected output in iotest 110, where we will now see a
json filename and the lack of an absolute filename instead of an error.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/qapi.c               | 7 ++++---
 tests/qemu-iotests/110.out | 5 ++++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index c61fb30..9c6c02d 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -245,10 +245,11 @@ void bdrv_query_image_info(BlockDriverState *bs,
         info->has_backing_filename = true;
         bdrv_get_full_backing_filename(bs, backing_filename2, PATH_MAX, &err);
         if (err) {
-            error_propagate(errp, err);
-            qapi_free_ImageInfo(info);
+            /* Can't reconstruct the full backing filename, so we must omit
+             * this field and apply a Best Effort to this query. */
             g_free(backing_filename2);
-            return;
+            backing_filename2 = NULL;
+            error_free(err);
         }
 
         /* Always report the full_backing_filename if present, even if it's the
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index 0270980..b3584ff 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -11,7 +11,10 @@ backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
 
 === Non-reconstructable filename ===
 
-qemu-img: Cannot use relative backing file names for 'json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "set-state.0.new_state": 42}}'
+image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", "image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": "blkdebug", "set-state.0.new_state": 42}}
+file format: IMGFMT
+virtual size: 64M (67108864 bytes)
+backing file: t.IMGFMT.base (cannot determine actual path)
 
 === Backing name is always relative to the backed image ===
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH v3 3/5] block/qapi: explicitly warn if !has_full_backing_filename

[Max Reitz]

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

On 14.12.2015 20:55, John Snow wrote:
> Disambiguate "Backing filename and full backing filename" are equivalent
> from "full backing filename could not be determined."
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/qapi.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v3 5/5] block/qapi: allow best-effort query

[Max Reitz]

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

On 14.12.2015 20:55, John Snow wrote:
> For more complex BDS trees that can be created under normal circumstances,
> we lose the ability to issue query commands because of our inability to
> re-construct the absolute filename.
> 
> Instead, omit this field when it is a problem and present as much information
> as we can.
> 
> This will change the expected output in iotest 110, where we will now see a
> json filename and the lack of an absolute filename instead of an error.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/qapi.c               | 7 ++++---
>  tests/qemu-iotests/110.out | 5 ++++-
>  2 files changed, 8 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v3 3/5] block/qapi: explicitly warn if !has_full_backing_filename

[Max Reitz]

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

On 14.12.2015 20:55, John Snow wrote:
> Disambiguate "Backing filename and full backing filename" are equivalent

Just noticed: R-b is assuming s/" are equivalent/ are equivalent"/.

Max

> from "full backing filename could not be determined."
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/qapi.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)


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

Re: [Qemu-devel] [PATCH v3 0/5] block: allow partial info-block query

[Max Reitz]

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

On 14.12.2015 20:55, John Snow wrote:
> Allow info-block to succeed even when it cannot reconstruct filenames,
> but be explicit about this failure. Utilities that rely on the current
> behavior are modified to report this explicit failure to users.
> 
> v3:
>  - Fix qemu-img output such that "backing file IS the absolute backing path"
>    and "absolute backing path was indeterminate" are distinguishable via
>    a new error message present in block/qapi.c; see patch 3.
>  - Output for test 110 modified once more to expect the above error.
> 
> v2:
>  - Fix qemu-img from now choking when it gets a partial response.
> 
> ________________________________________________________________________________
> 
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch block-allow-partial-query
> https://github.com/jnsnow/qemu/tree/block-allow-partial-query
> 
> This version is tagged block-allow-partial-query-v3:
> https://github.com/jnsnow/qemu/releases/tag/block-allow-partial-query-v3
> 
> John Snow (5):
>   block/qapi: do not redundantly print "actual path"
>   block/qapi: always report full_backing_filename
>   block/qapi: explicitly warn if !has_full_backing_filename
>   qemu-img: abort when full_backing_filename not present
>   block/qapi: allow best-effort query
> 
>  block/qapi.c               | 19 ++++++++++++-------
>  qemu-img.c                 |  5 ++++-
>  tests/qemu-iotests/043.out |  2 ++
>  tests/qemu-iotests/110.out |  5 ++++-
>  4 files changed, 22 insertions(+), 9 deletions(-)

Due to Markus not being CC'd and the get_maintainers script giving both
him and Kevin, I just went ahead and...

...fixed patch 3's commit message and applied the series to my
block-next branch, thanks:

https://github.com/XanClic/qemu/commits/block-next

Max


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