| Message ID | 20170118161653.19296-7-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Wed, Jan 18, 2017 at 9:02 PM Eric Blake <eblake@redhat.com> wrote: > Having a named rather than anonymous C type will make it easier > to improve the testsuite in a later patch. No semantic change, > to any of the existing code or to the introspection output. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > You should move the "Example:" back to blockdev-change-medium command. Otherwise, Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > v2: rebase to master > --- > qapi/block-core.json | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 1b3e6eb..0e31d25 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3119,6 +3119,15 @@ > # combines blockdev-open-tray, x-blockdev-remove-medium, > # x-blockdev-insert-medium and blockdev-close-tray). > # > +# Since: 2.5 > +## > +{ 'command': 'blockdev-change-medium', > + 'data': 'BlockdevChangeMedium' } > + > + > +## > +# @BlockdevChangeMedium: > +# > # @device: #optional Block device name (deprecated, use @id > instead) > # > # @id: #optional The name or QOM path of the guest device > @@ -3165,7 +3174,7 @@ > # <- { "return": {} } > # > ## > -{ 'command': 'blockdev-change-medium', > +{ 'struct': 'BlockdevChangeMedium', > 'data': { '*device': 'str', > '*id': 'str', > 'filename': 'str', > -- > 2.9.3 > > > -- Marc-André Lureau
Eric Blake <eblake@redhat.com> writes: > Having a named rather than anonymous C type will make it easier > to improve the testsuite in a later patch. Post it together with said later patch then. > No semantic change, > to any of the existing code or to the introspection output. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: rebase to master > --- > qapi/block-core.json | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 1b3e6eb..0e31d25 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -3119,6 +3119,15 @@ > # combines blockdev-open-tray, x-blockdev-remove-medium, > # x-blockdev-insert-medium and blockdev-close-tray). > # > +# Since: 2.5 > +## > +{ 'command': 'blockdev-change-medium', > + 'data': 'BlockdevChangeMedium' } > + > + > +## > +# @BlockdevChangeMedium: > +# > # @device: #optional Block device name (deprecated, use @id instead) > # > # @id: #optional The name or QOM path of the guest device # (since: 2.8) # # @filename: filename of the new image to be loaded # # @format: #optional, format to open the new image with (defaults to # the probed format) # # @read-only-mode: #optional, change the read-only mode of the device; defaults # to 'retain' # # Since: 2.5 Isn't Since: 2.5 misleading? The anonymous type goes back to 2.5, but the name doesn't. > @@ -3165,7 +3174,7 @@ > # <- { "return": {} } > # > ## > -{ 'command': 'blockdev-change-medium', > +{ 'struct': 'BlockdevChangeMedium', > 'data': { '*device': 'str', > '*id': 'str', > 'filename': 'str',
On 01/19/2017 03:48 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Having a named rather than anonymous C type will make it easier >> to improve the testsuite in a later patch. > > Post it together with said later patch then. It was that way in the v1 series, but I had enough else going on with removal of dynamic JSON parsing that I thought I could pick this one early. But I have no problems omitting it in a v3 respin, and saving it for the dynamic JSON work (if we even want to revisit that discussion). > # @format: #optional, format to open the new image with (defaults to > # the probed format) > # > # @read-only-mode: #optional, change the read-only mode of the device; defaults > # to 'retain' > # > # Since: 2.5 > > Isn't Since: 2.5 misleading? The anonymous type goes back to 2.5, but > the name doesn't. It matches what we've done elsewhere - when refactoring .json files to create a new type, but where the new type doesn't represent anything different over the wire than what was previously sent, we've documented the new struct name as of the older release where the wire format was introduced. But as long as the command says 'since 2.5', I'm okay if we want to mark the struct as 'since 2.9', if that's easier to think about.
Eric Blake <eblake@redhat.com> writes: > On 01/19/2017 03:48 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Having a named rather than anonymous C type will make it easier >>> to improve the testsuite in a later patch. >> >> Post it together with said later patch then. > > It was that way in the v1 series, but I had enough else going on with > removal of dynamic JSON parsing that I thought I could pick this one > early. But I have no problems omitting it in a v3 respin, and saving it > for the dynamic JSON work (if we even want to revisit that discussion). > > >> # @format: #optional, format to open the new image with (defaults to >> # the probed format) >> # >> # @read-only-mode: #optional, change the read-only mode of the device; defaults >> # to 'retain' >> # >> # Since: 2.5 >> >> Isn't Since: 2.5 misleading? The anonymous type goes back to 2.5, but >> the name doesn't. > > It matches what we've done elsewhere - when refactoring .json files to > create a new type, but where the new type doesn't represent anything > different over the wire than what was previously sent, we've documented > the new struct name as of the older release where the wire format was > introduced. But as long as the command says 'since 2.5', I'm okay if we > want to mark the struct as 'since 2.9', if that's easier to think about. Sticking to established practice is better than inconsistent practice. That said, I wonder why we bother to track "since" for types. "Since" is important information for external interfaces. Why is it useful for purely internal ones? External visible are commands, events, and members of types used by commands or events.
On 01/20/2017 01:15 AM, Markus Armbruster wrote: >>> Isn't Since: 2.5 misleading? The anonymous type goes back to 2.5, but >>> the name doesn't. >> >> It matches what we've done elsewhere - when refactoring .json files to >> create a new type, but where the new type doesn't represent anything >> different over the wire than what was previously sent, we've documented >> the new struct name as of the older release where the wire format was >> introduced. But as long as the command says 'since 2.5', I'm okay if we >> want to mark the struct as 'since 2.9', if that's easier to think about. > > Sticking to established practice is better than inconsistent practice. > > That said, I wonder why we bother to track "since" for types. "Since" > is important information for external interfaces. Why is it useful for > purely internal ones? > > External visible are commands, events, and members of types used by > commands or events. I guess what we can do is treat a type-wide 'Since: 2.5' as the default for all its members that don't supply any other '(since 2.6)' note. You're right that types themselves (even when used in a command) are NOT the API, and that we can refactor type names without breaking wire compatibility, but having a default for when each member of the type was first made available makes sense since we DO care when members were introduced.
diff --git a/qapi/block-core.json b/qapi/block-core.json index 1b3e6eb..0e31d25 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3119,6 +3119,15 @@ # combines blockdev-open-tray, x-blockdev-remove-medium, # x-blockdev-insert-medium and blockdev-close-tray). # +# Since: 2.5 +## +{ 'command': 'blockdev-change-medium', + 'data': 'BlockdevChangeMedium' } + + +## +# @BlockdevChangeMedium: +# # @device: #optional Block device name (deprecated, use @id instead) # # @id: #optional The name or QOM path of the guest device @@ -3165,7 +3174,7 @@ # <- { "return": {} } # ## -{ 'command': 'blockdev-change-medium', +{ 'struct': 'BlockdevChangeMedium', 'data': { '*device': 'str', '*id': 'str', 'filename': 'str',
Having a named rather than anonymous C type will make it easier to improve the testsuite in a later patch. No semantic change, to any of the existing code or to the introspection output. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: rebase to master --- qapi/block-core.json | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)