Re: [Qemu-block] [PATCH v8 21/26] block: Purify .bdrv_refresh_filename()

[Alberto Garcia]

On Mon 05 Feb 2018 04:18:30 PM CET, Max Reitz wrote:
> Currently, BlockDriver.bdrv_refresh_filename() is supposed to both
> refresh the filename (BDS.exact_filename) and set BDS.full_open_options.
> Now that we have generic code in the central bdrv_refresh_filename() for
> creating BDS.full_open_options, we can drop the latter part from all
> BlockDriver.bdrv_refresh_filename() implementations.
>
> This also means that we can drop all of the existing default code for
> this from the global bdrv_refresh_filename() itself.
>
> Furthermore, we now have to call BlockDriver.bdrv_refresh_filename()
> after having set BDS.full_open_options, because the block driver's
> implementation should now be allowed to depend on BDS.full_open_options
> being set correctly.
>
> Finally, with this patch we can drop the @options parameter from
> BlockDriver.bdrv_refresh_filename(); also, add a comment on this
> function's purpose in block/block_int.h while touching its interface.
>
> Signed-off-by: Max Reitz 

Reviewed-by: Alberto Garcia 

Berto

[Qemu-block] [PATCH v8 21/26] block: Purify .bdrv_refresh_filename()

[Max Reitz]

Currently, BlockDriver.bdrv_refresh_filename() is supposed to both
refresh the filename (BDS.exact_filename) and set BDS.full_open_options.
Now that we have generic code in the central bdrv_refresh_filename() for
creating BDS.full_open_options, we can drop the latter part from all
BlockDriver.bdrv_refresh_filename() implementations.

This also means that we can drop all of the existing default code for
this from the global bdrv_refresh_filename() itself.

Furthermore, we now have to call BlockDriver.bdrv_refresh_filename()
after having set BDS.full_open_options, because the block driver's
implementation should now be allowed to depend on BDS.full_open_options
being set correctly.

Finally, with this patch we can drop the @options parameter from
BlockDriver.bdrv_refresh_filename(); also, add a comment on this
function's purpose in block/block_int.h while touching its interface.

Signed-off-by: Max Reitz 
---
 include/block/block_int.h |   6 +-
 block.c   | 145 +++---
 block/blkdebug.c  |  55 +++---
 block/blkverify.c |  16 +
 block/commit.c|   2 +-
 block/mirror.c|   2 +-
 block/nbd.c   |  23 +---
 block/nfs.c   |  36 +---
 block/null.c  |  23 +---
 block/quorum.c|  30 --
 10 files changed, 66 insertions(+), 272 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 5e0bfe70d4..178adde654 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -133,7 +133,11 @@ struct BlockDriver {
 int (*bdrv_create)(const char *filename, QemuOpts *opts, Error **errp);
 int (*bdrv_make_empty)(BlockDriverState *bs);
 
-void (*bdrv_refresh_filename)(BlockDriverState *bs, QDict *options);
+/*
+ * Refreshes the bs->exact_filename field. If that is impossible,
+ * bs->exact_filename has to be left empty.
+ */
+void (*bdrv_refresh_filename)(BlockDriverState *bs);
 
 /*
  * Gathers the open options for all children into @target.
diff --git a/block.c b/block.c
index 7980090d25..2963cbc921 100644
--- a/block.c
+++ b/block.c
@@ -5085,47 +5085,6 @@ static bool append_significant_runtime_options(QDict *d, 
BlockDriverState *bs)
 return found_any;
 }
 
-static bool append_open_options(QDict *d, BlockDriverState *bs)
-{
-const QDictEntry *entry;
-QemuOptDesc *desc;
-BdrvChild *child;
-bool found_any = false;
-const char *p;
-
-for (entry = qdict_first(bs->options); entry;
- entry = qdict_next(bs->options, entry))
-{
-/* Exclude options for children */
-QLIST_FOREACH(child, >children, next) {
-if (strstart(qdict_entry_key(entry), child->name, )
-&& (!*p || *p == '.'))
-{
-break;
-}
-}
-if (child) {
-continue;
-}
-
-/* And exclude all non-driver-specific options */
-for (desc = bdrv_runtime_opts.desc; desc->name; desc++) {
-if (!strcmp(qdict_entry_key(entry), desc->name)) {
-break;
-}
-}
-if (desc->name) {
-continue;
-}
-
-qobject_incref(qdict_entry_value(entry));
-qdict_put_obj(d, qdict_entry_key(entry), qdict_entry_value(entry));
-found_any = true;
-}
-
-return found_any;
-}
-
 /* Updates the following BDS fields:
  *  - exact_filename: A filename which may be used for opening a block device
  *which (mostly) equals the given BDS (even without any
@@ -5143,6 +5102,8 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 BlockDriver *drv = bs->drv;
 BdrvChild *child;
 QDict *opts;
+bool generate_json_filename; /* Whether our default implementation should
+fill exact_filename (false) or not (true) 
*/
 
 if (!drv) {
 return;
@@ -5158,90 +5119,10 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 }
 }
 
-if (drv->bdrv_refresh_filename) {
-/* Obsolete information is of no use here, so drop the old file name
- * information before refreshing it */
-bs->exact_filename[0] = '\0';
-if (bs->full_open_options) {
-QDECREF(bs->full_open_options);
-bs->full_open_options = NULL;
-}
-
-opts = qdict_new();
-append_open_options(opts, bs);
-drv->bdrv_refresh_filename(bs, opts);
-QDECREF(opts);
-} else if (bs->file) {
-/* Try to reconstruct valid information from the underlying file */
-bool has_open_options;
-
-bs->exact_filename[0] = '\0';
-if (bs->full_open_options) {
-QDECREF(bs->full_open_options);
-bs->full_open_options = NULL;
-}
-
-opts = qdict_new();
-has_open_options = append_open_options(opts, bs);
-