[Qemu-devel] [PATCH v6 0/9] block: Drop BDS.filename

[Qemu-devel] [PATCH v6 0/9] block: Drop BDS.filename

[Max Reitz]

*** This series is based on v4 of my ***
*** "block: Fix some filename generation issues" series ***


The BDS filename field is generally only used when opening disk images
or emitting error or warning messages, the only exception to this rule
is the map command of qemu-img. However, using exact_filename there
instead should not be a problem. Therefore, we can drop the filename
field from the BlockDriverState and use a function instead which builds
the filename from scratch when called.

This is slower than reading a static char array but the problem of that
static array is that it may become obsolete due to changes in any
BlockDriverState or in the BDS graph. Using a function which rebuilds
the filename every time it is called resolves this problem.

The disadvantage of worse performance is negligible, on the other hand.
After patch 3 of this series, which replaces some queries of
BDS.filename by reads from somewhere else (mostly BDS.exact_filename),
the filename field is only used when a disk image is opened or some
message should be emitted, both of which cases do not suffer from the
performance hit.


http://lists.nongnu.org/archive/html/qemu-devel/2015-06/msg07025.html
gives an example of how to achieve an outdated filename, so we do need
this series. We can technically fix it differently (by appropriately
invoking bdrv_refresh_filename()), but that is pretty difficult™. I find
this series simpler and it it's something we wanted to do anyway.

Regarding the fear that this might change current behavior, especially
for libvirt which used filenames to identify nodes in the BDS graph:
Since bdrv_open() already calls bdrv_refresh_filename() today, and
apparently everything works fine, this series will most likely not break
anything. The only thing that will change is if the BDS graph is
dynamically reconfigured at runtime, and in that case it's most likely a
bug fix. Also, I don't think libvirt supports such cases already.


tl;dr: This series is a bug fix and I don't think it'll break legacy
management applications relying on the filename to identify a BDS; as
long as they are not trying to continue that practice with more advanced
configurations (e.g. with Quorum).


v6:
- Rebased after more than a year
- Patch 1 is newly required for patch 6
- Patch 3: Rebase conflicts
- Patch 4: Related bug fix; fits well into this series because it
           requires that format drivers do not query their
           bs->exact_filename, and this series can ensure exactly that
           (and after patch 3, we can be pretty sure this is the case)
- Patch 5: Rebase conflicts, improved a comment, noticed that I'm now
           replacing all of the existing bdrv_refresh_filename() calls
           and changed the commit message accordingly
- Patch 6: Rewritten because the affected code has been rewritten in the
           meantime.
- Patch 7: Rebase conflicts
- Patch 8: Added in this version. It makes the series a bit more
           rigorous, and I think that's good.
- Patch 9: Rebase conflict


git-backport-diff against v5:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/9:[down] 'block: Always set *file in get_block_status'
002/9:[----] [-C] 'block: Change bdrv_get_encrypted_filename()'
003/9:[0011] [FC] 'block: Avoid BlockDriverState.filename'
004/9:[down] 'block: Do not blindly copy filename from file'
005/9:[0020] [FC] 'block: Add bdrv_filename()'
006/9:[0041] [FC] 'qemu-img: Use bdrv_filename() for map'
007/9:[0072] [FC] 'block: Drop BlockDriverState.filename'
008/9:[down] 'block: Complete move to pull filename updates'
009/9:[0003] [FC] 'iotests: Test changed Quorum filename'


Max Reitz (9):
  block: Always set *file in get_block_status
  block: Change bdrv_get_encrypted_filename()
  block: Avoid BlockDriverState.filename
  block: Do not blindly copy filename from file
  block: Add bdrv_filename()
  qemu-img: Use bdrv_filename() for map
  block: Drop BlockDriverState.filename
  block: Complete move to pull filename updates
  iotests: Test changed Quorum filename

 include/block/block.h     |  3 +-
 include/block/block_int.h | 13 ++++++-
 block.c                   | 96 ++++++++++++++++++++++++++++++++++-------------
 block/commit.c            |  4 +-
 block/file-posix.c        |  6 +--
 block/file-win32.c        |  4 +-
 block/gluster.c           |  3 +-
 block/io.c                |  6 ++-
 block/mirror.c            | 16 ++++++--
 block/nbd.c               |  5 ++-
 block/nfs.c               |  4 +-
 block/qapi.c              |  4 +-
 block/raw-format.c        |  4 +-
 block/replication.c       |  2 -
 block/vhdx-log.c          |  7 +++-
 block/vmdk.c              | 24 +++++++++---
 block/vpc.c               |  7 +++-
 blockdev.c                | 33 +++++++++++-----
 monitor.c                 |  5 ++-
 qemu-img.c                | 31 ++++++++++++---
 tests/qemu-iotests/041    | 16 ++++++--
 21 files changed, 215 insertions(+), 78 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v6 1/9] block: Always set *file in get_block_status

[Max Reitz]

*file should always be set (to NULL, if nothing else) instead of leaving
it dangling sometimes. This should also be documented so callers can
rely on this behavior.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/io.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 4f005623f7..ff693d7f73 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1709,7 +1709,8 @@ typedef struct BdrvCoGetBlockStatusData {
  * beyond the end of the disk image it will be clamped.
  *
  * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file'
- * points to the BDS which the sector range is allocated in.
+ * points to the BDS which the sector range is allocated in. If the block driver
+ * does not set 'file', it will be set to NULL.
  */
 static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
                                                      int64_t sector_num,
@@ -1720,6 +1721,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
     int64_t n;
     int64_t ret, ret2;
 
+    *file = NULL;
+
     total_sectors = bdrv_nb_sectors(bs);
     if (total_sectors < 0) {
         return total_sectors;
@@ -1744,7 +1747,6 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
         return ret;
     }
 
-    *file = NULL;
     bdrv_inc_in_flight(bs);
     ret = bs->drv->bdrv_co_get_block_status(bs, sector_num, nb_sectors, pnum,
                                             file);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v6 1/9] block: Always set *file in get_block_status

[Eric Blake]

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

On 01/13/2017 02:52 PM, Max Reitz wrote:
> *file should always be set (to NULL, if nothing else) instead of leaving
> it dangling sometimes. This should also be documented so callers can
> rely on this behavior.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH v6 2/9] block: Change bdrv_get_encrypted_filename()

[Max Reitz]

Instead of returning a pointer to the filename, g_strdup() it. This will
become necessary once we do not have BlockDriverState.filename anymore.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  2 +-
 block.c               | 17 ++++++++++-------
 monitor.c             |  5 ++++-
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 7bcbd05338..3425e9fa79 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -432,7 +432,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
                             int64_t *cluster_offset,
                             unsigned int *cluster_bytes);
 
