[Qemu-devel] [PATCH v5 0/6] block: Drop BDS.filename

[Qemu-devel] [PATCH v5 0/6] block: Drop BDS.filename

[Max Reitz]

*** This series is based on v7 of my ***
*** "blockdev: BlockBackend and media" 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 2 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).


v5: (Rebased and addressed Eric's comments)
- Patch 1: Do not pstrcpy() into a caller-supplied buffer, but
  g_strdup() instead [Eric]
- Patch 2: Rebase conflicts
- Patch 3: Drop bdrv_filename_alloc(), instead bdrv_filename() will
  allocate a buffer if NULL is specified
- Patch 4: Use bdrv_filename() instead of bdrv_filename_alloc()
- Patch 5:
  - Rebase conflicts
  - Use bdrv_filename() instead of bdrv_filename_alloc()


git-backport-diff against v4:

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/6:[0029] [FC] 'block: Change bdrv_get_encrypted_filename()'
002/6:[0004] [FC] 'block: Avoid BlockDriverState.filename'
003/6:[0040] [FC] 'block: Add bdrv_filename()'
004/6:[0002] [FC] 'qemu-img: Use bdrv_filename() for map'
005/6:[0093] [FC] 'block: Drop BlockDriverState.filename'
006/6:[----] [--] 'iotests: Test changed Quorum filename'


Max Reitz (6):
  block: Change bdrv_get_encrypted_filename()
  block: Avoid BlockDriverState.filename
  block: Add bdrv_filename()
  qemu-img: Use bdrv_filename() for map
  block: Drop BlockDriverState.filename
  iotests: Test changed Quorum filename

 block.c                   | 105 +++++++++++++++++++++++++++++++++-------------
 block/commit.c            |   4 +-
 block/gluster.c           |   2 +-
 block/mirror.c            |  16 +++++--
 block/qapi.c              |   4 +-
 block/raw-posix.c         |   8 ++--
 block/raw-win32.c         |   4 +-
 block/raw_bsd.c           |   4 +-
 block/vhdx-log.c          |   5 ++-
 block/vmdk.c              |  22 +++++++---
 block/vpc.c               |   7 +++-
 blockdev.c                |  25 ++++++++---
 include/block/block.h     |   3 +-
 include/block/block_int.h |   1 -
 monitor.c                 |   5 ++-
 qemu-img.c                |  14 ++++++-
 tests/qemu-iotests/041    |  17 ++++++--
 17 files changed, 179 insertions(+), 67 deletions(-)

-- 
2.6.1

[Qemu-devel] [PATCH v5 1/6] 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>
---
 block.c               | 17 ++++++++++-------
 include/block/block.h |  2 +-
 monitor.c             |  5 ++++-
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index f4dec11..60f9450 100644
--- a/block.c
+++ b/block.c
@@ -2585,10 +2585,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);
         }
     }
 }
@@ -2810,14 +2812,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/include/block/block.h b/include/block/block.h
index 28d903c..340c00f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -421,7 +421,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
                             int64_t *cluster_sector_num,
                             int *cluster_nb_sectors);
 
-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);
 void bdrv_get_full_backing_filename(BlockDriverState *bs,
diff --git a/monitor.c b/monitor.c
index 301a143..63b884c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4117,10 +4117,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.6.1

Re: [Qemu-devel] [PATCH v5 1/6] block: Change bdrv_get_encrypted_filename()

[Eric Blake]

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

On 10/19/2015 12:49 PM, Max Reitz wrote:
> 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>
> ---
>  block.c               | 17 ++++++++++-------
>  include/block/block.h |  2 +-
>  monitor.c             |  5 ++++-
>  3 files changed, 15 insertions(+), 9 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 v5 1/6] block: Change bdrv_get_encrypted_filename()

[Kevin Wolf]

Am 19.10.2015 um 20:49 hat Max Reitz geschrieben:
> 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: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v5 2/6] 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/gluster.c   | 2 +-
 block/raw-posix.c | 8 ++++----
 block/raw-win32.c | 4 ++--
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 60f9450..0f08d7c 100644
--- a/block.c
+++ b/block.c
@@ -913,8 +913,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/gluster.c b/block/gluster.c
index 1eb3a8c..176682b 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -358,7 +358,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
 
     gconf = g_new0(GlusterConf, 1);
 
