| Message ID | 1480973521-28945-1-git-send-email-jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Dec 05, 2016 at 04:31:58PM -0500, Jeff Cody wrote: > The following changes since commit bc66cedb4141fb7588f2462c74310d8fb5dd4cf1: > > Merge remote-tracking branch 'yongbok/tags/mips-20161204' into staging (2016-12-05 10:56:45 +0000) > > are available in the git repository at: > > https://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request > > for you to fetch changes up to 76b5550f709b975a7b04fb4c887f300b7bb731c2: > > qemu-doc: update gluster protocol usage guide (2016-12-05 16:30:29 -0500) > > ---------------------------------------------------------------- > Gluster block patches for -rc3 > ---------------------------------------------------------------- > > Prasanna Kumar Kalever (3): > block/gluster: fix QMP to match debug option > block/nfs: fix QMP to match debug option > qemu-doc: update gluster protocol usage guide > > block/gluster.c | 38 ++++++++++++++++----------------- > block/nfs.c | 4 ++-- > qapi/block-core.json | 8 +++---- > qemu-doc.texi | 59 +++++++++++++++++++++++++++++++++++++++------------- > qemu-options.hx | 25 ++++++++++++++++++++-- > 5 files changed, 93 insertions(+), 41 deletions(-) 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. In the future please make sure all QAPI changes are marked by version. 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 Stefan
On 12/06/2016 04:04 AM, Stefan Hajnoczi wrote: >> >> Prasanna Kumar Kalever (3): >> block/gluster: fix QMP to match debug option >> block/nfs: fix QMP to match debug option >> qemu-doc: update gluster protocol usage guide >> >> block/gluster.c | 38 ++++++++++++++++----------------- >> block/nfs.c | 4 ++-- >> qapi/block-core.json | 8 +++---- >> qemu-doc.texi | 59 +++++++++++++++++++++++++++++++++++++++------------- >> qemu-options.hx | 25 ++++++++++++++++++++-- >> 5 files changed, 93 insertions(+), 41 deletions(-) > > 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. > > 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 > > Stefan >
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.
Am 06.12.2016 um 16:43 hat Eric Blake geschrieben: > 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. I don't completely understand the change anyway. 'debug-level' is the better name, and if the command line doesn't agree, change that one or grudgingly accept both. But I think we can just change the command line, it's only a debug option. Kevin
On 12/07/2016 04:08 AM, Kevin Wolf wrote: >> # 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. > > I don't completely understand the change anyway. 'debug-level' is the > better name, Perhaps so, but 2.7 already used 'debug' on the command line, and released libvirt already targets the command line. Changing the command line to accept 'debug-level' in addition to 'debug' is the only way to not break existing libvirt usage. What's more, we have several instances of 'debug' in other spots of blockdev-add; either they should all be consistently named 'debug-level', or none of them; we made the switch to none of them, as it was easier. > and if the command line doesn't agree, change that one or > grudgingly accept both. But I think we can just change the command line, > it's only a debug option. It may only be a debug option, but for good or bad, libvirt is already targetting it by its current spelling of 'debug'.