-const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
+char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
 char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp);
diff --git a/block.c b/block.c
index c63fc1b2da..95015251d5 100644
--- a/block.c
+++ b/block.c
@@ -2858,10 +2858,12 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
         }
     } else {
         if (bdrv_key_required(bs)) {
+            char *enc_filename = bdrv_get_encrypted_filename(bs);
             error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
                       "'%s' (%s) is encrypted",
                       bdrv_get_device_or_node_name(bs),
-                      bdrv_get_encrypted_filename(bs));
+                      enc_filename ?: "");
+            g_free(enc_filename);
         }
     }
 }
@@ -3111,14 +3113,15 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs)
     return false;
 }
 
-const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
+char *bdrv_get_encrypted_filename(BlockDriverState *bs)
 {
-    if (bs->backing && bs->backing->bs->encrypted)
-        return bs->backing_file;
-    else if (bs->encrypted)
-        return bs->filename;
-    else
+    if (bs->backing && bs->backing->bs->encrypted) {
+        return g_strdup(bs->backing_file);
+    } else if (bs->encrypted) {
+        return g_strdup(bs->filename);
+    } else {
         return NULL;
+    }
 }
 
 void bdrv_get_backing_filename(BlockDriverState *bs,
diff --git a/monitor.c b/monitor.c
index 0841d436b0..bb3000a2df 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4056,10 +4056,13 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                 BlockCompletionFunc *completion_cb,
                                 void *opaque)
 {
+    char *enc_filename;
     int err;
 
+    enc_filename = bdrv_get_encrypted_filename(bs);
     monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
-                   bdrv_get_encrypted_filename(bs));
+                   enc_filename ?: "");
+    g_free(enc_filename);
 
     mon->password_completion_cb = completion_cb;
     mon->password_opaque = opaque;
-- 
2.11.0

[Qemu-devel] [PATCH v6 3/9] block: Avoid BlockDriverState.filename

[Max Reitz]

In places which directly pass a filename to the OS, we should not use
the filename field at all but exact_filename instead (although the
former currently equals the latter if that is set).

In raw_open_common(), we do not need to access BDS.filename because we
already have a local variable pointing to the filename.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c            | 5 +++--
 block/file-posix.c | 6 +++---
 block/file-win32.c | 4 ++--
 block/gluster.c    | 3 ++-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 95015251d5..9943d8eff6 100644
--- a/block.c
+++ b/block.c
@@ -1146,8 +1146,9 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     if (ret < 0) {
         if (local_err) {
             error_propagate(errp, local_err);
-        } else if (bs->filename[0]) {
-            error_setg_errno(errp, -ret, "Could not open '%s'", bs->filename);
+        } else if (bs->exact_filename[0]) {
+            error_setg_errno(errp, -ret, "Could not open '%s'",
+                             bs->exact_filename);
         } else {
             error_setg_errno(errp, -ret, "Could not open image");
         }
diff --git a/block/file-posix.c b/block/file-posix.c
index 28b47d977b..2e3acd622d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -590,7 +590,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
 
     /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
     if (rs->fd == -1) {
-        const char *normalized_filename = state->bs->filename;
+        const char *normalized_filename = state->bs->exact_filename;
         ret = raw_normalize_devicepath(&normalized_filename);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not normalize device path");
@@ -2075,7 +2075,7 @@ static bool hdev_is_sg(BlockDriverState *bs)
     int sg_version;
     int ret;
 
-    if (stat(bs->filename, &st) < 0 || !S_ISCHR(st.st_mode)) {
+    if (stat(bs->exact_filename, &st) < 0 || !S_ISCHR(st.st_mode)) {
         return false;
     }
 
@@ -2510,7 +2510,7 @@ static int cdrom_reopen(BlockDriverState *bs)
      */
     if (s->fd >= 0)
         qemu_close(s->fd);
-    fd = qemu_open(bs->filename, s->open_flags, 0644);
+    fd = qemu_open(bs->exact_filename, s->open_flags, 0644);
     if (fd < 0) {
         s->fd = -1;
         return -EIO;
diff --git a/block/file-win32.c b/block/file-win32.c
index 800fabdd72..040c71830e 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -457,7 +457,7 @@ static void raw_close(BlockDriverState *bs)
 
     CloseHandle(s->hfile);
     if (bs->open_flags & BDRV_O_TEMPORARY) {
-        unlink(bs->filename);
+        unlink(bs->exact_filename);
     }
 }
 
@@ -525,7 +525,7 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
                                               DWORD * high);
     get_compressed_t get_compressed;
     struct _stati64 st;
-    const char *filename = bs->filename;
+    const char *filename = bs->exact_filename;
     /* WinNT support GetCompressedFileSize to determine allocate size */
     get_compressed =
         (get_compressed_t) GetProcAddress(GetModuleHandle("kernel32"),
diff --git a/block/gluster.c b/block/gluster.c
index ded13fb5fb..f6c2484c11 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -878,7 +878,8 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
     gconf->has_debug = true;
     gconf->logfile = g_strdup(s->logfile);
     gconf->has_logfile = true;
-    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, NULL, errp);
+    reop_s->glfs = qemu_gluster_init(gconf, state->bs->exact_filename, NULL,
+                                     errp);
     if (reop_s->glfs == NULL) {
         ret = -errno;
         goto exit;
-- 
2.11.0

Re: [Qemu-devel] [PATCH v6 3/9] block: Avoid BlockDriverState.filename

[Eric Blake]

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

On 01/13/2017 02:52 PM, Max Reitz wrote:
> In places which directly pass a filename to the OS, we should not use
> the filename field at all but exact_filename instead (although the
> former currently equals the latter if that is set).
> 
> In raw_open_common(), we do not need to access BDS.filename because we
> already have a local variable pointing to the filename.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c            | 5 +++--
>  block/file-posix.c | 6 +++---
>  block/file-win32.c | 4 ++--
>  block/gluster.c    | 3 ++-
>  4 files changed, 10 insertions(+), 8 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH v6 4/9] block: Do not blindly copy filename from file

[Max Reitz]

bdrv_refresh_filename() can do the same and it has some checks whether
the filename can actually be inherited or not, so we can let it do its
job in bdrv_open_inherit() after bdrv_open_common() has been called.

The only thing we need to set in bdrv_open_common() is the
exact_filename of a BDS without an underlying file, for two reasons:
(1) It cannot be inherited from an underlying file BDS, so it has to be
    set somewhere.
(2) The driver may need the filename in its bdrv_file_open()
    implementation (format drivers do not need their own filename,
    though they may need their file BDS's name).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 9943d8eff6..19f8a84d03 100644
--- a/block.c
+++ b/block.c
@@ -1116,12 +1116,11 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         bs->detect_zeroes = value;
     }
 
-    if (filename != NULL) {
-        pstrcpy(bs->filename, sizeof(bs->filename), filename);
+    if (!file && filename) {
+        pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), filename);
     } else {
-        bs->filename[0] = '\0';
+        assert(!drv->bdrv_needs_filename);
     }
-    pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->filename);
 
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
-- 
2.11.0

