| Message ID | 20170909054613.19148-1-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PULL v2 00/37] Staging patches
Message-id: 20170909054613.19148-1-famz@redhat.com
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
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
t [tag update] patchew/1504815384-8490-1-git-send-email-stefanb@linux.vnet.ibm.com -> patchew/1504815384-8490-1-git-send-email-stefanb@linux.vnet.ibm.com
t [tag update] patchew/20170908163859.29820-1-richard.henderson@linaro.org -> patchew/20170908163859.29820-1-richard.henderson@linaro.org
* [new tag] patchew/20170909054613.19148-1-famz@redhat.com -> patchew/20170909054613.19148-1-famz@redhat.com
Switched to a new branch 'test'
274b0b33cb buildsys: Move rdma libs to per object
dee13ca0ca buildsys: Move brlapi libs to per object
51486e4caf buildsys: Move usb redir cflags/libs to per object
e2d2141400 buildsys: Move libusb cflags/libs to per object
e653260901 buildsys: Move libcacard cflags/libs to per object
de80133478 buildsys: Move curese cflags/libs to per object
4da943d9c6 buildsys: Move audio libs to per object
c05be913ea buildsys: Move vnc cflags/libs to per object
066db85c1a buildsys: Move sdl cflags/libs to per object
50200dfb4c buildsys: Move gtk/vte cflags/libs to per object
0832346d7d buildsys: Move vde libs to per object
5cad9f269f vl: Don't include vde header
d642c77e93 docker: Drop 'set -e' from run script
05cb7137d9 docker: Add test-block
f829f5d141 docker: Add nettle-devel to fedora image
ca0c6ffecc docker: Use unconfined security profile
c5a6c192c7 docker: Add test_fail and prep_fail
ee94d2b7c9 docker: Fix return code of build_qemu()
0717923a55 docker: Use archive-source.py
b6a02aee1a tests: Add README for vm tests
f0bfdb4c3b MAINTAINERS: Add tests/vm entry
133ce1feb5 Makefile: Add rules to run vm tests
f88480f6c8 tests: Add OpenBSD image
d05508ab23 tests: Add NetBSD image
8370cb8f2f tests: Add FreeBSD image
a8b70bfa3b tests: Add ubuntu.i386 image
413861ba2d tests: Add vm test lib
e10c67002d scripts: Add archive-source.sh
c353b7acd2 qemu.py: Add "wait()" method
fac58fe6c0 gitignore: Ignore vm test images
1022469bf7 tests/docker: Clean up paths
e6dde9693e docker: Enable features explicitly in test-full
f08e8a2903 docker: Update ubuntu image
6c2c01bbe2 docker: reduce noise when building travis.docker
f63179c5a0 docker: don't install device-tree-compiler build-deps in travis.docker
f9d3a9d011 docker: docker.py make --no-cache skip checksum test
3695d96c65 docker: ensure NOUSER for travis images
=== OUTPUT BEGIN ===
Checking PATCH 1/37: docker: ensure NOUSER for travis images...
Checking PATCH 2/37: docker: docker.py make --no-cache skip checksum test...
Checking PATCH 3/37: docker: don't install device-tree-compiler build-deps in travis.docker...
Checking PATCH 4/37: docker: reduce noise when building travis.docker...
Checking PATCH 5/37: docker: Update ubuntu image...
Checking PATCH 6/37: docker: Enable features explicitly in test-full...
Checking PATCH 7/37: tests/docker: Clean up paths...
Checking PATCH 8/37: gitignore: Ignore vm test images...
Checking PATCH 9/37: qemu.py: Add "wait()" method...
Checking PATCH 10/37: scripts: Add archive-source.sh...
Checking PATCH 11/37: tests: Add vm test lib...
ERROR: line over 90 characters
#86: FILE: tests/vm/basevm.py:60:
+ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCikC46WYtXotUd0UGPz9547Aj0KqC4gk+nt4BBJm86IHgCD9FygSGX9EFutXlhz9KZIPg9Okk7+IzXRHCWI2MNvhrcjyrezKREm71z08j9iwfxY3340fY2Mo+0khwpO7bzsgzkljHIHqcOg7MgttPInVMNH/EfqpgR8EDKJuWCB2Ny+EBFN/3dAiff0X/EvKle9PUrY70EkSycnyURS8HZReEqj8lN9J5kXzA8F6jBo/0Q42Ttv6e4k5YcaDrwmLrBWLra2PCXZLNyHqXEiFkGmdXtA1Eox9gc/p4jIXim6xrPNmpN6WyrrEjaCF5xYvNv8wXkD6uSWwbHYU24lIAn qemu-vm-key
WARNING: line over 80 characters
#100: FILE: tests/vm/basevm.py:74:
+ self._tmpdir = tempfile.mkdtemp(prefix="vm-test-", suffix=".tmp", dir=".")
WARNING: line over 80 characters
#191: FILE: tests/vm/basevm.py:165:
+ logging.debug("Creating archive %s for src_dir dir: %s", tarfile, src_dir)
WARNING: line over 80 characters
#196: FILE: tests/vm/basevm.py:170:
+ "file=%s,if=none,id=%s,cache=writeback,format=raw" % \
WARNING: line over 80 characters
#199: FILE: tests/vm/basevm.py:173:
+ "virtio-blk,drive=%s,serial=%s,bootindex=1" % (name, name)]
ERROR: line over 90 characters
#246: FILE: tests/vm/basevm.py:220:
+ VM test utility. Exit codes: 0 = success, 1 = command line error, 2 = environment initialization failed, 3 = test command failed""")
WARNING: line over 80 characters
#253: FILE: tests/vm/basevm.py:227:
+ parser.add_option("--jobs", type=int, default=multiprocessing.cpu_count() / 2,
total: 2 errors, 5 warnings, 276 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 12/37: tests: Add ubuntu.i386 image...
Checking PATCH 13/37: tests: Add FreeBSD image...
Checking PATCH 14/37: tests: Add NetBSD image...
Checking PATCH 15/37: tests: Add OpenBSD image...
Checking PATCH 16/37: Makefile: Add rules to run vm tests...
Checking PATCH 17/37: MAINTAINERS: Add tests/vm entry...
Checking PATCH 18/37: tests: Add README for vm tests...
Checking PATCH 19/37: docker: Use archive-source.py...
Checking PATCH 20/37: docker: Fix return code of build_qemu()...
Checking PATCH 21/37: docker: Add test_fail and prep_fail...
Checking PATCH 22/37: docker: Use unconfined security profile...
Checking PATCH 23/37: docker: Add nettle-devel to fedora image...
Checking PATCH 24/37: docker: Add test-block...
Checking PATCH 25/37: docker: Drop 'set -e' from run script...
Checking PATCH 26/37: vl: Don't include vde header...
Checking PATCH 27/37: buildsys: Move vde libs to per object...
Checking PATCH 28/37: buildsys: Move gtk/vte cflags/libs to per object...
Checking PATCH 29/37: buildsys: Move sdl cflags/libs to per object...
Checking PATCH 30/37: buildsys: Move vnc cflags/libs to per object...
Checking PATCH 31/37: buildsys: Move audio libs to per object...
Checking PATCH 32/37: buildsys: Move curese cflags/libs to per object...
Checking PATCH 33/37: buildsys: Move libcacard cflags/libs to per object...
Checking PATCH 34/37: buildsys: Move libusb cflags/libs to per object...
Checking PATCH 35/37: buildsys: Move usb redir cflags/libs to per object...
Checking PATCH 36/37: buildsys: Move brlapi libs to per object...
Checking PATCH 37/37: buildsys: Move rdma libs to per object...
=== 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 9 September 2017 at 06:45, Fam Zheng <famz@redhat.com> wrote: > Signed-off-by: Fam Zheng <famz@redhat.com> > Message-Id: <20170905021201.25684-4-famz@redhat.com> > --- > scripts/archive-source.sh | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > create mode 100755 scripts/archive-source.sh > > diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh > new file mode 100755 > index 0000000000..1de369532e > --- /dev/null > +++ b/scripts/archive-source.sh > @@ -0,0 +1,31 @@ > +#!/bin/sh > +# > +# Author: Fam Zheng <famz@redhat.com> > +# > +# Create archive of source tree, including submodules > +# > +# This code is licensed under the GPL version 2 or later. See > +# the COPYING file in the top-level directory. So is this the script that for instance Mike Roth would use to create the release tarballs? If it isn't, should it be? Is it intended for end users to create tarballs with, or is it really just a helper script for the docker stuff? If the latter, it would be helpful to say so. If the former, it could really use more usage information/documentation... > + > +set -e > + > +if test $# -lt 1; then > + echo "Usage: $0 <output>" > + exit 1 > +fi > + > +submodules=$(git submodule foreach --recursive --quiet 'echo $name') > + > +if test -n "$submodules"; then > + { > + git ls-files > + for sm in $submodules; do > + (cd $sm; git ls-files) | sed "s:^:$sm/:" > + done > + } | grep -x -v $(for sm in $submodules; do echo "-e $sm"; done) > $1.list Supporting '-e something' in a tar -T listfile seems to be GNU tar specific. Do we care? > +else > + git ls-files > $1.list > +fi This will blow up if we ever have a file in the repo that starts with a '-'. Do we care? > + > +tar -cf $1 -T $1.list > +rm $1.list This is missing a lot of quoting for $1, so it will go wrong if there's a space in that filename argument. thanks -- PMM
On Sat, 09/09 13:07, Peter Maydell wrote: > On 9 September 2017 at 06:45, Fam Zheng <famz@redhat.com> wrote: > > Signed-off-by: Fam Zheng <famz@redhat.com> > > Message-Id: <20170905021201.25684-4-famz@redhat.com> > > --- > > scripts/archive-source.sh | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > create mode 100755 scripts/archive-source.sh > > > > diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh > > new file mode 100755 > > index 0000000000..1de369532e > > --- /dev/null > > +++ b/scripts/archive-source.sh > > @@ -0,0 +1,31 @@ > > +#!/bin/sh > > +# > > +# Author: Fam Zheng <famz@redhat.com> > > +# > > +# Create archive of source tree, including submodules > > +# > > +# This code is licensed under the GPL version 2 or later. See > > +# the COPYING file in the top-level directory. > > So is this the script that for instance Mike Roth would use > to create the release tarballs? If it isn't, should it be? I don't know the workflow of release tarballs, there is ./scripts/make-release for that. So this one is not. > Is it intended for end users to create tarballs with, or > is it really just a helper script for the docker stuff? > If the latter, it would be helpful to say so. If the former, > it could really use more usage information/documentation... I'm not sure what end users would use this for, but it should do its work just the same.. What kind of usage information do you suggest? More explaination in the header, or in the "usage" output when no target is specified, or a "-h" option for help? > > > + > > +set -e > > + > > +if test $# -lt 1; then > > + echo "Usage: $0 <output>" > > + exit 1 > > +fi > > + > > +submodules=$(git submodule foreach --recursive --quiet 'echo $name') > > + > > +if test -n "$submodules"; then > > + { > > + git ls-files > > + for sm in $submodules; do > > + (cd $sm; git ls-files) | sed "s:^:$sm/:" > > + done > > + } | grep -x -v $(for sm in $submodules; do echo "-e $sm"; done) > $1.list > > Supporting '-e something' in a tar -T listfile seems to > be GNU tar specific. Do we care? The "-e $sm" will go to the grep command line, so it isn't GNU specific, is it? > > > +else > > + git ls-files > $1.list > > +fi > > This will blow up if we ever have a file in the repo > that starts with a '-'. Do we care? I'm sure such a file will be a trouble, but I'd rather we deal with it when there is one: I don't think anyone will think about adding, or welcome such a file. > > > + > > +tar -cf $1 -T $1.list > > +rm $1.list > > This is missing a lot of quoting for $1, so it will go wrong > if there's a space in that filename argument. Yes, good point. I will fix it before sending another pull request along with other comments that are just raised. Fam
On 09.09.2017 17:07, Fam Zheng wrote: > On Sat, 09/09 13:07, Peter Maydell wrote: >> On 9 September 2017 at 06:45, Fam Zheng <famz@redhat.com> wrote: >>> Signed-off-by: Fam Zheng <famz@redhat.com> >>> Message-Id: <20170905021201.25684-4-famz@redhat.com> >>> --- >>> scripts/archive-source.sh | 31 +++++++++++++++++++++++++++++++ >>> 1 file changed, 31 insertions(+) >>> create mode 100755 scripts/archive-source.sh >>> >>> diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh >>> new file mode 100755 >>> index 0000000000..1de369532e >>> --- /dev/null >>> +++ b/scripts/archive-source.sh >>> @@ -0,0 +1,31 @@ >>> +#!/bin/sh >>> +# >>> +# Author: Fam Zheng <famz@redhat.com> >>> +# >>> +# Create archive of source tree, including submodules >>> +# >>> +# This code is licensed under the GPL version 2 or later. See >>> +# the COPYING file in the top-level directory. >> >> So is this the script that for instance Mike Roth would use >> to create the release tarballs? If it isn't, should it be? > > I don't know the workflow of release tarballs, there is ./scripts/make-release > for that. So this one is not. > >> Is it intended for end users to create tarballs with, or >> is it really just a helper script for the docker stuff? >> If the latter, it would be helpful to say so. If the former, >> it could really use more usage information/documentation... > > I'm not sure what end users would use this for, but it should do its work just > the same.. What kind of usage information do you suggest? More explaination in > the header, or in the "usage" output when no target is specified, or a "-h" > option for help? > >> >>> + >>> +set -e >>> + >>> +if test $# -lt 1; then >>> + echo "Usage: $0 <output>" >>> + exit 1 >>> +fi >>> + >>> +submodules=$(git submodule foreach --recursive --quiet 'echo $name') >>> + >>> +if test -n "$submodules"; then >>> + { >>> + git ls-files >>> + for sm in $submodules; do >>> + (cd $sm; git ls-files) | sed "s:^:$sm/:" >>> + done >>> + } | grep -x -v $(for sm in $submodules; do echo "-e $sm"; done) > $1.list >> >> Supporting '-e something' in a tar -T listfile seems to >> be GNU tar specific. Do we care? > > The "-e $sm" will go to the grep command line, so it isn't GNU specific, is it? > BSD OSes use GNU grep(1) the latest version GPLv2. There is a work in progress BSD grep(1), but not turned on by default as it's slower. The -e option is supported by SmartOS grep(1). >> >>> +else >>> + git ls-files > $1.list >>> +fi >> >> This will blow up if we ever have a file in the repo >> that starts with a '-'. Do we care? > > I'm sure such a file will be a trouble, but I'd rather we deal with it when > there is one: I don't think anyone will think about adding, or welcome such a > file. > >> >>> + >>> +tar -cf $1 -T $1.list >>> +rm $1.list >> >> This is missing a lot of quoting for $1, so it will go wrong >> if there's a space in that filename argument. > > Yes, good point. I will fix it before sending another pull request along with > other comments that are just raised. > > Fam >
On 9 September 2017 at 16:07, Fam Zheng <famz@redhat.com> wrote: > On Sat, 09/09 13:07, Peter Maydell wrote: >> On 9 September 2017 at 06:45, Fam Zheng <famz@redhat.com> wrote: >> > Signed-off-by: Fam Zheng <famz@redhat.com> >> > Message-Id: <20170905021201.25684-4-famz@redhat.com> >> > --- >> > scripts/archive-source.sh | 31 +++++++++++++++++++++++++++++++ >> > 1 file changed, 31 insertions(+) >> > create mode 100755 scripts/archive-source.sh >> > >> > diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh >> > new file mode 100755 >> > index 0000000000..1de369532e >> > --- /dev/null >> > +++ b/scripts/archive-source.sh >> > @@ -0,0 +1,31 @@ >> > +#!/bin/sh >> > +# >> > +# Author: Fam Zheng <famz@redhat.com> >> > +# >> > +# Create archive of source tree, including submodules >> > +# >> > +# This code is licensed under the GPL version 2 or later. See >> > +# the COPYING file in the top-level directory. >> >> So is this the script that for instance Mike Roth would use >> to create the release tarballs? If it isn't, should it be? > > I don't know the workflow of release tarballs, there is ./scripts/make-release > for that. So this one is not. > >> Is it intended for end users to create tarballs with, or >> is it really just a helper script for the docker stuff? >> If the latter, it would be helpful to say so. If the former, >> it could really use more usage information/documentation... > > I'm not sure what end users would use this for, but it should do its work just > the same.. What kind of usage information do you suggest? More explaination in > the header, or in the "usage" output when no target is specified, or a "-h" > option for help? Basically, if it's a special purpose script that is only being used by docker, then it should clearly say that so that anybody who looks at it will know it's not something relevant to them. If it's not special purpose then it should say what it's for and how you use it. Just putting this in comments at the top of the script is fine. thanks -- PMM
On Sat, 09/09 17:08, Kamil Rytarowski wrote: > >>> +submodules=$(git submodule foreach --recursive --quiet 'echo $name') > >>> + > >>> +if test -n "$submodules"; then > >>> + { > >>> + git ls-files > >>> + for sm in $submodules; do > >>> + (cd $sm; git ls-files) | sed "s:^:$sm/:" > >>> + done > >>> + } | grep -x -v $(for sm in $submodules; do echo "-e $sm"; done) > $1.list > >> > >> Supporting '-e something' in a tar -T listfile seems to > >> be GNU tar specific. Do we care? > > > > The "-e $sm" will go to the grep command line, so it isn't GNU specific, is it? > > > > BSD OSes use GNU grep(1) the latest version GPLv2. There is a work in > progress BSD grep(1), but not turned on by default as it's slower. > > The -e option is supported by SmartOS grep(1). Is there a more portable way to filter out multiple patterns? Fam
On 10.09.2017 04:05, Fam Zheng wrote: > On Sat, 09/09 17:08, Kamil Rytarowski wrote: >>>>> +submodules=$(git submodule foreach --recursive --quiet 'echo $name') >>>>> + >>>>> +if test -n "$submodules"; then >>>>> + { >>>>> + git ls-files >>>>> + for sm in $submodules; do >>>>> + (cd $sm; git ls-files) | sed "s:^:$sm/:" >>>>> + done >>>>> + } | grep -x -v $(for sm in $submodules; do echo "-e $sm"; done) > $1.list >>>> >>>> Supporting '-e something' in a tar -T listfile seems to >>>> be GNU tar specific. Do we care? >>> >>> The "-e $sm" will go to the grep command line, so it isn't GNU specific, is it? >>> >> >> BSD OSes use GNU grep(1) the latest version GPLv2. There is a work in >> progress BSD grep(1), but not turned on by default as it's slower. >> >> The -e option is supported by SmartOS grep(1). > > Is there a more portable way to filter out multiple patterns? > > Fam > I don't think that there is needed more portability. If so, there is an option to detect "ggrep" (GNU grep) and if it exists - use it.
The following changes since commit fcea73709b966a7ded9efa7b106ea50c7fe9025c: Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2017-09-08 16:04:42 +0100) are available in the git repository at: git://github.com/famz/qemu.git tags/staging-pull-request for you to fetch changes up to 734e2410b1c14a4cf17b1500bda1dcc7e7e57cfc: buildsys: Move rdma libs to per object (2017-09-09 11:55:51 +0800) ---------------------------------------------------------------- v2: Drop opengl flags patch. Change the new scripts' licenses to GPLv2+. ---------------------------------------------------------------- Alex Bennée (4): docker: ensure NOUSER for travis images docker: docker.py make --no-cache skip checksum test docker: don't install device-tree-compiler build-deps in travis.docker docker: reduce noise when building travis.docker Fam Zheng (33): docker: Update ubuntu image docker: Enable features explicitly in test-full tests/docker: Clean up paths gitignore: Ignore vm test images qemu.py: Add "wait()" method scripts: Add archive-source.sh tests: Add vm test lib tests: Add ubuntu.i386 image tests: Add FreeBSD image tests: Add NetBSD image tests: Add OpenBSD image Makefile: Add rules to run vm tests MAINTAINERS: Add tests/vm entry tests: Add README for vm tests docker: Use archive-source.py docker: Fix return code of build_qemu() docker: Add test_fail and prep_fail docker: Use unconfined security profile docker: Add nettle-devel to fedora image docker: Add test-block docker: Drop 'set -e' from run script vl: Don't include vde header buildsys: Move vde libs to per object buildsys: Move gtk/vte cflags/libs to per object buildsys: Move sdl cflags/libs to per object buildsys: Move vnc cflags/libs to per object buildsys: Move audio libs to per object buildsys: Move curese cflags/libs to per object buildsys: Move libcacard cflags/libs to per object buildsys: Move libusb cflags/libs to per object buildsys: Move usb redir cflags/libs to per object buildsys: Move brlapi libs to per object buildsys: Move rdma libs to per object .gitignore | 1 + MAINTAINERS | 1 + Makefile | 2 + audio/Makefile.objs | 6 + chardev/Makefile.objs | 1 + configure | 61 ++++---- hw/usb/Makefile.objs | 13 +- migration/Makefile.objs | 1 + net/Makefile.objs | 2 + scripts/archive-source.sh | 31 ++++ scripts/qemu.py | 7 + tests/.gitignore | 1 + tests/docker/Makefile.include | 17 +- tests/docker/common.rc | 20 ++- tests/docker/docker.py | 3 +- tests/docker/dockerfiles/fedora.docker | 1 + tests/docker/dockerfiles/travis.docker | 6 +- tests/docker/dockerfiles/ubuntu.docker | 11 +- tests/docker/run | 18 +-- tests/docker/test-block | 21 +++ tests/docker/test-full | 82 +++++++++- tests/vm/Makefile.include | 42 +++++ tests/vm/README | 63 ++++++++ tests/vm/basevm.py | 276 +++++++++++++++++++++++++++++++++ tests/vm/freebsd | 42 +++++ tests/vm/netbsd | 42 +++++ tests/vm/openbsd | 43 +++++ tests/vm/ubuntu.i386 | 88 +++++++++++ ui/Makefile.objs | 29 ++-- vl.c | 4 - 30 files changed, 850 insertions(+), 85 deletions(-) create mode 100755 scripts/archive-source.sh create mode 100755 tests/docker/test-block create mode 100644 tests/vm/Makefile.include create mode 100644 tests/vm/README create mode 100755 tests/vm/basevm.py create mode 100755 tests/vm/freebsd create mode 100755 tests/vm/netbsd create mode 100755 tests/vm/openbsd create mode 100755 tests/vm/ubuntu.i386