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?
Everything else looks okay with this patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