Re: [Qemu-devel] [PATCH v6 4/9] block: Do not blindly copy filename from file

[Eric Blake]

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

On 01/13/2017 02:52 PM, Max Reitz wrote:
> bdrv_refresh_filename() can do the same and it has some checks whether
> the filename can actually be inherited or not, so we can let it do its
> job in bdrv_open_inherit() after bdrv_open_common() has been called.
> 
> The only thing we need to set in bdrv_open_common() is the
> exact_filename of a BDS without an underlying file, for two reasons:
> (1) It cannot be inherited from an underlying file BDS, so it has to be
>     set somewhere.
> (2) The driver may need the filename in its bdrv_file_open()
>     implementation (format drivers do not need their own filename,
>     though they may need their file BDS's name).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH v6 5/9] block: Add bdrv_filename()

[Max Reitz]

Split the part which actually refreshes the BlockDriverState.filename
field off of bdrv_refresh_filename() into a more generic function
bdrv_filename(), which first calls bdrv_refresh_filename() and then
stores a qemu-usable filename in the given buffer instead of
BlockDriverState.filename.

Since bdrv_refresh_filename() therefore no longer refreshes that field,
all of the existing calls to that function have to be replaced by calls
to bdrv_filename() "manually" refreshing the BDS filename field (this is
only temporary).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h |  1 +
 block.c               | 50 +++++++++++++++++++++++++++++++++++++++++---------
 block/replication.c   |  2 +-
 blockdev.c            |  3 ++-
 4 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 3425e9fa79..8abc3da69f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -252,6 +252,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
 void bdrv_refresh_filename(BlockDriverState *bs);
+char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz);
 int bdrv_truncate(BlockDriverState *bs, int64_t offset);
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
diff --git a/block.c b/block.c
index 19f8a84d03..a631d94702 100644
--- a/block.c
+++ b/block.c
@@ -1731,7 +1731,8 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     bdrv_append(bs_snapshot, bs);
 
     bs_snapshot->backing_overridden = true;
-    bdrv_refresh_filename(bs_snapshot);
+    bdrv_filename(bs_snapshot, bs_snapshot->filename,
+                  sizeof(bs_snapshot->filename));
 
     g_free(tmp_filename);
     return bs_snapshot;
@@ -1923,7 +1924,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
         }
     }
 
-    bdrv_refresh_filename(bs);
+    bdrv_filename(bs, bs->filename, sizeof(bs->filename));
 
     /* Check if any unknown options were used */
     if (options && (qdict_size(options) != 0)) {
@@ -4101,9 +4102,6 @@ static bool append_significant_runtime_options(QDict *d, BlockDriverState *bs)
  *  - full_open_options: Options which, when given when opening a block device
  *                       (without a filename), result in a BDS (mostly)
  *                       equalling the given one
- *  - filename: If exact_filename is set, it is copied here. Otherwise,
- *              full_open_options is converted to a JSON object, prefixed with
- *              "json:" (for use through the JSON pseudo protocol) and put here.
  */
 void bdrv_refresh_filename(BlockDriverState *bs)
 {
@@ -4120,7 +4118,8 @@ void bdrv_refresh_filename(BlockDriverState *bs)
     /* This BDS's file name may depend on any of its children's file names, so
      * refresh those first */
     QLIST_FOREACH(child, &bs->children, next) {
-        bdrv_refresh_filename(child->bs);
+        bdrv_filename(child->bs, child->bs->filename,
+                      sizeof(child->bs->filename));
 
         if (child->role == &child_backing && child->bs->backing_overridden) {
             bs->backing_overridden = true;
@@ -4184,15 +4183,48 @@ void bdrv_refresh_filename(BlockDriverState *bs)
             strcpy(bs->exact_filename, bs->file->bs->exact_filename);
         }
     }
+}
+
+/* First refreshes exact_filename and full_open_options by calling
+ * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into
+ * the target buffer. Otherwise, full_open_options is converted to a JSON
+ * object, prefixed with "json:" (for use through the JSON pseudo protocol) and
+ * put there.
+ *
+ * If @dest is not NULL, the filename will be truncated to @sz - 1 bytes and
+ * placed there. If @sz > 0, it will always be null-terminated.
+ *
+ * If @dest is NULL, @sz is ignored and a new buffer will be allocated which is
+ * large enough to hold the filename and the trailing '\0'. This buffer is then
+ * returned and has to be freed by the caller when it is no longer needed.
+ *
+ * Returns @dest if it is not NULL, and the newly allocated buffer otherwise.
+ */
+char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz)
+{
+    bdrv_refresh_filename(bs);
+
+    if (sz > INT_MAX) {
+        sz = INT_MAX;
+    }
 
     if (bs->exact_filename[0]) {
-        pstrcpy(bs->filename, sizeof(bs->filename), bs->exact_filename);
+        if (dest) {
+            pstrcpy(dest, sz, bs->exact_filename);
+        } else {
+            dest = g_strdup(bs->exact_filename);
+        }
     } else {
         QString *json = qobject_to_json(QOBJECT(bs->full_open_options));
-        snprintf(bs->filename, sizeof(bs->filename), "json:%s",
-                 qstring_get_str(json));
+        if (dest) {
+            snprintf(dest, sz, "json:%s", qstring_get_str(json));
+        } else {
+            dest = g_strdup_printf("json:%s", qstring_get_str(json));
+        }
         QDECREF(json);
     }
+
+    return dest;
 }
 
 char *bdrv_dirname(BlockDriverState *bs, Error **errp)
diff --git a/block/replication.c b/block/replication.c
index 729dd12499..4deff13645 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -586,7 +586,7 @@ static void replication_done(void *opaque, int ret)
         s->replication_state = BLOCK_REPLICATION_DONE;
 
         /* refresh top bs's filename */
