Re: [PATCH RFC] qapi: Allow getting flat output from 'query-named-block-nodes'

[Markus Armbruster <armbru@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: