| Message ID | 20200813162935.210070-16-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: Add infrastructure and QAPI for block exports | expand |
On 13.08.20 18:29, Kevin Wolf wrote: > Every block export needs a block node to export, so move the 'device' > option from BlockExportOptionsNbd to BlockExportOptions. > > To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs > to be wrapped by a new type NbdServerAddOptions that adds 'device' back > because nbd-server-add doesn't use the BlockExportOptions base type. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-export.json | 27 +++++++++++++++++++++------ > block/export/export.c | 26 ++++++++++++++++++++------ > block/monitor/block-hmp-cmds.c | 6 +++--- > blockdev-nbd.c | 4 ++-- > qemu-nbd.c | 2 +- > 5 files changed, 47 insertions(+), 18 deletions(-) (Code diff looks good, just a question on the interface:) > diff --git a/qapi/block-export.json b/qapi/block-export.json > index 4ce163411f..d68f3bf87e 100644 > --- a/qapi/block-export.json > +++ b/qapi/block-export.json [...] > @@ -167,6 +179,8 @@ > # Describes a block export, i.e. how single node should be exported on an > # external interface. > # > +# @device: The device name or node name of the node to be exported > +# Wouldn’t it be better to restrict ourselves to a node name here? (Bluntly ignoring the fact that doing so would make this patch an incompatible change, which would require some reordering in this series, unless we decide to just ignore that intra-series incompatibility.) OTOH... What does “better” mean. It won’t hurt anyone to keep this as @device. It’s just that I feel like if we had no legacy burden and did this all from scratch, we would just make it @node-name. Did you deliberately decide against @node-name? Max
Am 17.08.2020 um 17:13 hat Max Reitz geschrieben: > On 13.08.20 18:29, Kevin Wolf wrote: > > Every block export needs a block node to export, so move the 'device' > > option from BlockExportOptionsNbd to BlockExportOptions. > > > > To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs > > to be wrapped by a new type NbdServerAddOptions that adds 'device' back > > because nbd-server-add doesn't use the BlockExportOptions base type. > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > qapi/block-export.json | 27 +++++++++++++++++++++------ > > block/export/export.c | 26 ++++++++++++++++++++------ > > block/monitor/block-hmp-cmds.c | 6 +++--- > > blockdev-nbd.c | 4 ++-- > > qemu-nbd.c | 2 +- > > 5 files changed, 47 insertions(+), 18 deletions(-) > > (Code diff looks good, just a question on the interface:) > > > diff --git a/qapi/block-export.json b/qapi/block-export.json > > index 4ce163411f..d68f3bf87e 100644 > > --- a/qapi/block-export.json > > +++ b/qapi/block-export.json > > [...] > > > @@ -167,6 +179,8 @@ > > # Describes a block export, i.e. how single node should be exported on an > > # external interface. > > # > > +# @device: The device name or node name of the node to be exported > > +# > > Wouldn’t it be better to restrict ourselves to a node name here? > (Bluntly ignoring the fact that doing so would make this patch an > incompatible change, which would require some reordering in this series, > unless we decide to just ignore that intra-series incompatibility.) We already have intra-series incompatibility, so I wouldn't mind that. > OTOH... What does “better” mean. It won’t hurt anyone to keep this as > @device. It’s just that I feel like if we had no legacy burden and did > this all from scratch, we would just make it @node-name. > > Did you deliberately decide against @node-name? At first I thought I could still share code between nbd-server-add and block-export-add, but that's not the case. Then I guess the only other reason I have is consistency with other QMP commands. I won't pretend it's a strong one. Kevin
On 17.08.20 17:27, Kevin Wolf wrote: > Am 17.08.2020 um 17:13 hat Max Reitz geschrieben: >> On 13.08.20 18:29, Kevin Wolf wrote: >>> Every block export needs a block node to export, so move the 'device' >>> option from BlockExportOptionsNbd to BlockExportOptions. >>> >>> To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs >>> to be wrapped by a new type NbdServerAddOptions that adds 'device' back >>> because nbd-server-add doesn't use the BlockExportOptions base type. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> qapi/block-export.json | 27 +++++++++++++++++++++------ >>> block/export/export.c | 26 ++++++++++++++++++++------ >>> block/monitor/block-hmp-cmds.c | 6 +++--- >>> blockdev-nbd.c | 4 ++-- >>> qemu-nbd.c | 2 +- >>> 5 files changed, 47 insertions(+), 18 deletions(-) >> >> (Code diff looks good, just a question on the interface:) >> >>> diff --git a/qapi/block-export.json b/qapi/block-export.json >>> index 4ce163411f..d68f3bf87e 100644 >>> --- a/qapi/block-export.json >>> +++ b/qapi/block-export.json >> >> [...] >> >>> @@ -167,6 +179,8 @@ >>> # Describes a block export, i.e. how single node should be exported on an >>> # external interface. >>> # >>> +# @device: The device name or node name of the node to be exported >>> +# >> >> Wouldn’t it be better to restrict ourselves to a node name here? >> (Bluntly ignoring the fact that doing so would make this patch an >> incompatible change, which would require some reordering in this series, >> unless we decide to just ignore that intra-series incompatibility.) > > We already have intra-series incompatibility, so I wouldn't mind that. > >> OTOH... What does “better” mean. It won’t hurt anyone to keep this as >> @device. It’s just that I feel like if we had no legacy burden and did >> this all from scratch, we would just make it @node-name. >> >> Did you deliberately decide against @node-name? > > At first I thought I could still share code between nbd-server-add and > block-export-add, but that's not the case. Then I guess the only other > reason I have is consistency with other QMP commands. I won't pretend > it's a strong one. If “using @node-name would be more natural” doesn’t resonate with you, then I suppose we should just leave it as @device. It isn’t harder to handle for qemu, and maybe it makes transitioning easier for some users (although I can’t quite imagine how). Max
On 8/13/20 11:29 AM, Kevin Wolf wrote: > Every block export needs a block node to export, so move the 'device' > option from BlockExportOptionsNbd to BlockExportOptions. > > To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs > to be wrapped by a new type NbdServerAddOptions that adds 'device' back > because nbd-server-add doesn't use the BlockExportOptions base type. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > +++ b/qapi/block-export.json > @@ -62,9 +62,8 @@ > ## > # @BlockExportOptionsNbd: > # > -# An NBD block export. > -# > -# @device: The device name or node name of the node to be exported > +# An NBD block export (options shared between nbd-server-add and the NBD branch > +# of block-export-add). > # > # @name: Export name. If unspecified, the @device parameter is used as the > # export name. (Since 2.12) > @@ -82,8 +81,21 @@ > # Since: 5.0 > ## > { 'struct': 'BlockExportOptionsNbd', > - 'data': {'device': 'str', '*name': 'str', '*description': 'str', > - '*writable': 'bool', '*bitmap': 'str' } } > + 'data': { '*name': 'str', '*description': 'str', > + '*writable': 'bool', '*bitmap': 'str' } } > + > +## > +# @NbdServerAddOptions: > +# > +# An NBD block export. > +# > +# @device: The device name or node name of the node to be exported > +# > +# Since: 5.0 5.2, now? > +## > +{ 'struct': 'NbdServerAddOptions', > + 'base': 'BlockExportOptionsNbd', > + 'data': { 'device': 'str' } } I understand why nbd sticks with device that can name device or node, but... > > ## > # @nbd-server-add: > @@ -96,7 +108,7 @@ > # Since: 1.3.0 > ## > { 'command': 'nbd-server-add', > - 'data': 'BlockExportOptionsNbd', 'boxed': true } > + 'data': 'NbdServerAddOptions', 'boxed': true } > > ## > # @NbdServerRemoveMode: > @@ -167,6 +179,8 @@ > # Describes a block export, i.e. how single node should be exported on an > # external interface. > # > +# @device: The device name or node name of the node to be exported > +# > # @writethrough: If true, caches are flushed after every write request to the > # export before completion is signalled. (since: 5.2; > # default: false) > @@ -175,6 +189,7 @@ > ## > { 'union': 'BlockExportOptions', > 'base': { 'type': 'BlockExportType', > + 'device': 'str', for block export, I'd prefer that we mandate node name only, and naming it @node-name (rather than @device) feels nicer, for something that only new code will be using (that is, we should be encouraging the use of node names, especially now that libvirt has finally made that leap).
diff --git a/qapi/block-export.json b/qapi/block-export.json index 4ce163411f..d68f3bf87e 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -62,9 +62,8 @@ ## # @BlockExportOptionsNbd: # -# An NBD block export. -# -# @device: The device name or node name of the node to be exported +# An NBD block export (options shared between nbd-server-add and the NBD branch +# of block-export-add). # # @name: Export name. If unspecified, the @device parameter is used as the # export name. (Since 2.12) @@ -82,8 +81,21 @@ # Since: 5.0 ## { 'struct': 'BlockExportOptionsNbd', - 'data': {'device': 'str', '*name': 'str', '*description': 'str', - '*writable': 'bool', '*bitmap': 'str' } } + 'data': { '*name': 'str', '*description': 'str', + '*writable': 'bool', '*bitmap': 'str' } } + +## +# @NbdServerAddOptions: +# +# An NBD block export. +# +# @device: The device name or node name of the node to be exported +# +# Since: 5.0 +## +{ 'struct': 'NbdServerAddOptions', + 'base': 'BlockExportOptionsNbd', + 'data': { 'device': 'str' } } ## # @nbd-server-add: @@ -96,7 +108,7 @@ # Since: 1.3.0 ## { 'command': 'nbd-server-add', - 'data': 'BlockExportOptionsNbd', 'boxed': true } + 'data': 'NbdServerAddOptions', 'boxed': true } ## # @NbdServerRemoveMode: @@ -167,6 +179,8 @@ # Describes a block export, i.e. how single node should be exported on an # external interface. # +# @device: The device name or node name of the node to be exported +# # @writethrough: If true, caches are flushed after every write request to the # export before completion is signalled. (since: 5.2; # default: false) @@ -175,6 +189,7 @@ ## { 'union': 'BlockExportOptions', 'base': { 'type': 'BlockExportType', + 'device': 'str', '*writethrough': 'bool' }, 'discriminator': 'type', 'data': { diff --git a/block/export/export.c b/block/export/export.c index 1d5de564c7..ef86bf892b 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -68,15 +68,26 @@ void qmp_block_export_add(BlockExportOptions *export, Error **errp) blk_exp_add(export, errp); } -void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) +void qmp_nbd_server_add(NbdServerAddOptions *arg, Error **errp) { BlockExport *export; BlockDriverState *bs; BlockBackend *on_eject_blk; - BlockExportOptions export_opts = { - .type = BLOCK_EXPORT_TYPE_NBD, - .u.nbd = *arg, + BlockExportOptions *export_opts = g_new(BlockExportOptions, 1); + *export_opts = (BlockExportOptions) { + .type = BLOCK_EXPORT_TYPE_NBD, + .device = g_strdup(arg->device), + .u.nbd = { + .has_name = arg->has_name, + .name = g_strdup(arg->name), + .has_description = arg->has_description, + .description = g_strdup(arg->description), + .has_writable = arg->has_writable, + .writable = arg->writable, + .has_bitmap = arg->has_bitmap, + .bitmap = g_strdup(arg->bitmap), + }, }; /* @@ -89,9 +100,9 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) arg->writable = false; } - export = blk_exp_add(&export_opts, errp); + export = blk_exp_add(export_opts, errp); if (!export) { - return; + goto fail; } /* @@ -102,4 +113,7 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp) if (on_eject_blk) { nbd_export_set_on_eject_blk(export, on_eject_blk); } + +fail: + qapi_free_BlockExportOptions(export_opts); } diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c index a651954e16..6c823234a9 100644 --- a/block/monitor/block-hmp-cmds.c +++ b/block/monitor/block-hmp-cmds.c @@ -398,7 +398,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict) Error *local_err = NULL; BlockInfoList *block_list, *info; SocketAddress *addr; - BlockExportOptionsNbd export; + NbdServerAddOptions export; if (writable && !all) { error_setg(&local_err, "-w only valid together with -a"); @@ -431,7 +431,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict) continue; } - export = (BlockExportOptionsNbd) { + export = (NbdServerAddOptions) { .device = info->value->device, .has_writable = true, .writable = writable, @@ -458,7 +458,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict) bool writable = qdict_get_try_bool(qdict, "writable", false); Error *local_err = NULL; - BlockExportOptionsNbd export = { + NbdServerAddOptions export = { .device = (char *) device, .has_name = !!name, .name = (char *) name, diff --git a/blockdev-nbd.c b/blockdev-nbd.c index a8b7b785e7..5e97975c80 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -188,7 +188,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) } if (!arg->has_name) { - arg->name = arg->device; + arg->name = exp_args->device; } if (strlen(arg->name) > NBD_MAX_STRING_SIZE) { @@ -206,7 +206,7 @@ BlockExport *nbd_export_create(BlockExportOptions *exp_args, Error **errp) return NULL; } - bs = bdrv_lookup_bs(arg->device, arg->device, errp); + bs = bdrv_lookup_bs(exp_args->device, exp_args->device, errp); if (!bs) { return NULL; } diff --git a/qemu-nbd.c b/qemu-nbd.c index d967b8fcb9..f31868708c 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -1055,10 +1055,10 @@ int main(int argc, char **argv) export_opts = g_new(BlockExportOptions, 1); *export_opts = (BlockExportOptions) { .type = BLOCK_EXPORT_TYPE_NBD, + .device = g_strdup(bdrv_get_node_name(bs)), .has_writethrough = true, .writethrough = writethrough, .u.nbd = { - .device = g_strdup(bdrv_get_node_name(bs)), .has_name = true, .name = g_strdup(export_name), .has_description = !!export_description,
Every block export needs a block node to export, so move the 'device' option from BlockExportOptionsNbd to BlockExportOptions. To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs to be wrapped by a new type NbdServerAddOptions that adds 'device' back because nbd-server-add doesn't use the BlockExportOptions base type. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-export.json | 27 +++++++++++++++++++++------ block/export/export.c | 26 ++++++++++++++++++++------ block/monitor/block-hmp-cmds.c | 6 +++--- blockdev-nbd.c | 4 ++-- qemu-nbd.c | 2 +- 5 files changed, 47 insertions(+), 18 deletions(-)