Re: [Qemu-devel] [PATCH v10 1/4] qapi: [trivial] Break long command lines

Re: [Qemu-devel] [PATCH v10 1/4] qapi: [trivial] Break long command lines

[Eric Blake]

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

On 04/30/2014 01:09 PM, Lluís Vilanova wrote:
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  Makefile       |   24 ++++++++++++++++++------
>  tests/Makefile |   20 ++++++++++++++++----
>  2 files changed, 34 insertions(+), 10 deletions(-)
> 

> @@ -362,7 +368,13 @@ check-tests/test-qapi.py: tests/test-qapi.py
>  
>  .PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
>  $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json
> -	$(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts $(PYTHON) $(SRC_PATH)/tests/qapi-schema/test-qapi.py <$^ >$*.test.out 2>$*.test.err; echo $$? >$*.test.exit, "  TEST  $*.out")
> +	$(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts \
> +		$(PYTHON) $(SRC_PATH)/tests/qapi-schema/test-qapi.py \
> +		<$^ \
> +		>$*.test.out \
> +		2>$*.test.err; \

I've already made the comment several times that I would have put these
three lines as one instead of excessively wrapping into three; I'm a bit
surprised you haven't picked up on the hint when rebasing for other
reasons.  But the patch as-is is still valid, so my Reviewed-by is still
okay whether or not you have a reason to spin a v11.

-- 
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 v10 3/4] qapi: Use an explicit input file

[Eric Blake]

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

On 04/30/2014 01:09 PM, Lluís Vilanova wrote:
> Use an explicit input file on the command-line instead of reading from standard
> input.
> 
> It also outputs the proper file name when there's an error.
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Technically, you made a non-trivial change (use of perl to sanitize
SRC_PATH) in response to a comment that did not come from my review. You
properly called it out in the cover letter, so I looked at this patch
again; but in general, when making non-trivial changes, it's best to
remove the Reviewed-by to make sure that earlier reviewers notice that
they need to look again, and that they are still happy with the changes.

Fortunately, in this case, your changes still look okay, so I'm okay
with leaving my Reviewed-by on this version of the patch.

>  		"  TEST  $*.out")
>  	@diff -q $(SRC_PATH)/$*.out $*.test.out
> -	@diff -q $(SRC_PATH)/$*.err $*.test.err
> +	@# Sanitize error messages (make them independent of build directory)
> +	@perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err -
>  	@diff -q $(SRC_PATH)/$*.exit $*.test.exit
>  

-- 
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 v10 4/4] qapi: Add a primitive to include other files from a QAPI schema file

[Eric Blake]

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

On 04/30/2014 01:09 PM, Lluís Vilanova wrote:
> The primitive uses JSON syntax, and include paths are relative to the file using the directive:
> 
>   { 'include': 'path/to/file.json' }
> 
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
> ---

> +++ b/tests/qapi-schema/include-after-err.json
> @@ -0,0 +1,2 @@
> +{ 'include': 'include-simple-sub.json' }
> +{ 'command' 'missing-colon' }

In v9, Markus pointed out that 'include-after-err' is the wrong name for
this test - you are testing that an error after include is detected
correctly, and not the behavior of an error after an include.

It's not a showstopper (I could live with v10 going into the tree), so:

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

But if you want to spin a v11, then rename the related files and the
Makefile rules to run the new name.  As a rename is trivially testable
by 'make check' still passing, then if that's the only change you make,
I'm still okay if you keep my R-b in your v11 message.

-- 
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 v10 0/4] qapi: Allow modularization of QAPI schema files

[Markus Armbruster]

Lluís Vilanova <vilanova@ac.upc.edu> writes:

> Adds an include primitive to the syntax of QAPI schema files, allowing these to
> be modularized into multiple per-topic files in the future.
>
> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>

A few more things could be polished (see Eric's comments), but they can
also be polished on top.  Additionally, review found some atrocious
error reporting in these scripts, but fixing that is clearly beyond the
scope of this series.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v10 4/4] qapi: Add a primitive to include other files from a QAPI schema file

[Lluís Vilanova]

Eric Blake writes:

> On 04/30/2014 01:09 PM, Lluís Vilanova wrote:
>> The primitive uses JSON syntax, and include paths are relative to the file using the directive:
>> 
>> { 'include': 'path/to/file.json' }
>> 
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> ---

>> +++ b/tests/qapi-schema/include-after-err.json
>> @@ -0,0 +1,2 @@
>> +{ 'include': 'include-simple-sub.json' }
>> +{ 'command' 'missing-colon' }

> In v9, Markus pointed out that 'include-after-err' is the wrong name for
> this test - you are testing that an error after include is detected
> correctly, and not the behavior of an error after an include.

> It's not a showstopper (I could live with v10 going into the tree), so:

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

> But if you want to spin a v11, then rename the related files and the
> Makefile rules to run the new name.  As a rename is trivially testable
> by 'make check' still passing, then if that's the only change you make,
> I'm still okay if you keep my R-b in your v11 message.

Right, I forgot to comment on this change. I did not want to change the name
because right now all tests start with "include-", which makes it simple to spot
them on a directory list.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH v10 3/4] qapi: Use an explicit input file

[Lluís Vilanova]

Eric Blake writes:

> On 04/30/2014 01:09 PM, Lluís Vilanova wrote:
>> Use an explicit input file on the command-line instead of reading from standard
>> input.
>> 
>> It also outputs the proper file name when there's an error.
>> 
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> Reviewed-by: Eric Blake <eblake@redhat.com>

