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

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

[Max Reitz]

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.


Max Reitz (4):
  block: Change bdrv_get_encrypted_filename()
  block: Avoid BlockDriverState.filename
  block: Add bdrv_filename()
  block: Drop BlockDriverState.filename

 block.c                   | 90 ++++++++++++++++++++++++++++++++---------------
 block/commit.c            |  4 ++-
 block/gluster.c           |  2 +-
 block/mirror.c            | 14 +++++---
 block/qapi.c              |  7 ++--
 block/raw-posix.c         |  8 ++---
 block/raw-win32.c         |  4 +--
 block/vhdx-log.c          |  5 ++-
 block/vmdk.c              | 22 ++++++++----
 block/vpc.c               |  6 ++--
 blockdev.c                | 21 ++++++++---
 include/block/block.h     |  3 +-
 include/block/block_int.h |  1 -
 monitor.c                 |  7 ++--
 qemu-img.c                |  2 +-
 qmp.c                     |  4 ++-
 16 files changed, 136 insertions(+), 64 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/4] block: Change bdrv_get_encrypted_filename()

[Max Reitz]

Instead of returning a pointer to the filename, copy it into a buffer
specified by the caller. While adapting the callers accordingly, this
patch also fixes them apparently ignoring that this function's return
value may be NULL.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 20 ++++++++++++++------
 blockdev.c            |  4 +++-
 include/block/block.h |  2 +-
 monitor.c             |  7 +++++--
 qmp.c                 |  4 +++-
 5 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index a2fa1e1..4cf7620 100644
--- a/block.c
+++ b/block.c
@@ -4185,14 +4185,22 @@ int bdrv_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
-const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
+char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz)
 {
-    if (bs->backing_hd && bs->backing_hd->encrypted)
-        return bs->backing_file;
-    else if (bs->encrypted)
-        return bs->filename;
-    else
+    if (sz > INT_MAX) {
+        sz = INT_MAX;
+    }
+
+    if (bs->backing_hd && bs->backing_hd->encrypted) {
+        pstrcpy(dest, sz, bs->backing_file);
+        return dest;
+    } else if (bs->encrypted) {
+        pstrcpy(dest, sz, bs->filename);
+        return dest;
+    } else {
+        dest[0] = '\0';
         return NULL;
+    }
 }
 
 void bdrv_get_backing_filename(BlockDriverState *bs,
diff --git a/blockdev.c b/blockdev.c
index 450f95c..aed0519 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1616,8 +1616,10 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
                 error_set(errp, QERR_INVALID_PASSWORD);
             }
         } else {
+            char enc_filename[PATH_MAX];
+            bdrv_get_encrypted_filename(bs, enc_filename, sizeof(enc_filename));
             error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
-                      bdrv_get_encrypted_filename(bs));
+                      enc_filename);
         }
     } else if (password) {
         error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
diff --git a/include/block/block.h b/include/block/block.h
index 3318f0d..c6e82f3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -429,7 +429,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, char *dest, size_t sz);
 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 667efb7..217d91e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5382,6 +5382,7 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
                                 BlockDriverCompletionFunc *completion_cb,
                                 void *opaque)
 {
+    char enc_filename[PATH_MAX];
     int err;
 
     if (!bdrv_key_required(bs)) {
@@ -5390,14 +5391,16 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
         return 0;
     }
 
+    bdrv_get_encrypted_filename(bs, enc_filename, sizeof(enc_filename));
+
     if (monitor_ctrl_mode(mon)) {
         qerror_report(QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
-                      bdrv_get_encrypted_filename(bs));
+                      enc_filename);
         return -1;
     }
 
     monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
-                   bdrv_get_encrypted_filename(bs));
+                   enc_filename);
 
     mon->password_completion_cb = completion_cb;
     mon->password_opaque = opaque;
diff --git a/qmp.c b/qmp.c
index c6767c4..f8ca3d1 100644
--- a/qmp.c
+++ b/qmp.c
@@ -164,9 +164,11 @@ void qmp_cont(Error **errp)
     }
     for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
         if (bdrv_key_required(bs)) {
+            char enc_filename[PATH_MAX];
+            bdrv_get_encrypted_filename(bs, enc_filename, sizeof(enc_filename));
             error_set(errp, QERR_DEVICE_ENCRYPTED,
                       bdrv_get_device_name(bs),
-                      bdrv_get_encrypted_filename(bs));
+                      enc_filename);
             return;
         }
     }
