| Message ID | 20201009215533.1194742-5-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Exposing backing-chain allocation over NBD | expand |
10.10.2020 00:55, Eric Blake wrote: > Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to > 5.2, we can still tweak the interface. Allowing 'bitmaps':['str'] is > nicer than 'bitmap':'str'. This wires up the qapi and qemu-nbd > changes to permit passing multiple bitmaps as distinct metadata > contexts that the NBD client may request, but the actual support for > more than one will require a further patch to the server. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- [..] > break; > case 'B': > - bitmap = optarg; > + tmp = g_new(strList, 1); > + tmp->value = g_strdup(optarg); > + tmp->next = bitmaps; > + bitmaps = tmp; If publish QAPI_LIST_ADD, defined in block.c, it would look like: QAPI_LIST_ADD(bitmaps, g_strdup(optarg)); anyway: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 10/14/20 7:15 AM, Vladimir Sementsov-Ogievskiy wrote: > 10.10.2020 00:55, Eric Blake wrote: >> Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to >> 5.2, we can still tweak the interface. Allowing 'bitmaps':['str'] is >> nicer than 'bitmap':'str'. This wires up the qapi and qemu-nbd >> changes to permit passing multiple bitmaps as distinct metadata >> contexts that the NBD client may request, but the actual support for >> more than one will require a further patch to the server. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- > > [..] > >> break; >> case 'B': >> - bitmap = optarg; >> + tmp = g_new(strList, 1); >> + tmp->value = g_strdup(optarg); >> + tmp->next = bitmaps; >> + bitmaps = tmp; > > If publish QAPI_LIST_ADD, defined in block.c, it would look like: > > QAPI_LIST_ADD(bitmaps, g_strdup(optarg)); #define QAPI_LIST_ADD(list, element) do { \ typeof(list) _tmp = g_new(typeof(*(list)), 1); \ _tmp->value = (element); \ _tmp->next = (list); \ (list) = _tmp; \ } while (0) Markus, thoughts on if we should publish this macro, and if so, which header would be best? > > anyway: > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >
Eric Blake <eblake@redhat.com> writes: > On 10/14/20 7:15 AM, Vladimir Sementsov-Ogievskiy wrote: >> 10.10.2020 00:55, Eric Blake wrote: >>> Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to >>> 5.2, we can still tweak the interface. Allowing 'bitmaps':['str'] is >>> nicer than 'bitmap':'str'. This wires up the qapi and qemu-nbd >>> changes to permit passing multiple bitmaps as distinct metadata >>> contexts that the NBD client may request, but the actual support for >>> more than one will require a further patch to the server. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> --- >> [..] >> >>> break; >>> case 'B': >>> - bitmap = optarg; >>> + tmp = g_new(strList, 1); >>> + tmp->value = g_strdup(optarg); >>> + tmp->next = bitmaps; >>> + bitmaps = tmp; >> If publish QAPI_LIST_ADD, defined in block.c, it would look like: >> QAPI_LIST_ADD(bitmaps, g_strdup(optarg)); > > #define QAPI_LIST_ADD(list, element) do { \ > typeof(list) _tmp = g_new(typeof(*(list)), 1); \ > _tmp->value = (element); \ > _tmp->next = (list); \ > (list) = _tmp; \ > } while (0) > > > Markus, thoughts on if we should publish this macro, If it's widely useful. "git-grep -- '->value ='" matches ~200 times. A patch converting these to the macro where possible would make a strong case for having the macro. > and if so, which > header would be best? The macro is generic: @list's type may be any of the struct TYPEList we generate for the QAPI type ['TYPE']. We don't want to generate this macro next to each of these struct definitions. A non-generic macro would go there, but let's not generate almost a hundred non-generic macros where a single generic one can do the job. The closest we have to a common base is GenericList. It's in in visitor.h because it's only used by visitors so far. Adding the macro next it is not so smart, because we don't want non-visitor code to include visitor.h just for this macro. Perhaps the macro should go into qapi/util.h, and perhaps GenericList and GenericAlternate should move there, too. [...]
On 10/20/20 3:51 AM, Markus Armbruster wrote: >> #define QAPI_LIST_ADD(list, element) do { \ >> typeof(list) _tmp = g_new(typeof(*(list)), 1); \ >> _tmp->value = (element); \ >> _tmp->next = (list); \ >> (list) = _tmp; \ >> } while (0) >> >> >> Markus, thoughts on if we should publish this macro, > > If it's widely useful. > > "git-grep -- '->value ='" matches ~200 times. A patch converting these > to the macro where possible would make a strong case for having the > macro. > >> and if so, which >> header would be best? > > The macro is generic: @list's type may be any of the struct TYPEList we > generate for the QAPI type ['TYPE']. > > We don't want to generate this macro next to each of these struct > definitions. A non-generic macro would go there, but let's not generate > almost a hundred non-generic macros where a single generic one can do > the job. Agreed. > > The closest we have to a common base is GenericList. It's in in > visitor.h because it's only used by visitors so far. Adding the macro > next it is not so smart, because we don't want non-visitor code to > include visitor.h just for this macro. Also agreed. > > Perhaps the macro should go into qapi/util.h, and perhaps GenericList > and GenericAlternate should move there, too. GenericList is easy, but GenericAlternate is harder: it would introduce a cyclic declaration dependency (generated qapi-builtin-types.h includes qapi/util.h for the definition of QEnumLookup, but qapi/util.h declaring GenericAlternate would depend on including qapi-builtin-types.h for the definition of QType).
Eric Blake <eblake@redhat.com> writes: > On 10/20/20 3:51 AM, Markus Armbruster wrote: > >>> #define QAPI_LIST_ADD(list, element) do { \ >>> typeof(list) _tmp = g_new(typeof(*(list)), 1); \ >>> _tmp->value = (element); \ >>> _tmp->next = (list); \ >>> (list) = _tmp; \ >>> } while (0) >>> >>> >>> Markus, thoughts on if we should publish this macro, >> >> If it's widely useful. >> >> "git-grep -- '->value ='" matches ~200 times. A patch converting these >> to the macro where possible would make a strong case for having the >> macro. >> >>> and if so, which >>> header would be best? >> >> The macro is generic: @list's type may be any of the struct TYPEList we >> generate for the QAPI type ['TYPE']. >> >> We don't want to generate this macro next to each of these struct >> definitions. A non-generic macro would go there, but let's not generate >> almost a hundred non-generic macros where a single generic one can do >> the job. > > Agreed. > >> >> The closest we have to a common base is GenericList. It's in in >> visitor.h because it's only used by visitors so far. Adding the macro >> next it is not so smart, because we don't want non-visitor code to >> include visitor.h just for this macro. > > Also agreed. > >> >> Perhaps the macro should go into qapi/util.h, and perhaps GenericList >> and GenericAlternate should move there, too. > > GenericList is easy, but GenericAlternate is harder: it would introduce > a cyclic declaration dependency (generated qapi-builtin-types.h includes > qapi/util.h for the definition of QEnumLookup, but qapi/util.h declaring > GenericAlternate would depend on including qapi-builtin-types.h for the > definition of QType). You're right. QType is a built-in QAPI type. The typedef is generated into qapi-builtin-types.h. It needs to be a QAPI type because it's the type of alternates' (implicit) member @type. I figure the easiest way to move GenericAlternate (if we want to), is creating a new header, or rather splitting qapi/util.h into the part needed by qapi-builtin-types.h and the part that needs qapi-builtin-types.h. Doesn't seem to be worth our while now. We can simply put the macro into qapi/util.h and call it a day.
diff --git a/qapi/block-export.json b/qapi/block-export.json index 893d5cde5dfe..c7c749d61097 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -74,10 +74,10 @@ # @description: Free-form description of the export, up to 4096 bytes. # (Since 5.0) # -# @bitmap: Also export the dirty bitmap reachable from @device, so the -# NBD client can use NBD_OPT_SET_META_CONTEXT with the -# metadata context name "qemu:dirty-bitmap:NAME" to inspect the -# bitmap. (since 4.0) +# @bitmaps: Also export each of the named dirty bitmaps reachable from +# @device, so the NBD client can use NBD_OPT_SET_META_CONTEXT with +# the metadata context name "qemu:dirty-bitmap:BITMAP" to inspect +# each bitmap. (since 5.2) # # @allocation-depth: Also export the allocation depth map for @device, so # the NBD client can use NBD_OPT_SET_META_CONTEXT with @@ -88,7 +88,7 @@ ## { 'struct': 'BlockExportOptionsNbd', 'data': { '*name': 'str', '*description': 'str', - '*bitmap': 'str', '*allocation-depth': 'bool' } } + '*bitmaps': ['str'], '*allocation-depth': 'bool' } } ## # @NbdServerAddOptions: @@ -100,12 +100,18 @@ # @writable: Whether clients should be able to write to the device via the # NBD connection (default false). # +# @bitmap: Also export a single dirty bitmap reachable from @device, so the +# NBD client can use NBD_OPT_SET_META_CONTEXT with the metadata +# context name "qemu:dirty-bitmap:BITMAP" to inspect the bitmap +# (since 4.0). Mutually exclusive with @bitmaps, and newer +# clients should use that instead. +# # Since: 5.0 ## { 'struct': 'NbdServerAddOptions', 'base': 'BlockExportOptionsNbd', 'data': { 'device': 'str', - '*writable': 'bool' } } + '*writable': 'bool', '*bitmap': 'str' } } ## # @nbd-server-add: diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 43856c1058a5..359a198de2c7 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -192,6 +192,20 @@ void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp) return; } + /* + * New code should use the list 'bitmaps'; but until this code is + * gone, we must support the older single 'bitmap'. Use only one. + */ + if (arg->has_bitmap) { + if (arg->has_bitmaps) { + error_setg(errp, "Can't mix 'bitmap' and 'bitmaps'"); + return; + } + arg->has_bitmaps = true; + arg->bitmaps = g_new0(strList, 1); + arg->bitmaps->value = g_strdup(arg->bitmap); + } + /* * block-export-add would default to the node-name, but we may have to use * the device name as a default here for compatibility. diff --git a/nbd/server.c b/nbd/server.c index 30cfe0eee467..884ffa00f1bd 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1495,6 +1495,7 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, uint64_t perm, shared_perm; bool readonly = !exp_args->writable; bool shared = !exp_args->writable; + strList *bitmaps; int ret; assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD); @@ -1556,12 +1557,18 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, } exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE); - if (arg->bitmap) { + /* XXX Allow more than one bitmap */ + if (arg->bitmaps && arg->bitmaps->next) { + error_setg(errp, "multiple bitmaps per export not supported yet"); + return -EOPNOTSUPP; + } + for (bitmaps = arg->bitmaps; bitmaps; bitmaps = bitmaps->next) { + const char *bitmap = bitmaps->value; BlockDriverState *bs = blk_bs(blk); BdrvDirtyBitmap *bm = NULL; while (bs) { - bm = bdrv_find_dirty_bitmap(bs, arg->bitmap); + bm = bdrv_find_dirty_bitmap(bs, bitmap); if (bm != NULL) { break; } @@ -1571,7 +1578,7 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, if (bm == NULL) { ret = -ENOENT; - error_setg(errp, "Bitmap '%s' is not found", arg->bitmap); + error_setg(errp, "Bitmap '%s' is not found", bitmap); goto fail; } @@ -1585,15 +1592,15 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, ret = -EINVAL; error_setg(errp, "Enabled bitmap '%s' incompatible with readonly export", - arg->bitmap); + bitmap); goto fail; } bdrv_dirty_bitmap_set_busy(bm, true); exp->export_bitmap = bm; - assert(strlen(arg->bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE); + assert(strlen(bitmap) <= BDRV_BITMAP_MAX_NAME_SIZE); exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", - arg->bitmap); + bitmap); assert(strlen(exp->export_bitmap_context) < NBD_MAX_STRING_SIZE); } diff --git a/qemu-nbd.c b/qemu-nbd.c index 847fde435a7f..fa5c68749e8f 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -572,7 +572,7 @@ int main(int argc, char **argv) const char *export_name = NULL; /* defaults to "" later for server mode */ const char *export_description = NULL; bool alloc_depth = false; - const char *bitmap = NULL; + strList *bitmaps = NULL, *tmp; const char *tlscredsid = NULL; bool imageOpts = false; bool writethrough = true; @@ -701,7 +701,10 @@ int main(int argc, char **argv) alloc_depth = true; break; case 'B': - bitmap = optarg; + tmp = g_new(strList, 1); + tmp->value = g_strdup(optarg); + tmp->next = bitmaps; + bitmaps = tmp; break; case 'k': sockpath = optarg; @@ -798,7 +801,7 @@ int main(int argc, char **argv) } if (export_name || export_description || dev_offset || device || disconnect || fmt || sn_id_or_name || alloc_depth || - bitmap || seen_aio || seen_discard || seen_cache) { + bitmaps || seen_aio || seen_discard || seen_cache) { error_report("List mode is incompatible with per-device settings"); exit(EXIT_FAILURE); } @@ -1082,8 +1085,8 @@ int main(int argc, char **argv) .name = g_strdup(export_name), .has_description = !!export_description, .description = g_strdup(export_description), - .has_bitmap = !!bitmap, - .bitmap = g_strdup(bitmap), + .has_bitmaps = !!bitmaps, + .bitmaps = bitmaps, .has_allocation_depth = alloc_depth, .allocation_depth = alloc_depth, },
Since 'nbd-server-add' is deprecated, and 'block-export-add' is new to 5.2, we can still tweak the interface. Allowing 'bitmaps':['str'] is nicer than 'bitmap':'str'. This wires up the qapi and qemu-nbd changes to permit passing multiple bitmaps as distinct metadata contexts that the NBD client may request, but the actual support for more than one will require a further patch to the server. Signed-off-by: Eric Blake <eblake@redhat.com> --- qapi/block-export.json | 18 ++++++++++++------ blockdev-nbd.c | 14 ++++++++++++++ nbd/server.c | 19 +++++++++++++------ qemu-nbd.c | 13 ++++++++----- 4 files changed, 47 insertions(+), 17 deletions(-)