-        bdrv_refresh_filename(bs);
+        bdrv_filename(bs, bs->filename, sizeof(bs->filename));
         s->active_disk = NULL;
         s->secondary_disk = NULL;
         s->hidden_disk = NULL;
diff --git a/blockdev.c b/blockdev.c
index 7889babd40..4128fee78f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1797,7 +1797,8 @@ static void external_snapshot_commit(BlkActionState *common)
 
     if (image_was_existing) {
         state->new_bs->backing_overridden = true;
-        bdrv_refresh_filename(state->new_bs);
+        bdrv_filename(state->new_bs, state->new_bs->filename,
+                      sizeof(state->new_bs->filename));
     }
 }
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH v6 5/9] block: Add bdrv_filename()

[Eric Blake]

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

On 01/13/2017 02:52 PM, Max Reitz wrote:
> Split the part which actually refreshes the BlockDriverState.filename
> field off of bdrv_refresh_filename() into a more generic function
> bdrv_filename(), which first calls bdrv_refresh_filename() and then
> stores a qemu-usable filename in the given buffer instead of
> BlockDriverState.filename.
> 
> Since bdrv_refresh_filename() therefore no longer refreshes that field,
> all of the existing calls to that function have to be replaced by calls
> to bdrv_filename() "manually" refreshing the BDS filename field (this is
> only temporary).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block.h |  1 +
>  block.c               | 50 +++++++++++++++++++++++++++++++++++++++++---------
>  block/replication.c   |  2 +-
>  blockdev.c            |  3 ++-
>  4 files changed, 45 insertions(+), 11 deletions(-)
> 

> +
> +/* First refreshes exact_filename and full_open_options by calling
> + * bdrv_refresh_filename(). Then, if exact_filename is set, it is copied into
> + * the target buffer. Otherwise, full_open_options is converted to a JSON
> + * object, prefixed with "json:" (for use through the JSON pseudo protocol) and
> + * put there.
> + *
> + * If @dest is not NULL, the filename will be truncated to @sz - 1 bytes and
> + * placed there. If @sz > 0, it will always be null-terminated.
> + *
> + * If @dest is NULL, @sz is ignored and a new buffer will be allocated which is
> + * large enough to hold the filename and the trailing '\0'. This buffer is then
> + * returned and has to be freed by the caller when it is no longer needed.
> + *
> + * Returns @dest if it is not NULL, and the newly allocated buffer otherwise.
> + */
> +char *bdrv_filename(BlockDriverState *bs, char *dest, size_t sz)

Seems like a reasonable interface.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH v6 7/9] block: Drop BlockDriverState.filename

[Max Reitz]

That field is now only used during initialization of BlockDriverStates
(opening images) and for error or warning messages. Performance is not
that much of an issue here, so we can drop the field and replace its use
by a call to bdrv_filename(). By doing so we can ensure the result
always to be recent, whereas the contents of BlockDriverState.filename
may have been obsoleted by manipulations of single BlockDriverStates or
of the BDS graph.

The users of the BDS filename field were changed as follows:
- copying the filename into another buffer is trivially replaced by
  using bdrv_filename() instead of the copy function
- strdup() on the filename is replaced by a call to
  bdrv_filename(filename, NULL, 0)
- bdrv_filename(bs, bs->filename, sizeof(bs->filename)) is replaced by
  bdrv_refresh_filename(bs)
  (these were introduced by the patch before the previous patch)
- anywhere else bdrv_filename(..., NULL, 0) is used, any access to
  BlockDriverState.filename is then replaced by the buffer returned, and
  it is freed when it is no longer needed

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block_int.h |  1 -
 block.c                   | 31 ++++++++++++++++++++-----------
 block/commit.c            |  4 +++-
 block/mirror.c            | 16 ++++++++++++----
 block/nbd.c               |  5 ++++-
 block/nfs.c               |  4 +++-
 block/qapi.c              |  4 ++--
 block/raw-format.c        |  4 +++-
 block/replication.c       |  2 +-
 block/vhdx-log.c          |  7 +++++--
 block/vmdk.c              | 22 ++++++++++++++++------
 block/vpc.c               |  7 +++++--
 blockdev.c                | 35 +++++++++++++++++++++++++----------
 13 files changed, 99 insertions(+), 43 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index eba463b99b..83c7c4e5d9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -466,7 +466,6 @@ struct BlockDriverState {
     QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
     bool walking_aio_notifiers; /* to make removal during iteration safe */
 
-    char filename[PATH_MAX];
     char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
                                     this file image */
     char backing_format[16]; /* if non-zero and backing_file exists */
diff --git a/block.c b/block.c
index a631d94702..58f7575d4f 100644
--- a/block.c
+++ b/block.c
@@ -1006,6 +1006,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
                             QDict *options, Error **errp)
 {
     int ret, open_flags;
+    char *filename_buffer = NULL;
     const char *filename;
     const char *driver_name = NULL;
     const char *node_name = NULL;
@@ -1033,7 +1034,8 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     assert(drv != NULL);
 
     if (file != NULL) {
-        filename = file->bs->filename;
+        filename_buffer = bdrv_filename(file->bs, NULL, 0);
+        filename = filename_buffer;
     } else {
         filename = qdict_get_try_str(options, "filename");
     }
@@ -1172,6 +1174,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     assert(is_power_of_2(bs->bl.request_alignment));
 
     qemu_opts_del(opts);
+    g_free(filename_buffer);
     return 0;
 
 free_and_fail:
@@ -1181,6 +1184,7 @@ free_and_fail:
     bs->drv = NULL;
 fail_opts:
     qemu_opts_del(opts);
+    g_free(filename_buffer);
     return ret;
 }
 
@@ -1453,7 +1457,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     }
     bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_backing);
     bs->open_flags &= ~BDRV_O_NO_BACKING;
-    pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
+    bdrv_filename(backing_hd, bs->backing_file, sizeof(bs->backing_file));
     pstrcpy(bs->backing_format, sizeof(bs->backing_format),
             backing_hd->drv ? backing_hd->drv->format_name : "");
 
@@ -1731,8 +1735,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     bdrv_append(bs_snapshot, bs);
 
     bs_snapshot->backing_overridden = true;
-    bdrv_filename(bs_snapshot, bs_snapshot->filename,
-                  sizeof(bs_snapshot->filename));
+    bdrv_refresh_filename(bs_snapshot);
 
     g_free(tmp_filename);
     return bs_snapshot;
@@ -1924,7 +1927,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
         }
     }
 
