[Qemu-devel] [PATCH v2 4/5] 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)
  (these were introduced by the previous patch)
- 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 <address@hidden>
---
 block.c                   | 40 ++++++++++++++++++++++++----------------
 block/blkverify.c         |  3 +--
 block/commit.c            |  4 +++-
 block/mirror.c            | 14 ++++++++++----
 block/qapi.c              |  5 +++--
 block/quorum.c            |  2 +-
 block/raw-posix.c         |  8 ++++++--
 block/raw_bsd.c           |  4 +++-
 block/vhdx-log.c          |  5 ++++-
 block/vmdk.c              | 21 +++++++++++++++------
 block/vpc.c               |  6 ++++--
 blockdev.c                | 21 ++++++++++++++++-----
 include/block/block_int.h |  1 -
 13 files changed, 90 insertions(+), 44 deletions(-)

diff --git a/block.c b/block.c
index cbc53e2..30ae243 100644
--- a/block.c
+++ b/block.c
@@ -227,8 +227,8 @@ 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[PATH_MAX];
+    bdrv_filename(bs, backed, sizeof(backed));
     bdrv_get_full_backing_filename_from_filename(backed, bs->backing_file,
                                                  dest, sz, errp);
 }
@@ -821,6 +821,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;
     QemuOpts *opts;
@@ -831,7 +832,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");
     }
@@ -888,11 +890,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);
@@ -1157,7 +1158,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd)
     }
     bs->backing_child = 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 : "");
 
@@ -1558,7 +1559,7 @@ static int bdrv_open_inherit(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. */
@@ -1822,8 +1823,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;
         }
@@ -2440,6 +2443,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;
@@ -2486,7 +2490,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) {
@@ -2992,8 +2999,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;
@@ -3094,7 +3100,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,
@@ -3127,10 +3133,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)) {
@@ -3139,7 +3147,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 cb9ce02..d277e63 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 7312a5b..6d387df 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -227,7 +227,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 fc4d8f5..61fac1d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -781,25 +781,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 2ce5097..dc58135 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -38,7 +38,8 @@ 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, g_malloc(PATH_MAX),
+                                                 PATH_MAX);
     info->ro                     = bs->read_only;
     info->drv                    = g_strdup(bs->drv->format_name);
     info->encrypted              = bs->encrypted;
@@ -218,7 +219,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 00d1fb0..2f6c45f 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -1003,7 +1003,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/raw-posix.c b/block/raw-posix.c
index b61c11f..ef56915 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -512,12 +512,14 @@ static int raw_open_common(BlockDriverState *bs, QDict 
*options,
         goto fail;
     }
     if (!s->use_aio && (bdrv_flags & BDRV_O_NATIVE_AIO)) {
+        char filename[PATH_MAX];
+        bdrv_filename(bs, filename, sizeof(filename));
         error_printf("WARNING: aio=native was specified for '%s', but "
                      "it requires cache.direct=on, which was not "
                      "specified. Falling back to aio=threads.\n"
                      "         This will become an error condition in "
                      "future QEMU versions.\n",
-                     bs->filename);
+                     filename);
     }
 #endif
 
@@ -2095,8 +2097,10 @@ static bool hdev_is_sg(BlockDriverState *bs)
     struct stat st;
     struct sg_scsi_id scsiid;
     int sg_version;
+    char filename[PATH_MAX];
 
-    if (stat(bs->filename, &st) >= 0 && S_ISCHR(st.st_mode) &&
+    bdrv_filename(bs, filename, sizeof(filename));
+    if (stat(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",
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index e3d2d04..98b8a12 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -210,6 +210,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, 
int flags,
     bs->sg = bs->file->sg;
 
     if (bs->probed && !bdrv_is_read_only(bs)) {
+        char filename[PATH_MAX];
+        bdrv_filename(bs->file, filename, sizeof(filename));
         fprintf(stderr,
                 "WARNING: Image format was not specified for '%s' and probing "
                 "guessed raw.\n"
@@ -217,7 +219,7 @@ 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->filename);
+                filename);
     }
 
     return 0;
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 47fec63..1f4f57d 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -782,13 +782,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 fbaab67..cdc2bb3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -444,9 +444,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++) {
@@ -464,9 +466,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++) {
@@ -495,9 +499,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,
@@ -572,9 +577,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) {
@@ -818,8 +824,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[PATH_MAX];
+            bdrv_filename(bs->file, filename, sizeof(filename));
             error_setg(errp, "Cannot use relative extent paths with VMDK "
-                       "descriptor file '%s'", bs->file->filename);
+                       "descriptor file '%s'", filename);
             return -EINVAL;
         }
 
@@ -2086,7 +2094,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 3e385d9..581c997 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -204,9 +204,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 = cpu_to_be32(checksum);
diff --git a/blockdev.c b/blockdev.c
index 62a4586..21befcf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -227,9 +227,11 @@ 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[PATH_MAX];
+            bdrv_filename(blk_bs(blk), filename, sizeof(filename));
             fprintf(stderr, "Warning: Orphaned drive without device: "
                     "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
-                    blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
+                    blk_name(blk), filename, if_name[dinfo->type],
                     dinfo->bus, dinfo->unit);
             rs = true;
         }
@@ -1507,8 +1509,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) {
@@ -2388,7 +2392,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);
         }
     }
@@ -2536,7 +2542,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 {
@@ -2781,13 +2789,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 14ad4c3..677c05a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -370,7 +370,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.4.6