| Message ID | 1390361416-24234-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 22.01.2014 um 04:30 hat Fam Zheng geschrieben: > Currently there is no way to query BlockStats of the backing chain. This > adds "backing" and "file" fields into BlockStats to make it possible. > > The old optional field "parent", which has the information for "file", > is kept there for backward compatibility, as an alias for "file". > However field name "file" is more consistent with "file" option name in > the block device configuration interface. > > Signed-off-by: Fam Zheng <famz@redhat.com> I don't think it's a good idea to duplicate a field just because we like the new name better. Eric? Kevin
On 01/21/2014 08:30 PM, Fam Zheng wrote: > Currently there is no way to query BlockStats of the backing chain. This > adds "backing" and "file" fields into BlockStats to make it possible. > > The old optional field "parent", which has the information for "file", > is kept there for backward compatibility, as an alias for "file". Eww. BlockStats is a big struct (and getting bigger), which means this is a lot of JSON duplication to send over the wire just to get discarded (no matter whether the client uses the old or the new name). > However field name "file" is more consistent with "file" option name in > the block device configuration interface. Consistency is nice, but back-compat and efficiency take higher priority in this case. You are better off documenting that 'parent' refers to the details of the backing file, without adding a 'file' field. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/qapi.c | 7 +++++++ > qapi-schema.json | 15 +++++++++++---- > 2 files changed, 18 insertions(+), 4 deletions(-) > > +++ b/block/qapi.c > @@ -316,6 +316,13 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs) > if (bs->file) { > s->has_parent = true; > s->parent = bdrv_query_stats(bs->file); > + s->has_file = true; > + s->file = bdrv_query_stats(bs->file); > + } Drop this portion. > + > + if (bs->backing_hd) { > + s->has_backing = true; > + s->backing = bdrv_query_stats(bs->backing_hd); > } But this one is fine. > > return s; > diff --git a/qapi-schema.json b/qapi-schema.json > index f27c48a..c3c8aec 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -1022,15 +1022,22 @@ > # > # @stats: A @BlockDeviceStats for the device. > # > -# @parent: #optional This may point to the backing block device if this is a > -# a virtual block device. If it's a backing block, this will point > -# to the backing file is one is present. I'm okay if you improve the wording here, but keep this field untouched with just a better description. > +# @file: #optional This may point to the file block device if present. > +# (Since 2.0) Don't add this field. > +# > +# @backing: #optional This may point to the backing block device if present. > +# (Since 2.0) > +# s/may point/describes/ > +# @parent: #optional An alias of @file for backward comatibility. (Since > +# 0.14.0) s/comatibility/compatibility/ - except that I don't think you want to keep it as an alias. Also, the (Since 0.14.0) is bogus, since the field has existed for as long as the command has.
On 01/22/2014 02:35 AM, Kevin Wolf wrote: > Am 22.01.2014 um 04:30 hat Fam Zheng geschrieben: >> Currently there is no way to query BlockStats of the backing chain. This >> adds "backing" and "file" fields into BlockStats to make it possible. >> >> The old optional field "parent", which has the information for "file", >> is kept there for backward compatibility, as an alias for "file". >> However field name "file" is more consistent with "file" option name in >> the block device configuration interface. >> >> Signed-off-by: Fam Zheng <famz@redhat.com> > > I don't think it's a good idea to duplicate a field just because we like > the new name better. Eric? I concur - duplication for the sake of naming consistency doesn't add any measurable benefit; I'd rather just improve the docs for the existing name (libvirt will be forced to use the old name for back-compat reasons anyway, making the new name useless cruft that just makes the JSON longer and cost more cycles to parse then discard).
diff --git a/block/qapi.c b/block/qapi.c index a32cb79..858650e 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -316,6 +316,13 @@ BlockStats *bdrv_query_stats(const BlockDriverState *bs) if (bs->file) { s->has_parent = true; s->parent = bdrv_query_stats(bs->file); + s->has_file = true; + s->file = bdrv_query_stats(bs->file); + } + + if (bs->backing_hd) { + s->has_backing = true; + s->backing = bdrv_query_stats(bs->backing_hd); } return s; diff --git a/qapi-schema.json b/qapi-schema.json index f27c48a..c3c8aec 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1022,15 +1022,22 @@ # # @stats: A @BlockDeviceStats for the device. # -# @parent: #optional This may point to the backing block device if this is a -# a virtual block device. If it's a backing block, this will point -# to the backing file is one is present. +# @file: #optional This may point to the file block device if present. +# (Since 2.0) +# +# @backing: #optional This may point to the backing block device if present. +# (Since 2.0) +# +# @parent: #optional An alias of @file for backward comatibility. (Since +# 0.14.0) # # Since: 0.14.0 ## { 'type': 'BlockStats', 'data': {'*device': 'str', 'stats': 'BlockDeviceStats', - '*parent': 'BlockStats'} } + '*parent': 'BlockStats', + '*file': 'BlockStats', + '*backing': 'BlockStats'} } ## # @query-blockstats:
Currently there is no way to query BlockStats of the backing chain. This adds "backing" and "file" fields into BlockStats to make it possible. The old optional field "parent", which has the information for "file", is kept there for backward compatibility, as an alias for "file". However field name "file" is more consistent with "file" option name in the block device configuration interface. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/qapi.c | 7 +++++++ qapi-schema.json | 15 +++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-)