> Technically, you made a non-trivial change (use of perl to sanitize
> SRC_PATH) in response to a comment that did not come from my review. You
> properly called it out in the cover letter, so I looked at this patch
> again; but in general, when making non-trivial changes, it's best to
> remove the Reviewed-by to make sure that earlier reviewers notice that
> they need to look again, and that they are still happy with the changes.

> Fortunately, in this case, your changes still look okay, so I'm okay
> with leaving my Reviewed-by on this version of the patch.

>> "  TEST  $*.out")
>> @diff -q $(SRC_PATH)/$*.out $*.test.out
>> -	@diff -q $(SRC_PATH)/$*.err $*.test.err
>> +	@# Sanitize error messages (make them independent of build directory)
>> +	@perl -p -e 's|\Q$(SRC_PATH)\E/||g' $*.test.err | diff -q $(SRC_PATH)/$*.err -
>> @diff -q $(SRC_PATH)/$*.exit $*.test.exit
>> 

Ok. Also note that I did not do the exact change proposed by Markus. I've
removed the '^' from the regular expression, since "tracebacks" in error
messages also contain the file path.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH v10 1/4] qapi: [trivial] Break long command lines

[Lluís Vilanova]

Eric Blake writes:

> On 04/30/2014 01:09 PM, Lluís Vilanova wrote:
>> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>> Makefile       |   24 ++++++++++++++++++------
>> tests/Makefile |   20 ++++++++++++++++----
>> 2 files changed, 34 insertions(+), 10 deletions(-)
>> 

>> @@ -362,7 +368,13 @@ check-tests/test-qapi.py: tests/test-qapi.py
>> 
>> .PHONY: $(patsubst %, check-%, $(check-qapi-schema-y))
>> $(patsubst %, check-%, $(check-qapi-schema-y)): check-%.json: $(SRC_PATH)/%.json
>> -	$(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts $(PYTHON) $(SRC_PATH)/tests/qapi-schema/test-qapi.py <$^ >$*.test.out 2>$*.test.err; echo $$? >$*.test.exit, "  TEST  $*.out")
>> +	$(call quiet-command, PYTHONPATH=$(SRC_PATH)/scripts \
>> +		$(PYTHON) $(SRC_PATH)/tests/qapi-schema/test-qapi.py \
>> +		<$^ \
>> +		>$*.test.out \
>> +		2>$*.test.err; \

> I've already made the comment several times that I would have put these
> three lines as one instead of excessively wrapping into three; I'm a bit
> surprised you haven't picked up on the hint when rebasing for other
> reasons.  But the patch as-is is still valid, so my Reviewed-by is still
> okay whether or not you have a reason to spin a v11.

Sorry, forgot about that one.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth

Re: [Qemu-devel] [PATCH v10 4/4] qapi: Add a primitive to include other files from a QAPI schema file

[Eric Blake]

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

On 05/02/2014 06:16 AM, Lluís Vilanova wrote:

>>> +++ b/tests/qapi-schema/include-after-err.json
>>> @@ -0,0 +1,2 @@
>>> +{ 'include': 'include-simple-sub.json' }
>>> +{ 'command' 'missing-colon' }
> 
>> In v9, Markus pointed out that 'include-after-err' is the wrong name for
>> this test - you are testing that an error after include is detected
>> correctly, and not the behavior of an error after an include.
> 
>> It's not a showstopper (I could live with v10 going into the tree), so:
> 
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>> But if you want to spin a v11, then rename the related files and the
>> Makefile rules to run the new name.  As a rename is trivially testable
>> by 'make check' still passing, then if that's the only change you make,
>> I'm still okay if you keep my R-b in your v11 message.
> 
> Right, I forgot to comment on this change. I did not want to change the name
> because right now all tests start with "include-", which makes it simple to spot
> them on a directory list.

Then name it include-before-err.json

-- 
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 v10 4/4] qapi: Add a primitive to include other files from a QAPI schema file

[Lluís Vilanova]

Eric Blake writes:

> On 05/02/2014 06:16 AM, Lluís Vilanova wrote:
>>>> +++ b/tests/qapi-schema/include-after-err.json
>>>> @@ -0,0 +1,2 @@
>>>> +{ 'include': 'include-simple-sub.json' }
>>>> +{ 'command' 'missing-colon' }
>> 
>>> In v9, Markus pointed out that 'include-after-err' is the wrong name for
>>> this test - you are testing that an error after include is detected
>>> correctly, and not the behavior of an error after an include.
>> 
>>> It's not a showstopper (I could live with v10 going into the tree), so:
>> 
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 
>>> But if you want to spin a v11, then rename the related files and the
>>> Makefile rules to run the new name.  As a rename is trivially testable
>>> by 'make check' still passing, then if that's the only change you make,
>>> I'm still okay if you keep my R-b in your v11 message.
>> 
>> Right, I forgot to comment on this change. I did not want to change the name
>> because right now all tests start with "include-", which makes it simple to spot
>> them on a directory list.

> Then name it include-before-err.json

Ok, then I'll resend a trivial v11 with this rename and the line breaking
changes.


Lluis

-- 
 "And it's much the same thing with knowledge, for whenever you learn
 something new, the whole world becomes that much richer."
 -- The Princess of Pure Reason, as told by Norton Juster in The Phantom
 Tollbooth