-    bdrv_filename(bs, bs->filename, sizeof(bs->filename));
+    bdrv_refresh_filename(bs);
 
     /* Check if any unknown options were used */
     if (options && (qdict_size(options) != 0)) {
@@ -2297,8 +2300,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
             if (local_err != NULL) {
                 error_propagate(errp, local_err);
             } else {
+                char *filename = bdrv_filename(reopen_state->bs, NULL, 0);
                 error_setg(errp, "failed while preparing to reopen image '%s'",
-                           reopen_state->bs->filename);
+                           filename);
+                g_free(filename);
             }
             goto error;
         }
@@ -2662,6 +2667,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
                            BlockDriverState *base, const char *backing_file_str)
 {
     BlockDriverState *new_top_bs = NULL;
+    char *base_filename = NULL;
     int ret = -EIO;
 
     if (!top->drv || !base->drv) {
@@ -2688,7 +2694,10 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     }
 
     /* success - we can delete the intermediate states, and link top->base */
-    backing_file_str = backing_file_str ? backing_file_str : base->filename;
+    if (!backing_file_str) {
+        base_filename = bdrv_filename(base, NULL, 0);
+        backing_file_str = base_filename;
+    }
     ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
                                    base->drv ? base->drv->format_name : "");
     if (ret) {
@@ -2698,6 +2707,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
 
     ret = 0;
 exit:
+    g_free(base_filename);
     return ret;
 }
 
@@ -3119,7 +3129,7 @@ char *bdrv_get_encrypted_filename(BlockDriverState *bs)
     if (bs->backing && bs->backing->bs->encrypted) {
         return g_strdup(bs->backing_file);
     } else if (bs->encrypted) {
-        return g_strdup(bs->filename);
+        return bdrv_filename(bs, NULL, 0);
     } else {
         return NULL;
     }
@@ -3214,7 +3224,7 @@ bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag)
 }
 
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
- * relative, it must be relative to the chain.  So, passing in bs->filename
+ * relative, it must be relative to the chain.  So, passing in the filename
  * from a BDS as backing_file should not be done, as that may be relative to
  * the CWD rather than the chain. */
 BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
@@ -4118,8 +4128,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
     /* This BDS's file name may depend on any of its children's file names, so
      * refresh those first */
     QLIST_FOREACH(child, &bs->children, next) {
-        bdrv_filename(child->bs, child->bs->filename,
-                      sizeof(child->bs->filename));
+        bdrv_refresh_filename(child->bs);
 
         if (child->role == &child_backing && child->bs->backing_overridden) {
             bs->backing_overridden = true;
diff --git a/block/commit.c b/block/commit.c
index c284e8535d..ec31ee82e7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -230,7 +230,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     overlay_bs = bdrv_find_overlay(bs, top);
 
     if (overlay_bs == NULL) {
-        error_setg(errp, "Could not find overlay image for %s:", top->filename);
+        char *top_filename = bdrv_filename(top, NULL, 0);
+        error_setg(errp, "Could not find overlay image for %s:", top_filename);
+        g_free(top_filename);
         return;
     }
 
diff --git a/block/mirror.c b/block/mirror.c
index 6cb6cbd127..fb6720add9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1069,25 +1069,33 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
 
     length = bdrv_getlength(bs);
     if (length < 0) {
-        error_setg_errno(errp, -length,
-                         "Unable to determine length of %s", bs->filename);
+        char *filename = bdrv_filename(bs, NULL, 0);
+        error_setg_errno(errp, -length, "Unable to determine length of %s",
+                         filename);
+        g_free(filename);
         goto error_restore_flags;
     }
 
     base_length = bdrv_getlength(base);
     if (base_length < 0) {
+        char *base_filename = bdrv_filename(base, NULL, 0);
         error_setg_errno(errp, -base_length,
-                         "Unable to determine length of %s", base->filename);
+                         "Unable to determine length of %s", base_filename);
+        g_free(base_filename);
         goto error_restore_flags;
     }
 
     if (length > base_length) {
         ret = bdrv_truncate(base, length);
         if (ret < 0) {
+            char *filename = bdrv_filename(bs, NULL, 0);
+            char *base_filename = bdrv_filename(base, NULL, 0);
             error_setg_errno(errp, -ret,
                             "Top image %s is larger than base image %s, and "
                              "resize of base image failed",
-                             bs->filename, base->filename);
+                             filename, base_filename);
+            g_free(filename);
+            g_free(base_filename);
             goto error_restore_flags;
         }
     }
diff --git a/block/nbd.c b/block/nbd.c
index ea5d7a819e..bd58979511 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -533,6 +533,7 @@ static char *nbd_dirname(BlockDriverState *bs, Error **errp)
 {
     BDRVNBDState *s = bs->opaque;
     const char *host = NULL, *port = NULL, *path = NULL;
+    char *filename;
 
     if (s->saddr->type == SOCKET_ADDRESS_KIND_INET) {
         const InetSocketAddress *inet = s->saddr->u.inet.data;
@@ -552,8 +553,10 @@ static char *nbd_dirname(BlockDriverState *bs, Error **errp)
                                s->export ?: "", s->export ? "/" : "");
     }
 
+    filename = bdrv_filename(bs, NULL, 0);
     error_setg(errp, "Cannot generate a base directory for NBD node '%s'",
-               bs->filename);
+               filename);
+    g_free(filename);
     return NULL;
 }
 
diff --git a/block/nfs.c b/block/nfs.c
index 6e68c27437..16fbc5fba7 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -795,8 +795,10 @@ static char *nfs_dirname(BlockDriverState *bs, Error **errp)
     NFSClient *client = bs->opaque;
 
     if (client->uid || client->gid) {
+        char *filename = bdrv_filename(bs, NULL, 0);
         error_setg(errp, "Cannot generate a base directory for NBD node '%s'",
-                   bs->filename);
+                   filename);
+        g_free(filename);
         return NULL;
     }
 
diff --git a/block/qapi.c b/block/qapi.c
index 1543dd691a..d76bf95dce 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -41,7 +41,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
     BlockDriverState *bs0;
     BlockDeviceInfo *info = g_malloc0(sizeof(*info));
 
-    info->file                   = g_strdup(bs->filename);
+    info->file                   = bdrv_filename(bs, NULL, 0);
     info->ro                     = bs->read_only;
     info->drv                    = g_strdup(bs->drv->format_name);
     info->encrypted              = bs->encrypted;
@@ -243,7 +243,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
     }
 
     info = g_new0(ImageInfo, 1);
