| Message ID | 1374842387-17146-2-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/26/2013 06:39 AM, Markus Armbruster wrote: > The parser handles erroneous input badly. To be improved shortly. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- Lots of proof on how bad it is! I'd also like to see a couple tests on trailing commas: { 'enum': 'Foo', [ 'bar' ], } { 'enum': 'Gur', [ 'ble', ] } since we have had patches in the past to clean them up (shame on JSON for copying C89 instead of C99 with regards to trailing commas). Either way, Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 07/26/2013 06:39 AM, Markus Armbruster wrote: >> The parser handles erroneous input badly. To be improved shortly. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- > > Lots of proof on how bad it is! I'd also like to see a couple tests on > trailing commas: > > { 'enum': 'Foo', [ 'bar' ], } > { 'enum': 'Gur', [ 'ble', ] } I figure you mean { 'enum': 'Foo', 'data': [ 'bar' ], } { 'enum': 'Gur', 'data': [ 'ble', ] } My parser rejects both: <stdin>:1:37: Expected string <stdin>:2:35: Expected "{", "[" or string I commented out the first to get the second error. Making the parser continue after errors didn't seem to be worthwhile. > since we have had patches in the past to clean them up (shame on JSON > for copying C89 instead of C99 with regards to trailing commas). Indeed. > Either way, > > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
On 07/26/2013 08:16 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 07/26/2013 06:39 AM, Markus Armbruster wrote: >>> The parser handles erroneous input badly. To be improved shortly. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >> >> Lots of proof on how bad it is! I'd also like to see a couple tests on >> trailing commas: >> >> { 'enum': 'Foo', [ 'bar' ], } >> { 'enum': 'Gur', [ 'ble', ] } > > I figure you mean > > { 'enum': 'Foo', 'data': [ 'bar' ], } > { 'enum': 'Gur', 'data': [ 'ble', ] } Yep, you got my intent, even if I didn't type it right. > > My parser rejects both: > > <stdin>:1:37: Expected string > <stdin>:2:35: Expected "{", "[" or string Good! > > I commented out the first to get the second error. Making the parser > continue after errors didn't seem to be worthwhile. Agree about not continuing after errors; once the file is clean, anyone adding a new command will have errors in at most the one command they are trying to add, and even if it is an iterative approach to get them to find all the problems, it's a lot easier to have that one developer fix their work than trying to bake error recovery into the parser. If you do add these two test cases (which I recommend), it should indeed be two separate tests. Another thought - is it worth testing other valid JSON but invalid schema constructs, such as: { 'enum': 1, 'data': false }
Eric Blake <eblake@redhat.com> writes: > On 07/26/2013 08:16 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 07/26/2013 06:39 AM, Markus Armbruster wrote: >>>> The parser handles erroneous input badly. To be improved shortly. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>>> --- >>> >>> Lots of proof on how bad it is! I'd also like to see a couple tests on >>> trailing commas: >>> >>> { 'enum': 'Foo', [ 'bar' ], } >>> { 'enum': 'Gur', [ 'ble', ] } >> >> I figure you mean >> >> { 'enum': 'Foo', 'data': [ 'bar' ], } >> { 'enum': 'Gur', 'data': [ 'ble', ] } > > Yep, you got my intent, even if I didn't type it right. > >> >> My parser rejects both: >> >> <stdin>:1:37: Expected string >> <stdin>:2:35: Expected "{", "[" or string > > Good! > >> >> I commented out the first to get the second error. Making the parser >> continue after errors didn't seem to be worthwhile. > > Agree about not continuing after errors; once the file is clean, anyone > adding a new command will have errors in at most the one command they > are trying to add, and even if it is an iterative approach to get them > to find all the problems, it's a lot easier to have that one developer > fix their work than trying to bake error recovery into the parser. > > If you do add these two test cases (which I recommend), it should indeed > be two separate tests. Could be done on top. If I need to respin anyway, I'll add them in the first patch, of course. > Another thought - is it worth testing other valid JSON but invalid > schema constructs, such as: > > { 'enum': 1, 'data': false } Maybe, but I limited myself just to the parser in this series.
diff --git a/configure b/configure index 3d83e16..0a91434 100755 --- a/configure +++ b/configure @@ -4502,7 +4502,7 @@ if [ "$dtc_internal" = "yes" ]; then fi # build tree in object directory in case the source is not in the current directory -DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos" +DIRS="tests tests/tcg tests/tcg/cris tests/tcg/lm32 tests/libqos tests/qapi-schema" DIRS="$DIRS pc-bios/optionrom pc-bios/spapr-rtas pc-bios/s390-ccw" DIRS="$DIRS roms/seabios roms/vgabios" DIRS="$DIRS qapi-generated" diff --git a/tests/Makefile b/tests/Makefile index cdbb79e..89a467b 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -83,6 +83,12 @@ gcov-files-arm-y += hw/tmp105.c check-qtest-ppc-y += tests/boot-order-test$(EXESUF) check-qtest-ppc64-y += tests/boot-order-test$(EXESUF) +check-qapi-schema-y := $(addprefix tests/qapi-schema/, empty.json \ + funny-char.json indented-expr.json missing-colon.json \ + missing-comma.json non-objects.json \ + quoted-structural-chars.json unclosed-object.json \ + unclosed-string.json) + GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \ @@ -171,6 +177,7 @@ check-help: @echo " make check-qtest-TARGET Run qtest tests for given target" @echo " make check-qtest Run qtest tests" @echo " make check-unit Run qobject tests" + @echo " make check-qapi-schema Run QAPI schema tests" @echo " make check-block Run block tests" @echo " make check-report.html Generates an HTML test report" @echo @@ -233,13 +240,24 @@ check-report.html: check-report.xml check-tests/qemu-iotests-quick.sh: tests/qemu-iotests-quick.sh qemu-img$(EXESUF) qemu-io$(EXESUF) $< +.PHONY: check-tests/test-qapi.py +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 <$^ >$*.out 2>$*.err; echo $$? >$*.exit, " TEST $*.out") + @diff -q $(SRC_PATH)/$*.out $*.out + @diff -q $(SRC_PATH)/$*.err $*.err + @diff -q $(SRC_PATH)/$*.exit $*.exit + # Consolidated targets -.PHONY: check-qtest check-unit check +.PHONY: check-qapi-schema check-qtest check-unit check +check-qapi-schema: $(patsubst %,check-%, $(check-qapi-schema-y)) check-qtest: $(patsubst %,check-qtest-%, $(QTEST_TARGETS)) check-unit: $(patsubst %,check-%, $(check-unit-y)) check-block: $(patsubst %,check-%, $(check-block-y)) -check: check-unit check-qtest +check: check-qapi-schema check-unit check-qtest -include $(wildcard tests/*.d) -include $(wildcard tests/libqos/*.d) diff --git a/tests/qapi-schema/empty.err b/tests/qapi-schema/empty.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/empty.exit b/tests/qapi-schema/empty.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/empty.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/empty.json b/tests/qapi-schema/empty.json new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/empty.out b/tests/qapi-schema/empty.out new file mode 100644 index 0000000..b7f89a4 --- /dev/null +++ b/tests/qapi-schema/empty.out @@ -0,0 +1,3 @@ +[] +[] +[] diff --git a/tests/qapi-schema/funny-char.err b/tests/qapi-schema/funny-char.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/funny-char.exit b/tests/qapi-schema/funny-char.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/funny-char.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/funny-char.json b/tests/qapi-schema/funny-char.json new file mode 100644 index 0000000..d4973a2 --- /dev/null +++ b/tests/qapi-schema/funny-char.json @@ -0,0 +1,2 @@ +{ 'enum': 'Status', + 'data': [ 'good', 'bad', 'ugly' ]; } diff --git a/tests/qapi-schema/funny-char.out b/tests/qapi-schema/funny-char.out new file mode 100644 index 0000000..e3bd904 --- /dev/null +++ b/tests/qapi-schema/funny-char.out @@ -0,0 +1,3 @@ +[OrderedDict([('enum', 'Status'), ('data', ['good', 'bad', 'ugly'])])] +['Status'] +[] diff --git a/tests/qapi-schema/indented-expr.err b/tests/qapi-schema/indented-expr.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/indented-expr.exit b/tests/qapi-schema/indented-expr.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/indented-expr.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/indented-expr.json b/tests/qapi-schema/indented-expr.json new file mode 100644 index 0000000..d80af60 --- /dev/null +++ b/tests/qapi-schema/indented-expr.json @@ -0,0 +1,2 @@ +{ 'id' : 'eins' } + { 'id' : 'zwei' } diff --git a/tests/qapi-schema/indented-expr.out b/tests/qapi-schema/indented-expr.out new file mode 100644 index 0000000..98ae692 --- /dev/null +++ b/tests/qapi-schema/indented-expr.out @@ -0,0 +1,3 @@ +[OrderedDict([('id', 'eins')])] +[] +[] diff --git a/tests/qapi-schema/missing-colon.err b/tests/qapi-schema/missing-colon.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/missing-colon.exit b/tests/qapi-schema/missing-colon.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/missing-colon.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/missing-colon.json b/tests/qapi-schema/missing-colon.json new file mode 100644 index 0000000..6fc27ce --- /dev/null +++ b/tests/qapi-schema/missing-colon.json @@ -0,0 +1,2 @@ +{ 'enum' 'Status', + 'data': [ 'good', 'bad', 'ugly' ] } diff --git a/tests/qapi-schema/missing-colon.out b/tests/qapi-schema/missing-colon.out new file mode 100644 index 0000000..50f827e --- /dev/null +++ b/tests/qapi-schema/missing-colon.out @@ -0,0 +1,3 @@ +[OrderedDict([('enum', ','), ('data', ['good', 'bad', 'ugly'])])] +[','] +[] diff --git a/tests/qapi-schema/missing-comma.err b/tests/qapi-schema/missing-comma.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/missing-comma.exit b/tests/qapi-schema/missing-comma.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/missing-comma.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/missing-comma.json b/tests/qapi-schema/missing-comma.json new file mode 100644 index 0000000..50f5178 --- /dev/null +++ b/tests/qapi-schema/missing-comma.json @@ -0,0 +1,2 @@ +{ 'enum': 'Status' + 'data': [ 'good', 'bad', 'ugly' ] } diff --git a/tests/qapi-schema/missing-comma.out b/tests/qapi-schema/missing-comma.out new file mode 100644 index 0000000..e3bd904 --- /dev/null +++ b/tests/qapi-schema/missing-comma.out @@ -0,0 +1,3 @@ +[OrderedDict([('enum', 'Status'), ('data', ['good', 'bad', 'ugly'])])] +['Status'] +[] diff --git a/tests/qapi-schema/non-objects.err b/tests/qapi-schema/non-objects.err new file mode 100644 index 0000000..48c849d --- /dev/null +++ b/tests/qapi-schema/non-objects.err @@ -0,0 +1 @@ +Crashed: <type 'exceptions.AttributeError'> diff --git a/tests/qapi-schema/non-objects.exit b/tests/qapi-schema/non-objects.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/non-objects.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/non-objects.json b/tests/qapi-schema/non-objects.json new file mode 100644 index 0000000..f3fa851 --- /dev/null +++ b/tests/qapi-schema/non-objects.json @@ -0,0 +1,2 @@ +'string' +[ ] diff --git a/tests/qapi-schema/non-objects.out b/tests/qapi-schema/non-objects.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/quoted-structural-chars.err b/tests/qapi-schema/quoted-structural-chars.err new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/quoted-structural-chars.exit b/tests/qapi-schema/quoted-structural-chars.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/tests/qapi-schema/quoted-structural-chars.exit @@ -0,0 +1 @@ +0 diff --git a/tests/qapi-schema/quoted-structural-chars.json b/tests/qapi-schema/quoted-structural-chars.json new file mode 100644 index 0000000..9fe657a --- /dev/null +++ b/tests/qapi-schema/quoted-structural-chars.json @@ -0,0 +1 @@ +'{' 'key1' ':' 'value1' ',' 'key2' ':' '[' ']' '}' diff --git a/tests/qapi-schema/quoted-structural-chars.out b/tests/qapi-schema/quoted-structural-chars.out new file mode 100644 index 0000000..85405be --- /dev/null +++ b/tests/qapi-schema/quoted-structural-chars.out @@ -0,0 +1,3 @@ +[OrderedDict([('key1', 'value1'), ('key2', [])])] +[] +[] diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py new file mode 100644 index 0000000..3280eff --- /dev/null +++ b/tests/qapi-schema/test-qapi.py @@ -0,0 +1,25 @@ +# +# QAPI parser test harness +# +# Copyright (c) 2013 Red Hat Inc. +# +# Authors: +# Markus Armbruster <armbru@redhat.com> +# +# This work is licensed under the terms of the GNU GPL, version 2 or later. +# See the COPYING file in the top-level directory. +# + +from qapi import * +from pprint import pprint +import sys + +try: + exprs = parse_schema(sys.stdin) +except: + print >>sys.stderr, "Crashed:", sys.exc_info()[0] + exit(1) + +pprint(exprs) +pprint(enum_types) +pprint(struct_types) diff --git a/tests/qapi-schema/unclosed-object.err b/tests/qapi-schema/unclosed-object.err new file mode 100644 index 0000000..f9a9c2a --- /dev/null +++ b/tests/qapi-schema/unclosed-object.err @@ -0,0 +1 @@ +Crashed: <type 'exceptions.IndexError'> diff --git a/tests/qapi-schema/unclosed-object.exit b/tests/qapi-schema/unclosed-object.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/unclosed-object.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/unclosed-object.json b/tests/qapi-schema/unclosed-object.json new file mode 100644 index 0000000..97673d5 --- /dev/null +++ b/tests/qapi-schema/unclosed-object.json @@ -0,0 +1 @@ +{ 'key': [ 'value' diff --git a/tests/qapi-schema/unclosed-object.out b/tests/qapi-schema/unclosed-object.out new file mode 100644 index 0000000..e69de29 diff --git a/tests/qapi-schema/unclosed-string.err b/tests/qapi-schema/unclosed-string.err new file mode 100644 index 0000000..5af46c2 --- /dev/null +++ b/tests/qapi-schema/unclosed-string.err @@ -0,0 +1 @@ +Crashed: <type 'exceptions.Exception'> diff --git a/tests/qapi-schema/unclosed-string.exit b/tests/qapi-schema/unclosed-string.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/tests/qapi-schema/unclosed-string.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/unclosed-string.json b/tests/qapi-schema/unclosed-string.json new file mode 100644 index 0000000..8c16b6b --- /dev/null +++ b/tests/qapi-schema/unclosed-string.json @@ -0,0 +1,2 @@ +{ 'text': 'lorem ips +}
The parser handles erroneous input badly. To be improved shortly. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- configure | 2 +- tests/Makefile | 22 ++++++++++++++++++++-- tests/qapi-schema/empty.exit | 1 + tests/qapi-schema/empty.out | 3 +++ tests/qapi-schema/funny-char.exit | 1 + tests/qapi-schema/funny-char.json | 2 ++ tests/qapi-schema/funny-char.out | 3 +++ tests/qapi-schema/indented-expr.exit | 1 + tests/qapi-schema/indented-expr.json | 2 ++ tests/qapi-schema/indented-expr.out | 3 +++ tests/qapi-schema/missing-colon.exit | 1 + tests/qapi-schema/missing-colon.json | 2 ++ tests/qapi-schema/missing-colon.out | 3 +++ tests/qapi-schema/missing-comma.exit | 1 + tests/qapi-schema/missing-comma.json | 2 ++ tests/qapi-schema/missing-comma.out | 3 +++ tests/qapi-schema/non-objects.err | 1 + tests/qapi-schema/non-objects.exit | 1 + tests/qapi-schema/non-objects.json | 2 ++ tests/qapi-schema/quoted-structural-chars.exit | 1 + tests/qapi-schema/quoted-structural-chars.json | 1 + tests/qapi-schema/quoted-structural-chars.out | 3 +++ tests/qapi-schema/test-qapi.py | 25 +++++++++++++++++++++++++ tests/qapi-schema/unclosed-object.err | 1 + tests/qapi-schema/unclosed-object.exit | 1 + tests/qapi-schema/unclosed-object.json | 1 + tests/qapi-schema/unclosed-string.err | 1 + tests/qapi-schema/unclosed-string.exit | 1 + tests/qapi-schema/unclosed-string.json | 2 ++ 29 files changed, 90 insertions(+), 3 deletions(-) create mode 100644 tests/qapi-schema/empty.err create mode 100644 tests/qapi-schema/empty.exit create mode 100644 tests/qapi-schema/empty.json create mode 100644 tests/qapi-schema/empty.out create mode 100644 tests/qapi-schema/funny-char.err create mode 100644 tests/qapi-schema/funny-char.exit create mode 100644 tests/qapi-schema/funny-char.json create mode 100644 tests/qapi-schema/funny-char.out create mode 100644 tests/qapi-schema/indented-expr.err create mode 100644 tests/qapi-schema/indented-expr.exit create mode 100644 tests/qapi-schema/indented-expr.json create mode 100644 tests/qapi-schema/indented-expr.out create mode 100644 tests/qapi-schema/missing-colon.err create mode 100644 tests/qapi-schema/missing-colon.exit create mode 100644 tests/qapi-schema/missing-colon.json create mode 100644 tests/qapi-schema/missing-colon.out create mode 100644 tests/qapi-schema/missing-comma.err create mode 100644 tests/qapi-schema/missing-comma.exit create mode 100644 tests/qapi-schema/missing-comma.json create mode 100644 tests/qapi-schema/missing-comma.out create mode 100644 tests/qapi-schema/non-objects.err create mode 100644 tests/qapi-schema/non-objects.exit create mode 100644 tests/qapi-schema/non-objects.json create mode 100644 tests/qapi-schema/non-objects.out create mode 100644 tests/qapi-schema/quoted-structural-chars.err create mode 100644 tests/qapi-schema/quoted-structural-chars.exit create mode 100644 tests/qapi-schema/quoted-structural-chars.json create mode 100644 tests/qapi-schema/quoted-structural-chars.out create mode 100644 tests/qapi-schema/test-qapi.py create mode 100644 tests/qapi-schema/unclosed-object.err create mode 100644 tests/qapi-schema/unclosed-object.exit create mode 100644 tests/qapi-schema/unclosed-object.json create mode 100644 tests/qapi-schema/unclosed-object.out create mode 100644 tests/qapi-schema/unclosed-string.err create mode 100644 tests/qapi-schema/unclosed-string.exit create mode 100644 tests/qapi-schema/unclosed-string.json create mode 100644 tests/qapi-schema/unclosed-string.out diff --git a/tests/qapi-schema/unclosed-string.out b/tests/qapi-schema/unclosed-string.out new file mode 100644 index 0000000..e69de29