Re: [PATCH v2 2/3] qapi: nbd-export: allow select bitmaps by node/name pair

[Eric Blake]

On Tue, Mar 15, 2022 at 12:32:25AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <v.sementsov-og@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-og@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