-    info->filename        = g_strdup(bs->filename);
+    info->filename        = bdrv_filename(bs, NULL, 0);
     info->format          = g_strdup(bdrv_get_format_name(bs));
     info->virtual_size    = size;
     info->actual_size     = bdrv_get_allocated_file_size(bs);
diff --git a/block/raw-format.c b/block/raw-format.c
index 8404a82e0c..67539b0630 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -391,6 +391,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
         bs->file->bs->supported_zero_flags;
 
     if (bs->probed && !bdrv_is_read_only(bs)) {
+        char *filename = bdrv_filename(bs->file->bs, NULL, 0);
         fprintf(stderr,
                 "WARNING: Image format was not specified for '%s' and probing "
                 "guessed raw.\n"
@@ -398,7 +399,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                 "raw images, write operations on block 0 will be restricted.\n"
                 "         Specify the 'raw' format explicitly to remove the "
                 "restrictions.\n",
-                bs->file->bs->filename);
+                filename);
+        g_free(filename);
     }
 
     ret = raw_read_options(options, bs, s, errp);
diff --git a/block/replication.c b/block/replication.c
index 4deff13645..729dd12499 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -586,7 +586,7 @@ static void replication_done(void *opaque, int ret)
         s->replication_state = BLOCK_REPLICATION_DONE;
 
         /* refresh top bs's filename */
-        bdrv_filename(bs, bs->filename, sizeof(bs->filename));
+        bdrv_refresh_filename(bs);
         s->active_disk = NULL;
         s->secondary_disk = NULL;
         s->hidden_disk = NULL;
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 02eb104310..e7f4cff7c8 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -786,14 +786,17 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
 
     if (logs.valid) {
         if (bs->read_only) {
+            char *filename = bdrv_filename(bs, NULL, 0);
+
             ret = -EPERM;
             error_setg(errp,
                        "VHDX image file '%s' opened read-only, but "
                        "contains a log that needs to be replayed",
-                       bs->filename);
+                       filename);
             error_append_hint(errp,  "To replay the log, run:\n"
                               "qemu-img check -r all '%s'\n",
-                              bs->filename);
+                              filename);
+            g_free(filename);
             goto exit;
         }
         /* now flush the log */
diff --git a/block/vmdk.c b/block/vmdk.c
index 87ac4f0e68..c1100b9cb9 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -459,9 +459,11 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
                      extent->l1_table,
                      l1_size);
     if (ret < 0) {
+        char *extent_filename = bdrv_filename(extent->file->bs, NULL, 0);
         error_setg_errno(errp, -ret,
                          "Could not read l1 table from extent '%s'",
-                         extent->file->bs->filename);
+                         extent_filename);
+        g_free(extent_filename);
         goto fail_l1;
     }
     for (i = 0; i < extent->l1_size; i++) {
@@ -479,9 +481,11 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
                          extent->l1_backup_table,
                          l1_size);
         if (ret < 0) {
+            char *extent_filename = bdrv_filename(extent->file->bs, NULL, 0);
             error_setg_errno(errp, -ret,
                              "Could not read l1 backup table from extent '%s'",
-                             extent->file->bs->filename);
+                             extent_filename);
+            g_free(extent_filename);
             goto fail_l1b;
         }
         for (i = 0; i < extent->l1_size; i++) {
@@ -510,9 +514,11 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
 
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
+        char *filename = bdrv_filename(file->bs, NULL, 0);
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
-                         file->bs->filename);
+                         filename);
+        g_free(filename);
         return ret;
     }
     ret = vmdk_add_extent(bs, file, false,
@@ -587,9 +593,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
 
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
+        char *filename = bdrv_filename(file->bs, NULL, 0);
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
-                         file->bs->filename);
+                         filename);
+        g_free(filename);
         return -EINVAL;
     }
     if (header.capacity == 0) {
@@ -841,8 +849,10 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
         if (!path_is_absolute(fname) && !path_has_protocol(fname) &&
             !desc_file_path[0])
         {
+            char *filename = bdrv_filename(bs->file->bs, NULL, 0);
             error_setg(errp, "Cannot use relative extent paths with VMDK "
-                       "descriptor file '%s'", bs->file->bs->filename);
+                       "descriptor file '%s'", filename);
+            g_free(filename);
             return -EINVAL;
         }
 
@@ -2169,7 +2179,7 @@ static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
     ImageInfo *info = g_new0(ImageInfo, 1);
 
     *info = (ImageInfo){
-        .filename         = g_strdup(extent->file->bs->filename),
+        .filename         = bdrv_filename(extent->file->bs, NULL, 0),
         .format           = g_strdup(extent->type),
         .virtual_size     = extent->sectors * BDRV_SECTOR_SIZE,
         .compressed       = extent->compressed,
diff --git a/block/vpc.c b/block/vpc.c
index 8d5886f003..2d63e20f7f 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -270,9 +270,12 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
 
     checksum = be32_to_cpu(footer->checksum);
     footer->checksum = 0;
-    if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum)
+    if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum) {
+        char *filename = bdrv_filename(bs, NULL, 0);
         fprintf(stderr, "block-vpc: The header checksum of '%s' is "
-            "incorrect.\n", bs->filename);
+                "incorrect.\n", filename);
+        g_free(filename);
+    }
 
     /* Write 'checksum' back to footer, or else will leave it with zero. */
     footer->checksum = cpu_to_be32(checksum);
diff --git a/blockdev.c b/blockdev.c
index 4128fee78f..6f0d3957bb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -239,10 +239,13 @@ bool drive_check_orphaned(void)
         /* Unless this is a default drive, this may be an oversight. */
         if (!blk_get_attached_dev(blk) && !dinfo->is_default &&
             dinfo->type != IF_NONE) {
+            char *filename = blk_bs(blk) ? bdrv_filename(blk_bs(blk), NULL, 0)
+                                         : NULL;
             fprintf(stderr, "Warning: Orphaned drive without device: "
                     "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
-                    blk_name(blk), blk_bs(blk) ? blk_bs(blk)->filename : "",
-                    if_name[dinfo->type], dinfo->bus, dinfo->unit);
+                    blk_name(blk), filename ?: "", if_name[dinfo->type],
+                    dinfo->bus, dinfo->unit);
+            g_free(filename);
             rs = true;
         }
     }
