On 12/06/2016 09:11 AM, Eric Blake wrote: >> BlockdevOptionsGluster.debug(-level) does not have "Added in 2.8" so I >> had to dig through git-blame(1) to verify that it was indeed added in >> the current release cycle. > > Then that implies we should add yet one more patch that adds the > appropriate versioning information to all the gluster fields added for > 2.8. My reviewed-by was given on the assumption that debug was in 2.7 > and that this was a break from 2.7 behavior, but that we already KNOW > we're breaking blockdev-add between 2.7 and 2.8; while your argument is > that there is no backwards incompatibility because it was not in 2.7 to > begin with. I think both reasons are indeed acceptable, but it also > means that my reason was flawed because of the incomplete documentation.
I checked again. 'git diff v2.7.0..origin qapi/*.json' shows: ## -# @BlockdevOptionsGluster +# @BlockdevOptionsGluster: # # Driver specific block device options for Gluster # @@ -2140,7 +2195,9 @@ # # @server: gluster servers description # -# @debug-level: #optional libgfapi log level (default '4' which is Error) +# @debug: #optional libgfapi log level (default '4' which is Error) +# +# @logfile: #optional libgfapi log file (default /dev/stderr) (Since 2.8) # # Since: 2.7 ## @@ -2148,25 +2205,163 @@ 'data': { 'volume': 'str', 'path': 'str', 'server': ['GlusterServer'], - '*debug-level': 'int' } } + '*debug': 'int', + '*logfile': 'str' } } So I was right after all - this IS a backwards-incompatible change (and NOT something that was introduced only in 2.8) - but I still argue that the change is appropriate NOW (but not later) because blockdev-add is the only client of this type in QMP, and that command changed backwards-incompatibly at the same time. > >> >> In the future please make sure all QAPI changes are marked by version. > > Indeed, and I try to flag it in my reviews as often as I notice it. > >> If there tricky changes you can include a statement showing you are >> aware of QAPI backwards compatibility ("These new options were added in >> the 2.8 release cycle and can therefore still be changed without >> breaking backward compatibility"). This will make me confident that >> you've checked the QAPI changes. >> >> Thanks, applied to my staging tree: >> https://github.com/stefanha/qemu/commits/staging In my audit of all differences between 2.7 and current state of qapi .json files, I only found one addition that was lacking versioning information: event DEVICE_TRAY_MOVED gained 'id' parameter I'll submit a patch for that shortly. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature