| Message ID | 1474919934-21518-1-git-send-email-riku.voipio@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1474919934-21518-1-git-send-email-riku.voipio@linaro.org
Subject: [Qemu-devel] [PATCH] checkpatch.pl: disable arch-specific test for linux-user
=== 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 show --no-patch --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/1474919934-21518-1-git-send-email-riku.voipio@linaro.org -> patchew/1474919934-21518-1-git-send-email-riku.voipio@linaro.org
Switched to a new branch 'test'
6d30677 checkpatch.pl: disable arch-specific test for linux-user
=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch.pl: disable arch-specific test for linux-user...
ERROR: line over 90 characters
#24: FILE: scripts/checkpatch.pl:2410:
+ if (!($realfile =~ /^(linux|bsd)-user/) && $line =~ m@^.\s*\#\s*if.*\b__@) {
total: 1 errors, 0 warnings, 11 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 26 September 2016 at 12:58, <riku.voipio@linaro.org> wrote: > From: Riku Voipio <riku.voipio@linaro.org> > > Linux-user and bsd-user code needs lots of arch-specific ifdefs, > so disable the warning. > > Signed-off-by: Riku Voipio <riku.voipio@linaro.org> > --- > scripts/checkpatch.pl | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index dde3f5f..98a007f 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2405,8 +2405,9 @@ sub process { > } > # check of hardware specific defines > # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases > -# where they might be necessary. > - if ($line =~ m@^.\s*\#\s*if.*\b__@) { > +# where they might be necessary. Skip test on linux-user and bsd-user > +# where arch defines are needed > + if (!($realfile =~ /^(linux|bsd)-user/) && $line =~ m@^.\s*\#\s*if.*\b__@) { > ERROR("architecture specific defines should be avoided\n" . $herecurr); > } Do you have some examples of the false positives you want to suppress here? For new code I would hope that we can handle host-arch-specifics by having new files (or just new #defines etc) in linux-user/host/$ARCH/ rather than inline #ifdeffery in the main files. thanks -- PMM
On 27 September 2016 at 00:08, Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 September 2016 at 12:58, <riku.voipio@linaro.org> wrote: >> From: Riku Voipio <riku.voipio@linaro.org> >> >> Linux-user and bsd-user code needs lots of arch-specific ifdefs, >> so disable the warning. >> >> Signed-off-by: Riku Voipio <riku.voipio@linaro.org> >> --- >> scripts/checkpatch.pl | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index dde3f5f..98a007f 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -2405,8 +2405,9 @@ sub process { >> } >> # check of hardware specific defines >> # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases >> -# where they might be necessary. >> - if ($line =~ m@^.\s*\#\s*if.*\b__@) { >> +# where they might be necessary. Skip test on linux-user and bsd-user >> +# where arch defines are needed >> + if (!($realfile =~ /^(linux|bsd)-user/) && $line =~ m@^.\s*\#\s*if.*\b__@) { >> ERROR("architecture specific defines should be avoided\n" . $herecurr); >> } > > Do you have some examples of the false positives you want > to suppress here? For new code I would hope that we can > handle host-arch-specifics by having new files (or just > new #defines etc) in linux-user/host/$ARCH/ rather than > inline #ifdeffery in the main files. One example from your patch: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05650.html And another from Laurent: https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg06486.html Every new syscall will comes with "#ifdef TARGET_NR_foo and defined(__NR_foo)", while host/target combos catch up. Now, most TARGET_NR_foo's are needed only for unicore32, but the __NR_foo defines will be needed for a very long time. Riku
On 26 September 2016 at 16:36, Riku Voipio <riku.voipio@linaro.org> wrote: > On 27 September 2016 at 00:08, Peter Maydell <peter.maydell@linaro.org> wrote: >> Do you have some examples of the false positives you want >> to suppress here? For new code I would hope that we can >> handle host-arch-specifics by having new files (or just >> new #defines etc) in linux-user/host/$ARCH/ rather than >> inline #ifdeffery in the main files. > > One example from your patch: > > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg05650.html > > And another from Laurent: > > https://lists.gnu.org/archive/html/qemu-devel/2016-09/msg06486.html > > Every new syscall will comes with "#ifdef TARGET_NR_foo and > defined(__NR_foo)", while host/target combos catch up. Now, most > TARGET_NR_foo's are needed only for unicore32, but the __NR_foo > defines will be needed for a very long time. Oh, I see; I don't think of the __NR_foo as being "architecture specific". I think we'd be better off specifically whitelisting those in checkpatch rather than turning off the whole check for linux-user. thanks -- PMM
On 26/09/2016 21:58, riku.voipio@linaro.org wrote: > From: Riku Voipio <riku.voipio@linaro.org> > > Linux-user and bsd-user code needs lots of arch-specific ifdefs, > so disable the warning. > > Signed-off-by: Riku Voipio <riku.voipio@linaro.org> > --- > scripts/checkpatch.pl | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index dde3f5f..98a007f 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2405,8 +2405,9 @@ sub process { > } > # check of hardware specific defines > # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases > -# where they might be necessary. > - if ($line =~ m@^.\s*\#\s*if.*\b__@) { > +# where they might be necessary. Skip test on linux-user and bsd-user > +# where arch defines are needed > + if (!($realfile =~ /^(linux|bsd)-user/) && $line =~ m@^.\s*\#\s*if.*\b__@) { > ERROR("architecture specific defines should be avoided\n" . $herecurr); > } > > Hi Riku, I have already posted a pull request that degrades this to a warning. Later on we may make it an error except for some files and/or patterns. For linux-user I think that __NR_* should be definitely allowed, but a blanket permission is not necessary. Paolo
On 27 September 2016 at 14:58, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 26/09/2016 21:58, riku.voipio@linaro.org wrote: >> From: Riku Voipio <riku.voipio@linaro.org> >> >> Linux-user and bsd-user code needs lots of arch-specific ifdefs, >> so disable the warning. >> >> Signed-off-by: Riku Voipio <riku.voipio@linaro.org> >> --- >> scripts/checkpatch.pl | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index dde3f5f..98a007f 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -2405,8 +2405,9 @@ sub process { >> } >> # check of hardware specific defines >> # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases >> -# where they might be necessary. >> - if ($line =~ m@^.\s*\#\s*if.*\b__@) { >> +# where they might be necessary. Skip test on linux-user and bsd-user >> +# where arch defines are needed >> + if (!($realfile =~ /^(linux|bsd)-user/) && $line =~ m@^.\s*\#\s*if.*\b__@) { >> ERROR("architecture specific defines should be avoided\n" . $herecurr); >> } >> >> > > Hi Riku, > > I have already posted a pull request that degrades this to a warning. > > Later on we may make it an error except for some files and/or patterns. > For linux-user I think that __NR_* should be definitely allowed, but a > blanket permission is not necessary. I'll update my patch against your PR and to check not only for linux-user dir, but also match __NR_. Riku
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index dde3f5f..98a007f 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2405,8 +2405,9 @@ sub process { } # check of hardware specific defines # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases -# where they might be necessary. - if ($line =~ m@^.\s*\#\s*if.*\b__@) { +# where they might be necessary. Skip test on linux-user and bsd-user +# where arch defines are needed + if (!($realfile =~ /^(linux|bsd)-user/) && $line =~ m@^.\s*\#\s*if.*\b__@) { ERROR("architecture specific defines should be avoided\n" . $herecurr); }