| Message ID | 42dae98e1f6a9f444f48a20192f45195337824f0.1576246045.git.pkrempa@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] qapi: Allow getting flat output from 'query-named-block-nodes' | expand |
On 12/13/19 8:11 AM, Peter Krempa wrote: > When a management application manages node names there's no reason to > recurse into backing images in the output of query-named-block-nodes. > > Add a parameter to the command which will return just the top level > structs. At one point, Kevin was working on a saner command that tried to cut out on more than just the redundant nesting. But this is certainly a quick-and-easy fix to ease libvirt's use of the existing command, while we decide whether to add a saner new command. > > Signed-off-by: Peter Krempa <pkrempa@redhat.com> > --- > block.c | 5 +++-- > block/qapi.c | 10 ++++++++-- > blockdev.c | 12 ++++++++++-- > include/block/block.h | 2 +- > include/block/qapi.h | 4 +++- > monitor/hmp-cmds.c | 2 +- > qapi/block-core.json | 6 +++++- > 7 files changed, 31 insertions(+), 10 deletions(-) > > +++ b/blockdev.c > @@ -3707,9 +3707,17 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp) > } > } > > -BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) > +BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat, > + bool flat, > + Error **errp) > { > - return bdrv_named_nodes_list(errp); > + bool return_flat = false; > + > + if (has_flat) { > + return_flat = flat; > + } This could be shortened as 'bool return_flat = has_flat && flat;', but that's not essential. > + > + return bdrv_named_nodes_list(return_flat, errp); > } > Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 12/13/19 8:11 AM, Peter Krempa wrote: >> When a management application manages node names there's no reason to >> recurse into backing images in the output of query-named-block-nodes. >> >> Add a parameter to the command which will return just the top level >> structs. > > At one point, Kevin was working on a saner command that tried to cut > out on more than just the redundant nesting. But this is certainly a > quick-and-easy fix to ease libvirt's use of the existing command, > while we decide whether to add a saner new command. What exactly is the problem libvirt is having with the existing query-named-block-nodes? >> >> Signed-off-by: Peter Krempa <pkrempa@redhat.com> >> --- >> block.c | 5 +++-- >> block/qapi.c | 10 ++++++++-- >> blockdev.c | 12 ++++++++++-- >> include/block/block.h | 2 +- >> include/block/qapi.h | 4 +++- >> monitor/hmp-cmds.c | 2 +- >> qapi/block-core.json | 6 +++++- >> 7 files changed, 31 insertions(+), 10 deletions(-) >> > >> +++ b/blockdev.c >> @@ -3707,9 +3707,17 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp) >> } >> } >> >> -BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) >> +BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat, >> + bool flat, >> + Error **errp) >> { >> - return bdrv_named_nodes_list(errp); >> + bool return_flat = false; >> + >> + if (has_flat) { >> + return_flat = flat; >> + } > > This could be shortened as 'bool return_flat = has_flat && flat;', but > that's not essential. I'd prefer that. Even return_flat = flat would do, because !has_flat implies !flat when flat is bool. Matter of taste. >> + >> + return bdrv_named_nodes_list(return_flat, errp); >> } >> > > Reviewed-by: Eric Blake <eblake@redhat.com> Un-snipping the QAPI schema change: >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 0cf68fea14..bd651106bd 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1752,6 +1752,8 @@ >> # >> # Get the named block driver list >> # >> +# @flat: don't recurse into backing images if true. Default is false (Since 5.0) >> +# >> # Returns: the list of BlockDeviceInfo >> # >> # Since: 2.0 What does it mean not to recurse? Sounds like flat: false asks for a subset of the full set. How exactly is the subset defined? >> @@ -1805,7 +1807,9 @@ >> # } } ] } >> # >> ## >> -{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] } >> +{ 'command': 'query-named-block-nodes', >> + 'returns': [ 'BlockDeviceInfo' ], >> + 'data': { '*flat': 'bool' } } >> >> ## >> # @XDbgBlockGraphNodeType:
On 12/17/19 1:36 AM, Markus Armbruster wrote: > Un-snipping the QAPI schema change: Sorry about that... > >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 0cf68fea14..bd651106bd 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -1752,6 +1752,8 @@ >>> # >>> # Get the named block driver list >>> # >>> +# @flat: don't recurse into backing images if true. Default is false (Since 5.0) >>> +# >>> # Returns: the list of BlockDeviceInfo >>> # >>> # Since: 2.0 > > What does it mean not to recurse? Sounds like flat: false asks for a > subset of the full set. How exactly is the subset defined? Bike-shedding: Would it be easier to explain as: @recurse: If true, include child information in each node (note that this can result in redundant output). Default is true (since 5.0) and then pass false when you don't want recursion, with it being more obvious that using the new flag results in more compact output with no loss of information.
Eric Blake <eblake@redhat.com> writes: > On 12/17/19 1:36 AM, Markus Armbruster wrote: > >> Un-snipping the QAPI schema change: > > Sorry about that... > >> >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>> index 0cf68fea14..bd651106bd 100644 >>>> --- a/qapi/block-core.json >>>> +++ b/qapi/block-core.json >>>> @@ -1752,6 +1752,8 @@ >>>> # >>>> # Get the named block driver list >>>> # >>>> +# @flat: don't recurse into backing images if true. Default is false (Since 5.0) >>>> +# >>>> # Returns: the list of BlockDeviceInfo >>>> # >>>> # Since: 2.0 >> >> What does it mean not to recurse? Sounds like flat: false asks for a >> subset of the full set. How exactly is the subset defined? > > Bike-shedding: > > Would it be easier to explain as: > > @recurse: If true, include child information in each node (note that > this can result in redundant output). Default is true (since 5.0) > > and then pass false when you don't want recursion, with it being more > obvious that using the new flag results in more compact output with no > loss of information. Adds a bit of information, namely that the information suppressed by recurse: false is actually redundant. The command's doc comment is weak: it doesn't really specify what exactly is returned. Simply omitting redundant information always should still conform to this weak spec. Of course, what really counts isn't spec conformance, but meeting client expectations. I don't even understand what exactly gets returned, let alone how clients use it. The doc comment needs to be judged by someone with actual knowledge in this area.
On Tue, Dec 17, 2019 at 16:11:37 +0100, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > > > On 12/17/19 1:36 AM, Markus Armbruster wrote: > > > >> Un-snipping the QAPI schema change: > > > > Sorry about that... > > > >> > >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json > >>>> index 0cf68fea14..bd651106bd 100644 > >>>> --- a/qapi/block-core.json > >>>> +++ b/qapi/block-core.json > >>>> @@ -1752,6 +1752,8 @@ > >>>> # > >>>> # Get the named block driver list > >>>> # > >>>> +# @flat: don't recurse into backing images if true. Default is false (Since 5.0) > >>>> +# > >>>> # Returns: the list of BlockDeviceInfo > >>>> # > >>>> # Since: 2.0 > >> > >> What does it mean not to recurse? Sounds like flat: false asks for a > >> subset of the full set. How exactly is the subset defined? > > > > Bike-shedding: > > > > Would it be easier to explain as: > > > > @recurse: If true, include child information in each node (note that > > this can result in redundant output). Default is true (since 5.0) > > > > and then pass false when you don't want recursion, with it being more > > obvious that using the new flag results in more compact output with no > > loss of information. > > Adds a bit of information, namely that the information suppressed by > recurse: false is actually redundant. > > The command's doc comment is weak: it doesn't really specify what > exactly is returned. Simply omitting redundant information always > should still conform to this weak spec. Of course, what really counts > isn't spec conformance, but meeting client expectations. I don't even > understand what exactly gets returned, let alone how clients use it. Well I by default expect that if the top level array has data for all node names the nesting which repeats the information (partially) should not be there because you can just look elsewhere for a more detailed output. Said that two years ago I wrote some code which detects the node names from running qemu because at that time it was the only way to use the block write threshold event. This code needs to extract the nesting topology somehow but I don't remember nor fancy re-understanding the algorithm for the detection during the hollidays. The only thing I can say that the nesting is extracted either from the output of query-block or from query-named-block nodes as both these outputs are fed to that algorithm. With blockdev that piece of code thankfully is unused, but unfortunately I can't say that the nested output of query-named-block-nodes doesn't have it's use.
diff --git a/block.c b/block.c index 473eb6eeaa..b30bdfa0d3 100644 --- a/block.c +++ b/block.c @@ -4766,14 +4766,15 @@ BlockDriverState *bdrv_find_node(const char *node_name) } /* Put this QMP function here so it can access the static graph_bdrv_states. */ -BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp) +BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, + Error **errp) { BlockDeviceInfoList *list, *entry; BlockDriverState *bs; list = NULL; QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) { - BlockDeviceInfo *info = bdrv_block_device_info(NULL, bs, errp); + BlockDeviceInfo *info = bdrv_block_device_info(NULL, bs, flat, errp); if (!info) { qapi_free_BlockDeviceInfoList(list); return NULL; diff --git a/block/qapi.c b/block/qapi.c index 9a5d0c9b27..84048e1a57 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -42,7 +42,9 @@ #include "qemu/cutils.h" BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, - BlockDriverState *bs, Error **errp) + BlockDriverState *bs, + bool flat, + Error **errp) { ImageInfo **p_image_info; BlockDriverState *bs0; @@ -156,6 +158,10 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, return NULL; } + /* stop gathering data for flat output */ + if (flat) + break; + if (bs0->drv && bs0->backing) { info->backing_file_depth++; bs0 = bs0->backing->bs; @@ -389,7 +395,7 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info, if (bs && bs->drv) { info->has_inserted = true; - info->inserted = bdrv_block_device_info(blk, bs, errp); + info->inserted = bdrv_block_device_info(blk, bs, false, errp); if (info->inserted == NULL) { goto err; } diff --git a/blockdev.c b/blockdev.c index 8e029e9c01..5f9c5e258f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3707,9 +3707,17 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp) } } -BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) +BlockDeviceInfoList *qmp_query_named_block_nodes(bool has_flat, + bool flat, + Error **errp) { - return bdrv_named_nodes_list(errp); + bool return_flat = false; + + if (has_flat) { + return_flat = flat; + } + + return bdrv_named_nodes_list(return_flat, errp); } XDbgBlockGraph *qmp_x_debug_query_block_graph(Error **errp) diff --git a/include/block/block.h b/include/block/block.h index 1df9848e74..177ba09e3f 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -468,7 +468,7 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked); void bdrv_eject(BlockDriverState *bs, bool eject_flag); const char *bdrv_get_format_name(BlockDriverState *bs); BlockDriverState *bdrv_find_node(const char *node_name); -BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp); +BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp); XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp); BlockDriverState *bdrv_lookup_bs(const char *device, const char *node_name, diff --git a/include/block/qapi.h b/include/block/qapi.h index cd9410dee3..22c7807c89 100644 --- a/include/block/qapi.h +++ b/include/block/qapi.h @@ -29,7 +29,9 @@ #include "block/snapshot.h" BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, - BlockDriverState *bs, Error **errp); + BlockDriverState *bs, + bool flat, + Error **errp); int bdrv_query_snapshot_info_list(BlockDriverState *bs, SnapshotInfoList **p_list, Error **errp); diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index b2551c16d1..651969819b 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -620,7 +620,7 @@ void hmp_info_block(Monitor *mon, const QDict *qdict) } /* Print node information */ - blockdev_list = qmp_query_named_block_nodes(NULL); + blockdev_list = qmp_query_named_block_nodes(false, false, NULL); for (blockdev = blockdev_list; blockdev; blockdev = blockdev->next) { assert(blockdev->value->has_node_name); if (device && strcmp(device, blockdev->value->node_name)) { diff --git a/qapi/block-core.json b/qapi/block-core.json index 0cf68fea14..bd651106bd 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1752,6 +1752,8 @@ # # Get the named block driver list # +# @flat: don't recurse into backing images if true. Default is false (Since 5.0) +# # Returns: the list of BlockDeviceInfo # # Since: 2.0 @@ -1805,7 +1807,9 @@ # } } ] } # ## -{ 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] } +{ 'command': 'query-named-block-nodes', + 'returns': [ 'BlockDeviceInfo' ], + 'data': { '*flat': 'bool' } } ## # @XDbgBlockGraphNodeType:
When a management application manages node names there's no reason to recurse into backing images in the output of query-named-block-nodes. Add a parameter to the command which will return just the top level structs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- block.c | 5 +++-- block/qapi.c | 10 ++++++++-- blockdev.c | 12 ++++++++++-- include/block/block.h | 2 +- include/block/qapi.h | 4 +++- monitor/hmp-cmds.c | 2 +- qapi/block-core.json | 6 +++++- 7 files changed, 31 insertions(+), 10 deletions(-)