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(-)
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,
},
--
2.28.0
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>
--
Best regards,
Vladimir
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, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
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, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
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.
© 2016 - 2026 Red Hat, Inc.