Message ID | 1488871237-12332-1-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
Hi, This series seems to have some coding style problems. See output below for more information: Message-id: 1488871237-12332-1-git-send-email-armbru@redhat.com Subject: [Qemu-devel] [PULL v3 00/24] block: Command line option -blockdev Type: series === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 # Useful git options git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1488871237-12332-1-git-send-email-armbru@redhat.com -> patchew/1488871237-12332-1-git-send-email-armbru@redhat.com Switched to a new branch 'test' a539942 keyval: Support lists 3338f18 docs/qapi-code-gen.txt: Clarify naming rules 7335175 qapi: Improve how keyval input visitor reports unexpected dicts b79d59d block: Initial implementation of -blockdev 5a193ff qapi: New qobject_input_visitor_new_str() for convenience 084ecaf keyval: Restrict key components to valid QAPI names 02418a9 qapi: New parse_qapi_name() fcd2922 test-qapi-util: New, covering qapi/qapi-util.c 5a344d5 monitor: Assert qmp_schema_json[] is sane 1e7f77e test-visitor-serialization: Pass &error_abort to qobject_from_json() d6bdd79 check-qjson: Test errors from qobject_from_json() 923f8ae block: More detailed syntax error reporting for JSON filenames 0e60bb2 qobject: Propagate parse errors through qobject_from_json() ed7a2ec test-qobject-input-visitor: Abort earlier on bad test input 858bb50 qjson: Abort earlier on qobject_from_jsonf() misuse 24fc55d libqtest: Fix qmp() & friends to abort on JSON parse errors 365cf63 qobject: Propagate parse errors through qobject_from_jsonv() cf24199 qapi: Factor out common qobject_input_get_keyval() cc41e68 qapi: Factor out common part of qobject input visitor creation 6d5197b test-keyval: Cover use with qobject input visitor de50c3a qapi: qobject input visitor variant for use with keyval_parse() 2272cd0 keyval: New keyval_parse() b4218d4 tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y 7f341b4 test-qemu-opts: Cover qemu_opts_parse() of "no" === OUTPUT BEGIN === Checking PATCH 1/24: test-qemu-opts: Cover qemu_opts_parse() of "no"... Checking PATCH 2/24: tests: Fix gcov-files-test-qemu-opts-y, gcov-files-test-logging-y... Checking PATCH 3/24: keyval: New keyval_parse()... ERROR: suspect code indent for conditional statements (8, 8) #438: FILE: util/keyval.c:140: + for (len = 0; s + len < key_end && s[len] != '.'; len++) { + } total: 1 errors, 0 warnings, 447 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 4/24: qapi: qobject input visitor variant for use with keyval_parse()... Checking PATCH 5/24: test-keyval: Cover use with qobject input visitor... Checking PATCH 6/24: qapi: Factor out common part of qobject input visitor creation... Checking PATCH 7/24: qapi: Factor out common qobject_input_get_keyval()... Checking PATCH 8/24: qobject: Propagate parse errors through qobject_from_jsonv()... Checking PATCH 9/24: libqtest: Fix qmp() & friends to abort on JSON parse errors... Checking PATCH 10/24: qjson: Abort earlier on qobject_from_jsonf() misuse... Checking PATCH 11/24: test-qobject-input-visitor: Abort earlier on bad test input... Checking PATCH 12/24: qobject: Propagate parse errors through qobject_from_json()... Checking PATCH 13/24: block: More detailed syntax error reporting for JSON filenames... Checking PATCH 14/24: check-qjson: Test errors from qobject_from_json()... Checking PATCH 15/24: test-visitor-serialization: Pass &error_abort to qobject_from_json()... Checking PATCH 16/24: monitor: Assert qmp_schema_json[] is sane... Checking PATCH 17/24: test-qapi-util: New, covering qapi/qapi-util.c... Checking PATCH 18/24: qapi: New parse_qapi_name()... Checking PATCH 19/24: keyval: Restrict key components to valid QAPI names... Checking PATCH 20/24: qapi: New qobject_input_visitor_new_str() for convenience... Checking PATCH 21/24: block: Initial implementation of -blockdev... Checking PATCH 22/24: qapi: Improve how keyval input visitor reports unexpected dicts... Checking PATCH 23/24: docs/qapi-code-gen.txt: Clarify naming rules... Checking PATCH 24/24: keyval: Support lists... ERROR: spaces required around that '-' (ctx:VxV) #444: FILE: util/keyval.c:345: + assert(!elt[nelt-1]); /* need the sentinel to be null */ ^ total: 1 errors, 0 warnings, 420 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
On 7 March 2017 at 07:20, Markus Armbruster <armbru@redhat.com> wrote: > Actually, the command line option is the least part of this series. > Its bulk is about building infrastructure and getting errors out of > the JSON parser. > > The design of the command line interface was discussed here: > Subject: Non-flat command line option argument syntax > Message-ID: <87bmukmlau.fsf@dusky.pond.sub.org> > https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00555.html > > v3: A few commit messages touched up, code unchanged > > The following changes since commit fbddc2e5608eb655493253d080598375db61a748: > > Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2017-02-28' into staging (2017-03-06 10:18:33 +0000) > > are available in the git repository at: > > git://repo.or.cz/qemu/armbru.git tags/pull-block-2017-02-28-v3 > > for you to fetch changes up to de479f01438d974100ac9ec10ab82c2ee1ad124f: > > keyval: Support lists (2017-03-07 08:15:18 +0100) The clang sanitizer has a complaint about your new test: GTESTER tests/test-keyval /home/petmay01/linaro/qemu-for-merges/util/keyval.c:129:15: runtime error: member access within null pointer of type 'QString' (aka 'struct QString') which is the line: new = QOBJECT(value) ?: QOBJECT(qdict_new()); This is because QOBJECT is defined as: #define QOBJECT(obj) (&(obj)->base) and so it doesn't have the "if you pass in NULL you get NULL" semantics that OBJECT() and all the QOM object typecast macros do. (As an aside, it's rather confusing to have a QObject whose header claims it's part of the "QEMU Object Model" which is completely different from the QOM Object...) thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On 7 March 2017 at 07:20, Markus Armbruster <armbru@redhat.com> wrote: >> Actually, the command line option is the least part of this series. >> Its bulk is about building infrastructure and getting errors out of >> the JSON parser. >> >> The design of the command line interface was discussed here: >> Subject: Non-flat command line option argument syntax >> Message-ID: <87bmukmlau.fsf@dusky.pond.sub.org> >> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00555.html >> >> v3: A few commit messages touched up, code unchanged >> >> The following changes since commit fbddc2e5608eb655493253d080598375db61a748: >> >> Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2017-02-28' into staging (2017-03-06 10:18:33 +0000) >> >> are available in the git repository at: >> >> git://repo.or.cz/qemu/armbru.git tags/pull-block-2017-02-28-v3 >> >> for you to fetch changes up to de479f01438d974100ac9ec10ab82c2ee1ad124f: >> >> keyval: Support lists (2017-03-07 08:15:18 +0100) > > The clang sanitizer has a complaint about your new test: > > GTESTER tests/test-keyval > /home/petmay01/linaro/qemu-for-merges/util/keyval.c:129:15: runtime > error: member access within null pointer of type 'QString' (aka > 'struct QString') > > which is the line: > new = QOBJECT(value) ?: QOBJECT(qdict_new()); > > This is because QOBJECT is defined as: > > #define QOBJECT(obj) (&(obj)->base) > > and so it doesn't have the "if you pass in NULL you get NULL" > semantics that OBJECT() and all the QOM object typecast > macros do. QOBJECT() returns its argument, so this is rather pedantic. However, placating the sanitize isn't too onerous; this should do: new = value ? QOBJECT(value) : QOBJECT(qdict_new()); > (As an aside, it's rather confusing to have a QObject > whose header claims it's part of the "QEMU Object Model" > which is completely different from the QOM Object...) Hey, QObject claimed the name first! Seriously, I hate the name clash, too. Rewording the comment is easy enough. Even better would be renaming QObject, but that's a lot of churn.