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

[Vladimir Sementsov-Ogievskiy <v.sementsov-og@mail.ru>]

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