Re: [Qemu-devel] [PATCH v1 00/72] Extract qapi commons and block definitions

Re: [Qemu-devel] [PATCH v1 00/72] Extract qapi commons and block definitions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

On 05/31/2014 05:50 AM, Benoît Canet wrote:
> This hudge chainsawing series sit on top of my quorum maintenance series.
> 
> It extract commons definition and block definition into separate files.

I'm not sure if you had to split it quite so fine (a series of three
commits, one per file, might have been okay); but doing it one commit
per function does make each individual patch easy to review.

> 
> -qapi/common.json contains some definition required by all qapi modules
> -qapi/block-core.json contains core qapi block definition usable without the emulation
> code.
> -qapi/block.json is a superset of the previous usable with emulation
> 
> Transaction where left appart because they relies on internal snapshot and hence
> cannot be included in block-core.json for now.
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 01/72] qapi: Extract ErrorClass definition in qapi/common.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]

On 05/31/2014 05:50 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json | 28 ++--------------------------
>  qapi/common.json | 29 +++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 26 deletions(-)
>  create mode 100644 qapi/common.json
> 

> --- /dev/null
> +++ b/qapi/common.json
> @@ -0,0 +1,29 @@
> +# -*- Mode: Python -*-
> +#
> +# QAPI common definitions

I know qapi-schema.json lacks a copyright statement (which I assume
implies to mean it is GPLv2+ ?), but if you are going to create a new
file, it would be nice to make the copyright comment explicit.

As the problem is pre-existing, I'm okay fixing it up in a separate
patch at the same time qapi-schema.json is fixed.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 02/72] qapi: Extract VersionInfo definition in qapi/common.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

On 05/31/2014 05:50 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json | 26 --------------------------
>  qapi/common.json | 27 +++++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 26 deletions(-)

Why the difference in line counts?


> +           'package': 'str'} }
> +
> 

Ah, because you ended the file in a trailing blank line.  git can be set
up to have a commit hook that flags trailing blank lines, but as it is
cosmetic and probably transient based on the rest of the series, I can
overlook it.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 05/72] qapi: Extract query-commands definition into qapi/common.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 399 bytes --]

On 05/31/2014 05:50 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json | 11 -----------
>  qapi/common.json | 11 +++++++++++
>  2 files changed, 11 insertions(+), 11 deletions(-)

3-5: Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 06/72] qapi: create two block related json modules

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

On 05/31/2014 05:50 AM, Benoît Canet wrote:
> qapi/block-core.json contains block definitions unrelated to emulation.
> 
> qapi/block.json is a superset of the previous and contains definitions related
> to emulation.
> 
> The purpose of these extractions is to be able to hook qapi/block-core.json
> generated code on qemu-nbd.
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json     | 3 +++
>  qapi/block-core.json | 8 ++++++++
>  qapi/block.json      | 7 +++++++
>  3 files changed, 18 insertions(+)
>  create mode 100644 qapi/block-core.json
>  create mode 100644 qapi/block.json

> --- /dev/null
> +++ b/qapi/block-core.json
> @@ -0,0 +1,8 @@
> +# -*- Mode: Python -*-
> +#
> +# QAPI block core definitions (vm unrelated)

Same comment as in 1/72 about adding a copyright comment; and same
conclusion that doing it as a followup patch for all affected files
being okay.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 08/72] qapi: Extract ImageInfoSpecificQCow2 definition into qapi/block-core.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 447 bytes --]

On 05/31/2014 05:50 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json     | 15 ---------------
>  qapi/block-core.json | 15 +++++++++++++++
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 

> +  } }
> +
> 

Similar comments as in 2/72 about trailing blank lines.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 20/72] qapi: Extract BlockStats definition into qapi/block-core.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On 05/31/2014 05:50 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json     | 22 ----------------------
>  qapi/block-core.json | 22 ++++++++++++++++++++++
>  2 files changed, 22 insertions(+), 22 deletions(-)

7-20: Reviewed-by: Eric Blake <eblake@redhat.com>

And yes, I really do think this is too fine of a split.  It would have
been faster for me to do a review of a single patch covering all structs
moved to block-core.json in one go than it is to review lots of tiny
piecemeal patches (as I mentioned in the RFC, it is very easy to do a
review-by-sed for large code motion patches, as long as the code moved
maintains its relative ordering between the old and new files:
https://lists.gnu.org/archive/html/qemu-devel/2014-05/msg06116.html -
what I was complaining about there was that you had one patch with
changes intermixed into three files, so the code motion no longer had a
relative ordering between deletion and insertion lines).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 38/72] qapi: Extract block-stream definition into qapi/block-core.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

