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

[Vladimir Sementsov-Ogievskiy]

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-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?

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