17.03.2022 00:28, Eric Blake wrote:
On Tue, Mar 15, 2022 at 12:32:25AM +0300, Vladimir Sementsov-Ogievskiy wrote:
From: Vladimir Sementsov-Ogievskiy <v.sementsov...@ya.ru>
Hi all! Current logic of relying on search through backing chain is not
safe neither convenient.
Sometimes it leads to necessity of extra bitmap copying. Also, we are
going to add "snapshot-access" driver, to access some snapshot state
through NBD. And this driver is not formally a filter, and of course
it's not a COW format driver. So, searching through backing chain will
not work. Instead of widening the workaround of bitmap searching, let's
extend the interface so that user can select bitmap precisely.
Note, that checking for bitmap active status is not copied to the new
API, I don't see a reason for it, user should understand the risks. And
anyway, bitmap from other node is unrelated to this export being
read-only or read-write.
Signed-off-by: Vladimir Sementsov-Ogievskiy <v.sementsov...@ya.ru>
---
blockdev-nbd.c | 8 +++++-
nbd/server.c | 63 +++++++++++++++++++++++++++---------------
qapi/block-export.json | 5 +++-
qemu-nbd.c | 11 ++++++--
4 files changed, 61 insertions(+), 26 deletions(-)
@@ -1709,37 +1709,56 @@ static int nbd_export_create(BlockExport *blk_exp,
BlockExportOptions *exp_args,
}
exp->export_bitmaps = g_new0(BdrvDirtyBitmap *, exp->nr_export_bitmaps);
for (i = 0, bitmaps = arg->bitmaps; bitmaps;
- i++, bitmaps = bitmaps->next) {
- const char *bitmap = bitmaps->value;
+ i++, bitmaps = bitmaps->next)
+ {
+ const char *bitmap;
I'm not sure if our prevailing style splits { to its own line on a
multi-line 'for'. But this is a cosmetic question, not one of
correctness.
+ case QTYPE_QDICT:
+ bitmap = bitmaps->value->u.external.name;
+ bm = block_dirty_bitmap_lookup(bitmaps->value->u.external.node,
+ bitmap, NULL, errp);
+ if (!bm) {
+ ret = -ENOENT;
+ goto fail;
+ }
+ break;
+ default:
+ abort();
Not sure if g_assert_not_reached() or __builtin_unreachable() would be
any better here. I'm fine with the abort() for now.
+++ b/qapi/block-export.json
@@ -6,6 +6,7 @@
##
{ 'include': 'sockets.json' }
+{ 'include': 'block-core.json' }
Hmm. Does this extra inclusion negatively impact qemu-storage-daemon,
since that is why we created block-export.json in the first place (to
minimize the stuff that qsd pulled in without needing all of
block-core.json)? In other words, would it be better to move
BlockDirtyBitmapOrStr to this file?
And include block-export in block-core?
Another alternative is to move BlockDirtyBitmapOrStr to a separate file
included from both block-export and block-core but that seems to be too much.
Everything else looks okay with this patch.
--
Best regards,
Vladimir