@@ -1715,15 +1718,18 @@ static void external_snapshot_prepare(BlkActionState *common,
         /* create new image w/backing file */
         mode = s->has_mode ? s->mode : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
         if (mode != NEW_IMAGE_MODE_EXISTING) {
+            char *backing_file;
             int64_t size = bdrv_getlength(state->old_bs);
             if (size < 0) {
                 error_setg_errno(errp, -size, "bdrv_getlength failed");
                 return;
             }
-            bdrv_img_create(new_image_file, format,
-                            state->old_bs->filename,
+
+            backing_file = bdrv_filename(state->old_bs, NULL, 0);
+            bdrv_img_create(new_image_file, format, backing_file,
                             state->old_bs->drv->format_name,
                             NULL, size, flags, &local_err, false);
+            g_free(backing_file);
             if (local_err) {
                 error_propagate(errp, local_err);
                 return;
@@ -1797,8 +1803,7 @@ static void external_snapshot_commit(BlkActionState *common)
 
     if (image_was_existing) {
         state->new_bs->backing_overridden = true;
-        bdrv_filename(state->new_bs, state->new_bs->filename,
-                      sizeof(state->new_bs->filename));
+        bdrv_refresh_filename(state->new_bs);
     }
 }
 
@@ -2943,6 +2948,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     AioContext *aio_context;
     Error *local_err = NULL;
     const char *base_name = NULL;
+    char *base_bs_filename = NULL;
 
     if (!has_on_error) {
         on_error = BLOCKDEV_ON_ERROR_REPORT;
@@ -2983,7 +2989,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
             goto out;
         }
         assert(bdrv_get_aio_context(base_bs) == aio_context);
-        base_name = base_bs->filename;
+        base_name = base_bs_filename = bdrv_filename(base_bs, NULL, 0);
     }
 
     /* Check for op blockers in the whole chain between bs and base */
@@ -3015,6 +3021,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
 
 out:
     aio_context_release(aio_context);
+    g_free(base_bs_filename);
 }
 
 void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
@@ -3067,9 +3074,11 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
     top_bs = bs;
 
     if (has_top && top) {
-        if (strcmp(bs->filename, top) != 0) {
+        char *filename = bdrv_filename(bs, NULL, 0);
+        if (strcmp(filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
+        g_free(filename);
     }
 
     if (top_bs == NULL) {
@@ -3205,9 +3214,11 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
     if (backup->mode != NEW_IMAGE_MODE_EXISTING) {
         assert(backup->format);
         if (source) {
-            bdrv_img_create(backup->target, backup->format, source->filename,
+            char *source_filename = bdrv_filename(source, NULL, 0);
+            bdrv_img_create(backup->target, backup->format, source_filename,
                             source->drv->format_name, NULL,
                             size, flags, &local_err, false);
+            g_free(source_filename);
         } else {
             bdrv_img_create(backup->target, backup->format, NULL, NULL, NULL,
                             size, flags, &local_err, false);
@@ -3497,15 +3508,19 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
         bdrv_img_create(arg->target, format,
                         NULL, NULL, NULL, size, flags, &local_err, false);
     } else {
+        char *source_filename;
+
         switch (arg->mode) {
         case NEW_IMAGE_MODE_EXISTING:
             break;
         case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
             /* create new image with backing file */
+            source_filename = bdrv_filename(source, NULL, 0);
             bdrv_img_create(arg->target, format,
-                            source->filename,
+                            source_filename,
                             source->drv->format_name,
                             NULL, size, flags, &local_err, false);
+            g_free(source_filename);
             break;
         default:
             abort();
-- 
2.11.0

[Qemu-devel] [PATCH v6 8/9] block: Complete move to pull filename updates

[Max Reitz]

Before this patch series, filename were updates in a push model: Every
change to the BDS graph required bdrv_refresh_filename() to be called.
This patch completes the transition to a pull model: Every time a
filename is queried, the caller has to take responsibility that the new
filename is up-to-date.

In the case of bdrv_filename(), this is done automatically through the
call of bdrv_refresh_filename(). However, some places bypass that
function and query the BDS fields full_open_options and exact_filename
instead. For this case, this patch establishes rules on when such an
access requires a call to bdrv_refresh_filename() beforehand.

There are two places in the block layer that do require such a call
before they access a BDS's exact_filename, so add it there and drop it
in places that just called it because they followed the push model.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block_int.h | 12 ++++++++++++
 block.c                   |  4 +---
 block/replication.c       |  2 --
 block/vmdk.c              |  2 ++
 blockdev.c                |  1 -
 5 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 83c7c4e5d9..a3501a1163 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -471,7 +471,19 @@ struct BlockDriverState {
     char backing_format[16]; /* if non-zero and backing_file exists */
     bool backing_overridden; /* backing file has been specified by the user */
 
+    /* Before reading this field, bdrv_refresh_filename() should be called.
+     * For BlockDriver.bdrv_refresh_filename() and .bdrv_gather_child_options()
+     * implementations, this has already been done both for the respective BDS
+     * and recursively for all of its children. */
     QDict *full_open_options;
+
+    /* Before reading this field, bdrv_refresh_filename() should be called.
+     * For BlockDriver.bdrv_refresh_filename() implementations, this has already
+     * been done recursively for all of the respective BDS's children.
+     * Protocol drivers are generally free to access this field without a prior
+     * bdrv_refresh_filename() call because it will not be changed by the block
+     * layer from its initial value unless the protocol driver in question
+     * implements BlockDriver.bdrv_refresh_filename(). */
     char exact_filename[PATH_MAX];
 
     char *dirname;
diff --git a/block.c b/block.c
index 58f7575d4f..64e07aebd5 100644
--- a/block.c
+++ b/block.c
@@ -1735,7 +1735,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
     bdrv_append(bs_snapshot, bs);
 
     bs_snapshot->backing_overridden = true;
-    bdrv_refresh_filename(bs_snapshot);
 
     g_free(tmp_filename);
     return bs_snapshot;
@@ -1927,8 +1926,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
         }
     }
 
-    bdrv_refresh_filename(bs);
-
     /* Check if any unknown options were used */
     if (options && (qdict_size(options) != 0)) {
         const QDictEntry *entry = qdict_first(options);
@@ -4257,6 +4254,7 @@ char *bdrv_dirname(BlockDriverState *bs, Error **errp)
         return bdrv_dirname(bs->file->bs, errp);
     }
 
+    bdrv_refresh_filename(bs);
     if (bs->exact_filename[0] != '\0') {
         return path_combine(bs->exact_filename, "");
     }
diff --git a/block/replication.c b/block/replication.c
index 729dd12499..b64540e670 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -585,8 +585,6 @@ static void replication_done(void *opaque, int ret)
     if (ret == 0) {
         s->replication_state = BLOCK_REPLICATION_DONE;
 
-        /* refresh top bs's filename */
-        bdrv_refresh_filename(bs);
         s->active_disk = NULL;
         s->secondary_disk = NULL;
         s->hidden_disk = NULL;
diff --git a/block/vmdk.c b/block/vmdk.c
index c1100b9cb9..f3cd2310c0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -937,6 +937,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
     }
     s->create_type = g_strdup(ct);
     s->desc_offset = 0;
+
+    bdrv_refresh_filename(bs->file->bs);
     ret = vmdk_parse_extents(buf, bs, bs->file->bs->exact_filename, options,
                              errp);
 exit:
diff --git a/blockdev.c b/blockdev.c
index 6f0d3957bb..f2210c0021 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1803,7 +1803,6 @@ static void external_snapshot_commit(BlkActionState *common)
 
     if (image_was_existing) {
         state->new_bs->backing_overridden = true;
-        bdrv_refresh_filename(state->new_bs);
     }
 }
 
-- 
2.11.0

[Qemu-devel] [PATCH v6 9/9] iotests: Test changed Quorum filename

[Max Reitz]

After drive-mirror replacing a Quorum child, the filename of the Quorum
BDS should reflect the change. This patch replaces the existing test for
whether the operation did actually exchange the BDS (which simply tested
whether the new BDS existed) by a test which examines the children list
contained in the Quorum BDS's filename as returned by query-block.

As a nice side effect of confirming that the new BDS actually belongs to
the Quorum BDS, this checks whether the filename was properly updated.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/041 | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index bc6cf782fe..aa6efa5b5b 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -18,6 +18,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+import json
 import time
 import os
 import iotests
@@ -961,9 +962,18 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         self.complete_and_wait('job0')
-        self.assert_has_block_node("repair0", quorum_repair_img)
-        # TODO: a better test requiring some QEMU infrastructure will be added
-        #       to check that this file is really driven by quorum
+
+        result = self.vm.qmp('query-block')
+        for blockdev in result['return']:
+            if blockdev['device'] == 'quorum0':
+                filename = blockdev['inserted']['image']['filename']
+                self.assertEquals(filename[:5], 'json:')
+                children = json.loads(filename[5:])['children']
+                self.assertEquals(children[0]['file']['filename'], quorum_img1)
+                self.assertEquals(children[1]['file']['filename'],
+                                  quorum_repair_img)
+                self.assertEquals(children[2]['file']['filename'], quorum_img3)
+
         self.vm.shutdown()
 
 if __name__ == '__main__':
-- 
2.11.0

[Qemu-devel] [PATCH v6 6/9] qemu-img: Use bdrv_filename() for map

[Max Reitz]

Replaces bs->filename by the result of bdrv_filename() in the
qemu-img map subcommand. Since that value is queried relatively often,
however, we should try to reuse the last result.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 5df66fe661..6f14792025 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2469,12 +2469,14 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
 }
 
 static int get_block_status(BlockDriverState *bs, int64_t sector_num,
-                            int nb_sectors, MapEntry *e)
+                            int nb_sectors, MapEntry *e,
+                            BlockDriverState **current_file)
 {
     int64_t ret;
     int depth;
     BlockDriverState *file;
     bool has_offset;
+    char *filename;
 
     /* As an optimization, we could cache the current range of unallocated
      * clusters in each file of the chain, and avoid querying the same
@@ -2503,6 +2505,18 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
 
     has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
 
+    if (!file || !has_offset) {
+        g_free(e->filename);
+        filename = NULL;
+    } else if (file == *current_file && e->has_filename) {
+        filename = e->filename;
+    } else {
+        g_free(e->filename);
+        filename = bdrv_filename(file, NULL, 0);
+    }
+
+    *current_file = file;
+
     *e = (MapEntry) {
         .start = sector_num * BDRV_SECTOR_SIZE,
         .length = nb_sectors * BDRV_SECTOR_SIZE,
@@ -2511,8 +2525,8 @@ static int get_block_status(BlockDriverState *bs, int64_t sector_num,
         .offset = ret & BDRV_BLOCK_OFFSET_MASK,
         .has_offset = has_offset,
         .depth = depth,
-        .has_filename = file && has_offset,
-        .filename = file && has_offset ? file->filename : NULL,
+        .has_filename = filename,
+        .filename = filename,
     };
 
     return 0;
@@ -2544,10 +2558,10 @@ static int img_map(int argc, char **argv)
     int c;
     OutputFormat output_format = OFORMAT_HUMAN;
     BlockBackend *blk;
-    BlockDriverState *bs;
+    BlockDriverState *bs, *current_file = NULL;
     const char *filename, *fmt, *output;
     int64_t length;
-    MapEntry curr = { .length = 0 }, next;
+    MapEntry curr = { .length = 0 }, next = { 0 };
     int ret = 0;
     bool image_opts = false;
 
@@ -2633,7 +2647,7 @@ static int img_map(int argc, char **argv)
         /* Probe up to 1 GiB at a time.  */
         nsectors_left = DIV_ROUND_UP(length, BDRV_SECTOR_SIZE) - sector_num;
         n = MIN(1 << (30 - BDRV_SECTOR_BITS), nsectors_left);
-        ret = get_block_status(bs, sector_num, n, &next);
+        ret = get_block_status(bs, sector_num, n, &next, &current_file);
 
         if (ret < 0) {
             error_report("Could not read file metadata: %s", strerror(-ret));
@@ -2648,12 +2662,17 @@ static int img_map(int argc, char **argv)
         if (curr.length > 0) {
             dump_map_entry(output_format, &curr, &next);
         }
+
+        g_free(curr.filename);
         curr = next;
+        curr.filename = g_strdup(curr.filename);
     }
 
     dump_map_entry(output_format, &curr, NULL);
 
 out:
+    g_free(curr.filename);
+    g_free(next.filename);
     blk_unref(blk);
     return ret < 0;
 }
-- 
2.11.0

Re: [Qemu-devel] [PATCH v6 6/9] qemu-img: Use bdrv_filename() for map

[Eric Blake]

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

On 01/16/2017 10:15 AM, Max Reitz wrote:
> Replaces bs->filename by the result of bdrv_filename() in the
> qemu-img map subcommand. Since that value is queried relatively often,
> however, we should try to reuse the last result.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v6 0/9] block: Drop BDS.filename

[Max Reitz]

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

Sorry, for some reason the mail server did not like patch 6 originally.
That's why it cut off there on Friday and I didn't notice until today.
For some reason, modifying the time zone there from -0700 to +0200 fixed
it (???)...

Max


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