-    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
+    reop_s->glfs = qemu_gluster_init(gconf, state->bs->exact_filename, errp);
     if (reop_s->glfs == NULL) {
         ret = -errno;
         goto exit;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 918c756..9cb7ebd 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -505,7 +505,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
                      "specified. Falling back to aio=threads.\n"
                      "         This will become an error condition in "
                      "future QEMU versions.\n",
-                     bs->filename);
+                     filename);
     }
 #else
     if (bdrv_flags & BDRV_O_NATIVE_AIO) {
@@ -667,7 +667,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
 
     /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
     if (raw_s->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");
@@ -2099,7 +2099,7 @@ static bool hdev_is_sg(BlockDriverState *bs)
     struct sg_scsi_id scsiid;
     int sg_version;
 
-    if (stat(bs->filename, &st) >= 0 && S_ISCHR(st.st_mode) &&
+    if (stat(bs->exact_filename, &st) >= 0 && S_ISCHR(st.st_mode) &&
         !bdrv_ioctl(bs, SG_GET_VERSION_NUM, &sg_version) &&
         !bdrv_ioctl(bs, SG_GET_SCSI_ID, &scsiid)) {
         DPRINTF("SG device found: type=%d, version=%d\n",
@@ -2518,7 +2518,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/raw-win32.c b/block/raw-win32.c
index 2d0907a..e2bbe24 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -417,7 +417,7 @@ static void raw_close(BlockDriverState *bs)
 
     CloseHandle(s->hfile);
     if (bs->open_flags & BDRV_O_TEMPORARY) {
-        unlink(bs->filename);
+        unlink(bs->exact_filename);
     }
 }
 
@@ -485,7 +485,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"),
-- 
2.6.1

Re: [Qemu-devel] [PATCH v5 2/6] block: Avoid BlockDriverState.filename

[Eric Blake]

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

On 10/19/2015 12:49 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/gluster.c   | 2 +-
>  block/raw-posix.c | 8 ++++----
>  block/raw-win32.c | 4 ++--
>  4 files changed, 10 insertions(+), 9 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 v5 2/6] block: Avoid BlockDriverState.filename

[Kevin Wolf]

Am 19.10.2015 um 20:49 hat Max Reitz geschrieben:
> 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>

> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 918c756..9cb7ebd 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -505,7 +505,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>                       "specified. Falling back to aio=threads.\n"
>                       "         This will become an error condition in "
>                       "future QEMU versions.\n",
> -                     bs->filename);
> +                     filename);
>      }
>  #else
>      if (bdrv_flags & BDRV_O_NATIVE_AIO) {

           error_printf("WARNING: aio=native was specified for '%s', but "
                        "is not supported in this build. Falling back to "
                        "aio=threads.\n"
                        "         This will become an error condition in "
                        "future QEMU versions.\n",
                        bs->filename);
       }

That's another instance that should be changed.

Kevin

[Qemu-devel] [PATCH v5 3/6] 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,
some 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>
---
 block.c               | 43 ++++++++++++++++++++++++++++++++++++-------
 block/blkverify.c     |  3 ++-
 block/quorum.c        |  3 ++-
 include/block/block.h |  1 +
 4 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 0f08d7c..1061d80 100644
--- a/block.c
+++ b/block.c
@@ -1527,7 +1527,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 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)) {
@@ -3937,9 +3937,6 @@ static bool append_open_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)
 {
@@ -4027,13 +4024,45 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 
         bs->full_open_options = opts;
     }
+}
+
+/* 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 @sz > 0, the string put into the buffer 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 if (bs->full_open_options) {
         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;
 }
diff --git a/block/blkverify.c b/block/blkverify.c
index c5f8e8d..3af37f8 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -312,7 +312,8 @@ static void blkverify_refresh_filename(BlockDriverState *bs)
     BDRVBlkverifyState *s = bs->opaque;
 
     /* bs->file->bs has already been refreshed */
-    bdrv_refresh_filename(s->test_file->bs);
+    bdrv_filename(s->test_file->bs, s->test_file->bs->filename,
+                  sizeof(s->test_file->bs->filename));
 
     if (bs->file->bs->full_open_options
         && s->test_file->bs->full_open_options)
diff --git a/block/quorum.c b/block/quorum.c
index b9ba028..16ec188 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1005,7 +1005,8 @@ static void quorum_refresh_filename(BlockDriverState *bs)
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        bdrv_refresh_filename(s->children[i]->bs);
+        bdrv_filename(s->children[i]->bs, s->children[i]->bs->filename,
+                      sizeof(s->children[i]->bs->filename));
         if (!s->children[i]->bs->full_open_options) {
             return;
         }