On 05/31/2014 05:51 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json     | 38 --------------------------------------
>  qapi/block-core.json | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 38 deletions(-)

21-38: Reviewed-by: Eric Blake <eblake@redhat.com>

I will point out that all this refactoring work will have conflicts with
other pending patches in the same area, such as Jeff's work on using
node-name in block-stream; but such conflicts should be obvious
resolutions (either series can go first, as long as the final result is
the improved command ending up in the new file).  I don't know which
series going in first would be more of a conflict magnet to the other
series.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 44/72] qapi: Extract drive-mirror-replace definition into qapi/block-core.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 576 bytes --]

On 05/31/2014 05:51 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json     | 33 ---------------------------------
>  qapi/block-core.json | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 33 deletions(-)

39-44: Reviewed-by: Eric Blake <eblake@redhat.com>

> -# @drive-mirror-replace:

This obviously depends on your other series for additional quorum
maintenance commands.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 44/72] qapi: Extract drive-mirror-replace definition into qapi/block-core.json

[Benoît Canet]

The Tuesday 03 Jun 2014 à 08:55:24 (-0600), Eric Blake wrote :
> On 05/31/2014 05:51 AM, Benoît Canet wrote:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  qapi-schema.json     | 33 ---------------------------------
> >  qapi/block-core.json | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 33 insertions(+), 33 deletions(-)
> 
> 39-44: Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> > -# @drive-mirror-replace:
> 
> This obviously depends on your other series for additional quorum
> maintenance commands.

Yes

Thanks for the review

Best regards

Benoît

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH v1 72/72] qapi: Extract nbd-server-stop definition into qapi/block.json

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

On 05/31/2014 05:51 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json | 10 ----------
>  qapi/block.json  | 10 ++++++++++
>  2 files changed, 10 insertions(+), 10 deletions(-)

45-72: Reviewed-by: Eric Blake <eblake@redhat.com>

If you resend this series for any reason, PLEASE consolidate into fewer
patches. As long as the types are copied out of qapi-schema.json in the
same order that they are copied into block.json, it's much faster to
review that the code motion was done correctly if there is only one such
patch describing the motion to block.json than to read lots of tiny
patches one per struct/command.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 00/72] Extract qapi commons and block definitions

[Stefan Hajnoczi]

On Sat, May 31, 2014 at 01:50:39PM +0200, Benoît Canet wrote:
> This hudge chainsawing series sit on top of my quorum maintenance series.
> 
> It extract commons definition and block definition into separate files.
> 
> -qapi/common.json contains some definition required by all qapi modules
> -qapi/block-core.json contains core qapi block definition usable without the emulation
> code.
> -qapi/block.json is a superset of the previous usable with emulation
> 
> Transaction where left appart because they relies on internal snapshot and hence
> cannot be included in block-core.json for now.

I'm fine with this, it's mostly a mechanical change.

But please squash it down into just a few patches:

1. Extract block-core.json and include it.
2. Extract block.json and include it.

I also suggest you drop the quorum maintenance command dependency
because rebasing this series will be no fun.  This way you minimize
churn.  We can merge this qapi series very quickly, while the quorum
maintenance series may require more discussion.

Stefan

Re: [Qemu-devel] [PATCH v1 00/72] Extract qapi commons and block definitions

[Benoît Canet]

The Wednesday 04 Jun 2014 à 16:34:14 (+0200), Stefan Hajnoczi wrote :
> On Sat, May 31, 2014 at 01:50:39PM +0200, Benoît Canet wrote:
> > This hudge chainsawing series sit on top of my quorum maintenance series.
> > 
> > It extract commons definition and block definition into separate files.
> > 
> > -qapi/common.json contains some definition required by all qapi modules
> > -qapi/block-core.json contains core qapi block definition usable without the emulation
> > code.
> > -qapi/block.json is a superset of the previous usable with emulation
> > 
> > Transaction where left appart because they relies on internal snapshot and hence
> > cannot be included in block-core.json for now.
> 
> I'm fine with this, it's mostly a mechanical change.
> 
> But please squash it down into just a few patches:
> 
> 1. Extract block-core.json and include it.
> 2. Extract block.json and include it.

ok,

> 
> I also suggest you drop the quorum maintenance command dependency
> because rebasing this series will be no fun.  This way you minimize
> churn.  We can merge this qapi series very quickly, while the quorum
> maintenance series may require more discussion.

I don't mind rebasing this QAPI series 10 times as I don't need I before 2.2.
Right now the only thing that interest me is getting the quorum maintainance
series in.

Best regards

Benoît

> 
> Stefan
>