-- 
2.1.0

[Qemu-devel] [PATCH 2/4] 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 qemu-img's map command, we should be using the filename field; but
since this commit prepares to remove that field, using exact_filename is
fine, too (this is the only user of BlockDriverState.filename which
frequently queries that field).

Finally, in some other places there a buffer is available whose content
equals that of BlockDriverState.filename; use that buffer instead.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c           | 4 ++--
 block/gluster.c   | 2 +-
 block/qapi.c      | 2 +-
 block/raw-posix.c | 8 ++++----
 block/raw-win32.c | 4 ++--
 qemu-img.c        | 2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 4cf7620..9311cef 100644
--- a/block.c
+++ b/block.c
@@ -992,8 +992,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *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/qapi.c b/block/qapi.c
index 9733ebd..682109e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -213,7 +213,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
 
 #ifdef __linux__
     /* get NOCOW info */
-    fd = qemu_open(bs->filename, O_RDONLY | O_NONBLOCK);
+    fd = qemu_open(info->filename, O_RDONLY | O_NONBLOCK);
     if (fd >= 0) {
         if (ioctl(fd, FS_IOC_GETFLAGS, &attr) == 0 && (attr & FS_NOCOW_FL)) {
             info->has_nocow = true;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index a253697..3bee169 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -578,7 +578,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) {
         assert(!(raw_s->open_flags & O_CREAT));
-        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags);
+        raw_s->fd = qemu_open(state->bs->exact_filename, raw_s->open_flags);
         if (raw_s->fd == -1) {
             error_setg_errno(errp, errno, "Could not reopen file");
             ret = -1;
@@ -1896,7 +1896,7 @@ static int fd_open(BlockDriverState *bs)
 #endif
             return -EIO;
         }
-        s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
+        s->fd = qemu_open(bs->exact_filename, s->open_flags & ~O_NONBLOCK);
         if (s->fd < 0) {
             s->fd_error_time = get_clock();
             s->fd_got_error = 1;
@@ -2188,7 +2188,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag)
         qemu_close(s->fd);
         s->fd = -1;
     }
-    fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK);
+    fd = qemu_open(bs->exact_filename, s->open_flags | O_NONBLOCK);
     if (fd >= 0) {
         if (ioctl(fd, FDEJECT, 0) < 0)
             perror("FDEJECT");
@@ -2412,7 +2412,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 9bf8225..9afc4d1 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -416,7 +416,7 @@ static void raw_close(BlockDriverState *bs)
 
     CloseHandle(s->hfile);
     if (bs->open_flags & BDRV_O_TEMPORARY) {
-        unlink(bs->filename);
+        unlink(bs->exact_filename);
     }
 }
 
@@ -484,7 +484,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/qemu-img.c b/qemu-img.c
index dbf0904..ec42f21 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2009,7 +2009,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->exact_filename);
         }
         /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
          * Modify the flags here to allow more coalescing.
-- 
2.1.0

[Qemu-devel] [PATCH 3/4] 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 into 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.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 31 ++++++++++++++++++++++++-------
 block/blkverify.c     |  3 ++-
 block/quorum.c        |  2 +-
 include/block/block.h |  1 +
 4 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 9311cef..955ea3d 100644
--- a/block.c
+++ b/block.c
@@ -1508,7 +1508,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         }
     }
 
-    bdrv_refresh_filename(bs);
+    bdrv_filename(bs, bs->filename, sizeof(bs->filename));
 
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
@@ -6000,9 +6000,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)
 {
@@ -6089,15 +6086,35 @@ 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.
+ *
+ * Returns @dest.
+ */
+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);
+        pstrcpy(dest, sz, 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));
+        snprintf(dest, sz, "json:%s", qstring_get_str(json));
         QDECREF(json);
     }
