Re: [Qemu-devel] [PATCH 4/4] qmp: Add optional switch "query-nodes" in query-blockstats

[Fam Zheng]

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 <address@hidden>
> > ---
> >  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