Re: [Qemu-devel] [PATCH] checkpatch.pl: disable arch-specific test for linux-user

[Riku Voipio]

On 27 September 2016 at 14:58, Paolo Bonzini  wrote:
>
>
> On 26/09/2016 21:58, riku.voi...@linaro.org wrote:
>> From: Riku Voipio 
>>
>> Linux-user and bsd-user code needs lots of arch-specific ifdefs,
>> so disable the warning.
>>
>> Signed-off-by: Riku Voipio 
>> ---
>>  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

Re: [Qemu-devel] [PATCH] checkpatch.pl: disable arch-specific test for linux-user

[Paolo Bonzini]

On 26/09/2016 21:58, riku.voi...@linaro.org wrote:
> From: Riku Voipio 
> 
> Linux-user and bsd-user code needs lots of arch-specific ifdefs,
> so disable the warning.
> 
> Signed-off-by: Riku Voipio 
> ---
>  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

Re: [Qemu-devel] [PATCH] checkpatch.pl: disable arch-specific test for linux-user

[Peter Maydell]

On 26 September 2016 at 16:36, Riku Voipio  wrote:
> On 27 September 2016 at 00:08, Peter Maydell  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

Re: [Qemu-devel] [PATCH] checkpatch.pl: disable arch-specific test for linux-user

[Riku Voipio]

On 27 September 2016 at 00:08, Peter Maydell  wrote:
> On 26 September 2016 at 12:58,   wrote:
>> From: Riku Voipio 
>>
>> Linux-user and bsd-user code needs lots of arch-specific ifdefs,
>> so disable the warning.
>>
>> Signed-off-by: Riku Voipio 
>> ---
>>  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

Re: [Qemu-devel] [PATCH] checkpatch.pl: disable arch-specific test for linux-user

[Peter Maydell]

On 26 September 2016 at 12:58,   wrote:
> From: Riku Voipio 
>
> Linux-user and bsd-user code needs lots of arch-specific ifdefs,
> so disable the warning.
>
> Signed-off-by: Riku Voipio 
> ---
>  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

Re: [Qemu-devel] [PATCH] checkpatch.pl: disable arch-specific test for linux-user

[no-reply]

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.voi...@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.voi...@linaro.org -> 
patchew/1474919934-21518-1-git-send-email-riku.voi...@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-de...@freelists.org