+
+    return dest;
 }
 
 /* This accessor function purpose is to allow the device models to access the
diff --git a/block/blkverify.c b/block/blkverify.c
index 7d64a23..15896c0 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -309,7 +309,8 @@ static void blkverify_refresh_filename(BlockDriverState *bs)
     BDRVBlkverifyState *s = bs->opaque;
 
     /* bs->file has already been refreshed */
-    bdrv_refresh_filename(s->test_file);
+    bdrv_filename(s->test_file, s->test_file->filename,
+                  sizeof(s->test_file->filename));
 
     if (bs->file->full_open_options && s->test_file->full_open_options) {
         QDict *opts = qdict_new();
diff --git a/block/quorum.c b/block/quorum.c
index 7687466..fb0b921 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1039,7 +1039,7 @@ static void quorum_refresh_filename(BlockDriverState *bs)
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        bdrv_refresh_filename(s->bs[i]);
+        bdrv_filename(s->bs[i], s->bs[i]->filename, sizeof(s->bs[i]->filename));
         if (!s->bs[i]->full_open_options) {
             return;
         }
diff --git a/include/block/block.h b/include/block/block.h
index c6e82f3..3507c4e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -276,6 +276,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.1.0

[Qemu-devel] [PATCH 4/4] 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() on a
  newly allocated heap buffer
- bdrv_filename(bs, bs->filename, sizeof(bs->filename)) is replaced by
  bdrv_refresh_filename(bs)
- anywhere else a buffer is created on the stack to hold the result of
  bdrv_filename(); any access to BlockDriverState.filename is then
  replaced by this buffer

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   | 43 ++++++++++++++++++++++++++-----------------
 block/blkverify.c         |  3 +--
 block/commit.c            |  4 +++-
 block/mirror.c            | 14 ++++++++++----
 block/qapi.c              |  5 +++--
 block/quorum.c            |  2 +-
 block/vhdx-log.c          |  5 ++++-
 block/vmdk.c              | 22 +++++++++++++++-------
 block/vpc.c               |  6 ++++--
 blockdev.c                | 17 +++++++++++++----
 include/block/block_int.h |  1 -
 11 files changed, 80 insertions(+), 42 deletions(-)

diff --git a/block.c b/block.c
index 955ea3d..7421df3 100644
--- a/block.c
+++ b/block.c
@@ -311,7 +311,9 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs, char *dest, size_t sz)
     if (bs->backing_file[0] == '\0' || path_has_protocol(bs->backing_file)) {
         pstrcpy(dest, sz, bs->backing_file);
     } else {
-        path_combine(dest, sz, bs->filename, bs->backing_file);
+        char filename[PATH_MAX];
+        bdrv_filename(bs, filename, sizeof(filename));
+        path_combine(dest, sz, filename, bs->backing_file);
     }
 }
 
@@ -896,6 +898,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     QDict *options, int flags, BlockDriver *drv, Error **errp)
 {
     int ret, open_flags;
+    char filename_buffer[PATH_MAX];
     const char *filename;
     const char *node_name = NULL;
     Error *local_err = NULL;
@@ -905,7 +908,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     assert(options != NULL && bs->options != options);
 
     if (file != NULL) {
-        filename = file->filename;
+        filename = bdrv_filename(file,
+                                 filename_buffer, sizeof(filename_buffer));
     } else {
         filename = qdict_get_try_str(options, "filename");
     }
@@ -962,11 +966,10 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *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);
@@ -1164,7 +1167,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd)
         goto out;
     }
     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 : "");
 
@@ -1508,7 +1511,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
         }
     }
 
-    bdrv_filename(bs, bs->filename, sizeof(bs->filename));
+    bdrv_refresh_filename(bs);
 
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
@@ -1753,8 +1756,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
             if (local_err != NULL) {
                 error_propagate(errp, local_err);
             } else {
+                char filename[PATH_MAX];
+                bdrv_filename(reopen_state->bs, filename, sizeof(filename));
                 error_setg(errp, "failed while preparing to reopen image '%s'",
-                           reopen_state->bs->filename);
+                           filename);
             }
             goto error;
         }
@@ -2260,8 +2265,7 @@ int bdrv_commit(BlockDriverState *bs)
     }
 
     ro = bs->backing_hd->read_only;
