Re: [Qemu-block] [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 

Reviewed-by: Kevin Wolf 

[Qemu-block] [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 
---
 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, _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,
 }
 }
 
-