diff --git a/include/block/block.h b/include/block/block.h
index 340c00f..9273e62 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -263,6 +263,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);
-- 
2.6.1

Re: [Qemu-devel] [PATCH v5 3/6] block: Add bdrv_filename()

[Eric Blake]

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

On 10/19/2015 12:49 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,
> some 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>
> ---
>  block.c               | 43 ++++++++++++++++++++++++++++++++++++-------
>  block/blkverify.c     |  3 ++-
>  block/quorum.c        |  3 ++-
>  include/block/block.h |  1 +
>  4 files changed, 41 insertions(+), 9 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 @sz > 0, the string put into the buffer will always be null-terminated.

Maybe add a warning:

...always be null-terminated, even if this truncates the result.

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 v5 3/6] block: Add bdrv_filename()

[Kevin Wolf]

Am 19.10.2015 um 20:49 hat Max Reitz geschrieben:
> 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,
> some 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v5 4/6] 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, it should be cached.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 3025776..a44c77a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2142,6 +2142,7 @@ typedef struct MapEntry {
     int64_t length;
     int64_t offset;
     BlockDriverState *bs;
+    char *bs_filename;
 } MapEntry;
 
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
@@ -2156,7 +2157,7 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == BDRV_BLOCK_DATA) {
             printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
-                   e->start, e->length, e->offset, e->bs->filename);
+                   e->start, e->length, e->offset, e->bs_filename);
         }
         /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
          * Modify the flags here to allow more coalescing.
@@ -2306,6 +2307,12 @@ static int img_map(int argc, char **argv)
             goto out;
         }
 
+        if (next.bs == curr.bs) {
+            next.bs_filename = curr.bs_filename;
+        } else {
+            next.bs_filename = bdrv_filename(next.bs, NULL, 0);
+        }
+
         if (curr.length != 0 && curr.flags == next.flags &&
             curr.depth == next.depth &&
             ((curr.flags & BDRV_BLOCK_OFFSET_VALID) == 0 ||
@@ -2317,10 +2324,15 @@ static int img_map(int argc, char **argv)
         if (curr.length > 0) {
             dump_map_entry(output_format, &curr, &next);
         }
+
+        if (curr.bs != next.bs) {
+            g_free(curr.bs_filename);
+        }
         curr = next;
     }
 
     dump_map_entry(output_format, &curr, NULL);
+    g_free(curr.bs_filename);
 
 out:
     blk_unref(blk);
-- 
2.6.1

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

[Eric Blake]

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

On 10/19/2015 12:49 PM, 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, it should be cached.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 

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 v5 4/6] qemu-img: Use bdrv_filename() for map

[Kevin Wolf]

Am 19.10.2015 um 20:49 hat Max Reitz geschrieben:
> Replaces bs->filename by the result of bdrv_filename() in the
> qemu-img map subcommand. Since that value is queried relatively often,
> however, it should be cached.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v5 5/6] 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>
---
 block.c                   | 46 ++++++++++++++++++++++++++++++----------------
 block/blkverify.c         |  3 +--
 block/commit.c            |  4 +++-
 block/mirror.c            | 16 ++++++++++++----
 block/qapi.c              |  4 ++--
 block/quorum.c            |  3 +--
 block/raw_bsd.c           |  4 +++-
 block/vhdx-log.c          |  5 ++++-
 block/vmdk.c              | 22 ++++++++++++++++------
 block/vpc.c               |  7 +++++--
 blockdev.c                | 25 +++++++++++++++++++------
 include/block/block_int.h |  1 -
 12 files changed, 96 insertions(+), 44 deletions(-)

diff --git a/block.c b/block.c
index 1061d80..a17e95f 100644
--- a/block.c
+++ b/block.c
@@ -226,10 +226,10 @@ void bdrv_get_full_backing_filename_from_filename(const char *backed,
 void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz,
                                     Error **errp)
 {
-    char *backed = bs->exact_filename[0] ? bs->exact_filename : bs->filename;
-
+    char *backed = bdrv_filename(bs, NULL, 0);
     bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file,
                                                  dest, sz, errp);
+    g_free(backed);
 }
 
 void bdrv_register(BlockDriver *bdrv)
@@ -817,6 +817,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     QDict *options, int flags, BlockDriver *drv, Error **errp)
 {
     int ret, open_flags;
+    char *filename_buffer = NULL;
     const char *filename;
     const char *node_name = NULL;
     QemuOpts *opts;
@@ -827,7 +828,8 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     assert(options != NULL && bs->options != options);
 
     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");
     }