-    /* Use pstrcpy (not strncpy): filename must be NUL-terminated. */
-    pstrcpy(filename, sizeof(filename), bs->backing_hd->filename);
+    bdrv_filename(bs->backing_hd, filename, sizeof(filename));
     open_flags =  bs->backing_hd->open_flags;
 
     if (ro) {
@@ -2614,6 +2618,7 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top,
     BlockDriverState *base_bs = NULL;
     BlockDriverState *new_top_bs = NULL;
     BlkIntermediateStates *intermediate_state, *next;
+    char base_filename[PATH_MAX];
     int ret = -EIO;
 
     QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
@@ -2660,7 +2665,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_bs->filename;
+    if (!backing_file_str) {
+        bdrv_filename(base_bs, base_filename, sizeof(base_filename));
+        backing_file_str = base_filename;
+    }
     ret = bdrv_change_backing_file(new_top_bs, backing_file_str,
                                    base_bs->drv ? base_bs->drv->format_name : "");
     if (ret) {
@@ -4195,8 +4203,7 @@ char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz)
         pstrcpy(dest, sz, bs->backing_file);
         return dest;
     } else if (bs->encrypted) {
-        pstrcpy(dest, sz, bs->filename);
-        return dest;
+        return bdrv_filename(bs, dest, sz);
     } else {
         dest[0] = '\0';
         return NULL;
@@ -4354,7 +4361,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,
@@ -4387,10 +4394,12 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
                 break;
             }
         } else {
+            char curr_filename[PATH_MAX];
+
             /* 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);
+            bdrv_filename(curr_bs, curr_filename, sizeof(curr_filename));
+            path_combine(filename_tmp, PATH_MAX, curr_filename, backing_file);
 
             /* We are going to compare absolute pathnames */
             if (!realpath(filename_tmp, filename_full)) {
@@ -4399,7 +4408,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
 
             /* 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);
 
             if (!realpath(filename_tmp, backing_file_full)) {
diff --git a/block/blkverify.c b/block/blkverify.c
index 15896c0..7d64a23 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -309,8 +309,7 @@ static void blkverify_refresh_filename(BlockDriverState *bs)
     BDRVBlkverifyState *s = bs->opaque;
 
     /* bs->file has already been refreshed */
-    bdrv_filename(s->test_file, s->test_file->filename,
-                  sizeof(s->test_file->filename));
+    bdrv_refresh_filename(s->test_file);
 
     if (bs->file->full_open_options && s->test_file->full_open_options) {
         QDict *opts = qdict_new();
diff --git a/block/commit.c b/block/commit.c
index 91517d3..dbf4ac8 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -208,7 +208,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[PATH_MAX];
+        bdrv_filename(top, top_filename, sizeof(top_filename));
+        error_setg(errp, "Could not find overlay image for %s:", top_filename);
         return;
     }
 
diff --git a/block/mirror.c b/block/mirror.c
index 18b18e0..d056c7d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -705,25 +705,31 @@ 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[PATH_MAX];
+        error_setg_errno(errp, -length, "Unable to determine length of %s",
+                         bdrv_filename(bs, filename, sizeof(filename)));
         goto error_restore_flags;
     }
 
     base_length = bdrv_getlength(base);
     if (base_length < 0) {
+        char base_filename[PATH_MAX];
+        bdrv_filename(base, base_filename, sizeof(base_filename));
         error_setg_errno(errp, -base_length,
-                         "Unable to determine length of %s", base->filename);
+                         "Unable to determine length of %s", base_filename);
         goto error_restore_flags;
     }
 
     if (length > base_length) {
         ret = bdrv_truncate(base, length);
         if (ret < 0) {
+            char filename[PATH_MAX], base_filename[PATH_MAX];
+            bdrv_filename(bs, filename, sizeof(filename));
+            bdrv_filename(base, base_filename, sizeof(base_filename));
             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);
             goto error_restore_flags;
         }
     }
