On Thu, 10/30 15:47, Eric Blake wrote:
> On 10/28/2014 11:04 PM, Fam Zheng wrote:
> > This bool option will allow query all the node names. It iterates all
> > the BDSes that are assigned a name, also in this case don't query up the
> > backing chain.
> >
> > Signed-off-by: Fam Zheng <f...@redhat.com>
> > ---
> > block/qapi.c | 20 +++++++++++++-------
> > hmp.c | 2 +-
> > qapi/block-core.json | 4 +++-
> > qmp-commands.hx | 2 +-
> > 4 files changed, 18 insertions(+), 10 deletions(-)
> >
>
> > -BlockStatsList *qmp_query_blockstats(Error **errp)
> > +BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
> > + bool query_nodes,
> > + Error **errp)
> > {
> > BlockStatsList *head = NULL, **p_next = &head;
> > BlockDriverState *bs = NULL;
> >
> > - while ((bs = bdrv_next(bs))) {
> > + /* Just to be safe if query_nodes is not always intialized */
>
> s/intialized/initialized/
>
> > + query_nodes = query_nodes && has_query_nodes;
>
> If things aren't initialized (was true a while ago, but we recently
> fixed that to ensure 0 initialization, although no one yet really relies
> on the guarantee), then valgrind could complain of a branch on an uninit
> memory location. Idiomatically, this is usually written:
>
> query_nodes = has_query_nodes && query_nodes;
This does read better. Will fix.
>
> to pacify valgrind if we hadn't zero-initialized; although logically,
> the result is identical, so I don't care if you leave it.
>
> > +++ b/qapi/block-core.json
> > @@ -434,7 +434,9 @@
> > #
> > # Since: 0.14.0
> > ##
> > -{ 'command': 'query-blockstats', 'returns': ['BlockStats'] }
> > +{ 'command': 'query-blockstats',
> > + 'data': {'*query-nodes': 'bool' },
> > + 'returns': ['BlockStats'] }
>
> Max correctly pointed out that this is missing documentation.
>
> The idea looks sane; it will visit all named nodes (whether or not those
> nodes are also reachable from named devices), and omit any unnamed nodes
> (right now, libvirt would have to be taught to name nodes, or Jeff's
> patch to auto-name nodes will avoid that problem).
>
> Hmm, I wonder - if we are adding an optional parameter that controls
> what to iterate over, should we also add an optional parameter that says
> to limit the output to a given input name? Then again, we don't have
> very many existing query-* commands that filter, and at any rate, adding
> such a filter should be its own patch.
Or separate series :)
Thanks,
Fam