@@ -835,7 +837,8 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     if (drv->bdrv_needs_filename && !filename) {
         error_setg(errp, "The '%s' block driver requires a file name",
                    drv->format_name);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto fail_filename;
     }
 
     trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
@@ -883,11 +886,10 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     }
 
     if (filename != NULL) {
-        pstrcpy(bs->filename, sizeof(bs->filename), filename);
+        pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), filename);
     } else {
-        bs->filename[0] = '\0';
+        bs->exact_filename[0] = '\0';
     }
-    pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->filename);
 
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
@@ -947,6 +949,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     assert((bs->request_alignment != 0) || bdrv_is_sg(bs));
 
     qemu_opts_del(opts);
+    g_free(filename_buffer);
     return 0;
 
 free_and_fail:
@@ -956,6 +959,8 @@ free_and_fail:
     bs->drv = NULL;
 fail_opts:
     qemu_opts_del(opts);
+fail_filename:
+    g_free(filename_buffer);
     return ret;
 }
 
@@ -1155,7 +1160,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
     }
     bs->backing = bdrv_attach_child(bs, backing_hd, &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 : "");
 
@@ -1527,7 +1532,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 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)) {
@@ -1807,8 +1812,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;
         }
@@ -2369,6 +2376,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) {
@@ -2395,7 +2403,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) {
@@ -2405,6 +2416,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
 
     ret = 0;
 exit:
+    g_free(base_filename);
     return ret;
 }
 
@@ -2818,7 +2830,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;
     }
@@ -2918,7 +2930,7 @@ int bdrv_is_snapshot(BlockDriverState *bs)
 }
 
 /* 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,
@@ -2953,18 +2965,20 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
         } else {
             /* If not an absolute filename path, make it relative to the current
              * image's filename path */
-            path_combine(filename_tmp, PATH_MAX, curr_bs->filename,
-                         backing_file);
+            char *curr_filename = bdrv_filename(curr_bs, NULL, 0);
+            path_combine(filename_tmp, PATH_MAX, curr_filename, backing_file);
 
             /* We are going to compare absolute pathnames */
             if (!realpath(filename_tmp, filename_full)) {
+                g_free(curr_filename);
                 continue;
             }
 
             /* We need to make sure the backing filename we are comparing against
              * is relative to the current image filename (or absolute) */
-            path_combine(filename_tmp, PATH_MAX, curr_bs->filename,
+            path_combine(filename_tmp, PATH_MAX, curr_filename,
                          curr_bs->backing_file);
+            g_free(curr_filename);
 
             if (!realpath(filename_tmp, backing_file_full)) {
                 continue;
diff --git a/block/blkverify.c b/block/blkverify.c
index 3af37f8..c5f8e8d 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -312,8 +312,7 @@ static void blkverify_refresh_filename(BlockDriverState *bs)
     BDRVBlkverifyState *s = bs->opaque;
 
     /* bs->file->bs has already been refreshed */
-    bdrv_filename(s->test_file->bs, s->test_file->bs->filename,
-                  sizeof(s->test_file->bs->filename));
+    bdrv_refresh_filename(s->test_file->bs);
 
     if (bs->file->bs->full_open_options
         && s->test_file->bs->full_open_options)
diff --git a/block/commit.c b/block/commit.c
index fdebe87..3055ce1 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -228,7 +228,9 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
     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 b1252a1..4554d36 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -797,25 +797,33 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
 
     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/qapi.c b/block/qapi.c
index ec0f513..f43436b 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -38,7 +38,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp)
     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;
@@ -218,7 +218,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/quorum.c b/block/quorum.c
index 16ec188..b9ba028 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1005,8 +1005,7 @@ static void quorum_refresh_filename(BlockDriverState *bs)
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        bdrv_filename(s->children[i]->bs, s->children[i]->bs->filename,
-                      sizeof(s->children[i]->bs->filename));
+        bdrv_refresh_filename(s->children[i]->bs);
         if (!s->children[i]->bs->full_open_options) {
             return;
         }
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 0aded31..df01006 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -205,6 +205,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     bs->sg = bs->file->bs->sg;
 
     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"
@@ -212,7 +213,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);
     }
 
     return 0;
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 47ae4b1..c98b4d8 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -783,13 +783,16 @@ 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_errno(errp, EPERM,
                              "VHDX image file '%s' opened read-only, but "
                              "contains a log that needs to be replayed.  To "
                              "replay the log, execute:\n qemu-img check -r "
                              "all '%s'",
-                             bs->filename, bs->filename);
+                             filename, filename);
+            g_free(filename);
             goto exit;
         }
         /* now flush the log */
diff --git a/block/vmdk.c b/block/vmdk.c
index 0effb7d..3a6e660 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -445,9 +445,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++) {
@@ -465,9 +467,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++) {
@@ -496,9 +500,11 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
 
     ret = bdrv_pread(file->bs, 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,
@@ -573,9 +579,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
 
     ret = bdrv_pread(file->bs, 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) {
@@ -819,8 +827,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;
         }
 
@@ -2092,7 +2102,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 299d373..a3c4c17 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -204,9 +204,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 b79b0a6..5887e09 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -229,10 +229,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;
         }
     }
@@ -1596,10 +1599,12 @@ static void external_snapshot_prepare(BlkTransactionState *common,
 
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
+        char *old_filename = bdrv_filename(state->old_bs, NULL, 0);
         bdrv_img_create(new_image_file, format,
-                        state->old_bs->filename,
+                        old_filename,
                         state->old_bs->drv->format_name,
                         NULL, -1, flags, &local_err, false);
+        g_free(old_filename);
         if (local_err) {
             error_propagate(errp, local_err);
             return;
@@ -2650,9 +2655,11 @@ void qmp_block_commit(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) {
@@ -2791,9 +2798,11 @@ void qmp_drive_backup(const char *device, const char *target,
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         assert(format);
         if (source) {
-            bdrv_img_create(target, format, source->filename,
+            char *source_filename = bdrv_filename(source, NULL, 0);
+            bdrv_img_create(target, format, source_filename,
                             source->drv->format_name, NULL,
                             size, flags, &local_err, false);
+            g_free(source_filename);
         } else {
             bdrv_img_create(target, format, NULL, NULL, NULL,
                             size, flags, &local_err, false);
@@ -3043,15 +3052,19 @@ void qmp_drive_mirror(const char *device, const char *target,
         bdrv_img_create(target, format,
                         NULL, NULL, NULL, size, flags, &local_err, false);
     } else {
+        char *source_filename;
+
         switch (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(target, format,
-                            source->filename,
+                            source_filename,
                             source->drv->format_name,
                             NULL, size, flags, &local_err, false);
+            g_free(source_filename);
             break;
         default:
             abort();
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e472a03..65dd5bd 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -371,7 +371,6 @@ struct BlockDriverState {
      * regarding this BDS's context */
     QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
 
-    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 */
-- 
2.6.1

Re: [Qemu-devel] [PATCH v5 5/6] block: Drop BlockDriverState.filename

[Eric Blake]

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

On 10/19/2015 12:49 PM, Max Reitz wrote:
> 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>
> ---
>  block.c                   | 46 ++++++++++++++++++++++++++++++----------------
>  block/blkverify.c         |  3 +--
>  block/commit.c            |  4 +++-
>  block/mirror.c            | 16 ++++++++++++----
>  block/qapi.c              |  4 ++--
>  block/quorum.c            |  3 +--
>  block/raw_bsd.c           |  4 +++-
>  block/vhdx-log.c          |  5 ++++-
>  block/vmdk.c              | 22 ++++++++++++++++------
>  block/vpc.c               |  7 +++++--
>  blockdev.c                | 25 +++++++++++++++++++------
>  include/block/block_int.h |  1 -
>  12 files changed, 96 insertions(+), 44 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 v5 5/6] block: Drop BlockDriverState.filename

[Kevin Wolf]

Am 19.10.2015 um 20:49 hat Max Reitz geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH v5 6/6] 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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 05b5962..6703882 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
@@ -920,10 +921,18 @@ class TestRepairQuorum(iotests.QMPTestCase):
         self.assert_qmp(result, 'return', {})
 
         self.complete_and_wait(drive="quorum0")
-        result = self.vm.qmp('query-named-block-nodes')
-        self.assert_qmp(result, 'return[0]/file', 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.6.1

Re: [Qemu-devel] [PATCH v5 6/6] iotests: Test changed Quorum filename

[Eric Blake]

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

On 10/19/2015 12:49 PM, Max Reitz wrote:
> 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 | 17 +++++++++++++----
>  1 file changed, 13 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 --]

Re: [Qemu-devel] [PATCH v5 6/6] iotests: Test changed Quorum filename

[Kevin Wolf]

Am 19.10.2015 um 20:49 hat Max Reitz geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>