diff --git a/block/qapi.c b/block/qapi.c
index 682109e..5bcc561 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -40,7 +40,8 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
 {
     BlockDeviceInfo *info = g_malloc0(sizeof(*info));
 
-    info->file                   = g_strdup(bs->filename);
+    info->file                   = bdrv_filename(bs, g_malloc(PATH_MAX),
+                                                 PATH_MAX);
     info->ro                     = bs->read_only;
     info->drv                    = g_strdup(bs->drv->format_name);
     info->encrypted              = bs->encrypted;
@@ -191,7 +192,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
     }
 
     info = g_new0(ImageInfo, 1);
-    info->filename        = g_strdup(bs->filename);
+    info->filename        = bdrv_filename(bs, g_malloc(PATH_MAX), PATH_MAX);
     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 fb0b921..7687466 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1039,7 +1039,7 @@ static void quorum_refresh_filename(BlockDriverState *bs)
     int i;
 
     for (i = 0; i < s->num_children; i++) {
-        bdrv_filename(s->bs[i], s->bs[i]->filename, sizeof(s->bs[i]->filename));
+        bdrv_refresh_filename(s->bs[i]);
         if (!s->bs[i]->full_open_options) {
             return;
         }
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 6547bec..0f487a7 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -781,13 +781,16 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
 
     if (logs.valid) {
         if (bs->read_only) {
+            char filename[PATH_MAX];
+            bdrv_filename(bs, filename, sizeof(filename));
+
             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);
             goto exit;
         }
         /* now flush the log */
diff --git a/block/vmdk.c b/block/vmdk.c
index afdea1a..58d4728 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -464,9 +464,11 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
                      extent->l1_table,
                      l1_size);
     if (ret < 0) {
+        char extent_filename[PATH_MAX];
+        bdrv_filename(extent->file, extent_filename, sizeof(extent_filename));
         error_setg_errno(errp, -ret,
                          "Could not read l1 table from extent '%s'",
-                         extent->file->filename);
+                         extent_filename);
         goto fail_l1;
     }
     for (i = 0; i < extent->l1_size; i++) {
@@ -484,9 +486,11 @@ static int vmdk_init_tables(BlockDriverState *bs, VmdkExtent *extent,
                          extent->l1_backup_table,
                          l1_size);
         if (ret < 0) {
+            char extent_filename[PATH_MAX];
+            bdrv_filename(extent->file, extent_filename, sizeof(extent_filename));
             error_setg_errno(errp, -ret,
                              "Could not read l1 backup table from extent '%s'",
-                             extent->file->filename);
+                             extent_filename);
             goto fail_l1b;
         }
         for (i = 0; i < extent->l1_size; i++) {
@@ -515,9 +519,10 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs,
 
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
+        char filename[PATH_MAX];
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
-                         file->filename);
+                         bdrv_filename(file, filename, sizeof(filename)));
         return ret;
     }
     ret = vmdk_add_extent(bs, file, false,
@@ -583,9 +588,10 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
 
     ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header));
     if (ret < 0) {
+        char filename[PATH_MAX];
         error_setg_errno(errp, -ret,
                          "Could not read header from file '%s'",
-                         file->filename);
+                         bdrv_filename(file, filename, sizeof(filename)));
         return -EINVAL;
     }
     if (header.capacity == 0) {
@@ -875,7 +881,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
                                Error **errp)
 {
     int ret;
-    char ct[128];
+    char ct[128], filename[PATH_MAX];
     BDRVVmdkState *s = bs->opaque;
 
     if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) {
@@ -894,7 +900,8 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
     }
     s->create_type = g_strdup(ct);
     s->desc_offset = 0;
-    ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
+    bdrv_filename(bs->file, filename, sizeof(filename));
+    ret = vmdk_parse_extents(buf, bs, filename, errp);
 exit:
     return ret;
 }
@@ -2047,7 +2054,8 @@ static ImageInfo *vmdk_get_extent_info(VmdkExtent *extent)
     ImageInfo *info = g_new0(ImageInfo, 1);
 
     *info = (ImageInfo){
-        .filename         = g_strdup(extent->file->filename),
+        .filename         = bdrv_filename(extent->file, g_malloc(PATH_MAX),
+                                          PATH_MAX),
         .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 4947369..80258ff 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -202,9 +202,11 @@ 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[PATH_MAX];
         fprintf(stderr, "block-vpc: The header checksum of '%s' is "
-            "incorrect.\n", bs->filename);
+                "incorrect.\n", bdrv_filename(bs, filename, sizeof(filename)));
+    }
 
     /* Write 'checksum' back to footer, or else will leave it with zero. */
     footer->checksum = be32_to_cpu(checksum);
diff --git a/blockdev.c b/blockdev.c
index aed0519..8a18af7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1335,8 +1335,10 @@ static void external_snapshot_prepare(BlkTransactionState *common,
 
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
+        char old_filename[PATH_MAX];
+        bdrv_filename(state->old_bs, old_filename, sizeof(old_filename));
         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);
         if (local_err) {
@@ -1980,7 +1982,9 @@ void qmp_block_commit(const char *device,
     top_bs = bs;
 
     if (has_top && top) {
-        if (strcmp(bs->filename, top) != 0) {
+        char filename[PATH_MAX];
+        bdrv_filename(bs, filename, sizeof(filename));
+        if (strcmp(filename, top) != 0) {
             top_bs = bdrv_find_backing_image(bs, top);
         }
     }
@@ -2105,7 +2109,9 @@ void qmp_drive_backup(const char *device, const char *target,
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         assert(format && drv);
         if (source) {
-            bdrv_img_create(target, format, source->filename,
+            char source_filename[PATH_MAX];
+            bdrv_filename(source, source_filename, sizeof(source_filename));
+            bdrv_img_create(target, format, source_filename,
                             source->drv->format_name, NULL,
                             size, flags, &local_err, false);
         } else {
@@ -2265,13 +2271,16 @@ 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[PATH_MAX];
+
         switch (mode) {
         case NEW_IMAGE_MODE_EXISTING:
             break;
         case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
             /* create new image with backing file */
+            bdrv_filename(source, source_filename, sizeof(source_filename));
             bdrv_img_create(target, format,
-                            source->filename,
+                            source_filename,
                             source->drv->format_name,
                             NULL, size, flags, &local_err, false);
             break;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d86a6c..df7bde2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -335,7 +335,6 @@ struct BlockDriverState {
      * regarding this BDS's context */
     QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
 
-    char filename[1024];
     char backing_file[1024]; /* if non zero, the image is a diff of
                                 this file image */
     char backing_format[16]; /* if non-zero and backing_file exists */
-- 
2.1.0

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

[Kevin Wolf]

Am 24.09.2014 um 21:48 hat Max Reitz geschrieben:
> 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.

Surprisingly (or not), this one needs rebasing.

I tried it and it doesn't look too hard, but it's a little bit more than
what I'm comfortable with doing while applying a series.

Kevin

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

[Max Reitz]

On 2015-02-03 at 04:32, Kevin Wolf wrote:
> Am 24.09.2014 um 21:48 hat Max Reitz geschrieben:
>> 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.
> Surprisingly (or not), this one needs rebasing.

Well...

> I tried it and it doesn't look too hard, but it's a little bit more than
> what I'm comfortable with doing while applying a series.

I admire your courage, but I'm not sure whether this series is ready for 
being applied at all. First we (or I) will have to look into how users 
like libvirt which identify a BDS based on the filename can break from 
applying this series.

Max

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

[Kevin Wolf]

Am 03.02.2015 um 14:48 hat Max Reitz geschrieben:
> On 2015-02-03 at 04:32, Kevin Wolf wrote:
> >Am 24.09.2014 um 21:48 hat Max Reitz geschrieben:
> >>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.
> >Surprisingly (or not), this one needs rebasing.
> 
> Well...
> 
> >I tried it and it doesn't look too hard, but it's a little bit more than
> >what I'm comfortable with doing while applying a series.
> 
> I admire your courage, but I'm not sure whether this series is ready
> for being applied at all. First we (or I) will have to look into how
> users like libvirt which identify a BDS based on the filename can
> break from applying this series.

Well, I haven't reviewed it, so I can't tell. It didn't have a
(Self-)NACK and it's still on your list of to-be-merged patches, so I
took a look.  You're talking about courage - but I just wasn't
courageous enough yet to attack your larger series... ;-)

Kevin