* [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-18 15:46 [RFC PATCH 0/5] selftests: floppy: basic tests Denis Efremov
@ 2021-08-18 15:46 ` Denis Efremov
2021-08-18 16:00 ` Joe Perches
2021-08-18 15:46 ` [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio Denis Efremov
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Denis Efremov @ 2021-08-18 15:46 UTC (permalink / raw)
To: linux-kselftest
Cc: Denis Efremov, linux-kernel, linux-block, Jens Axboe,
Jiri Kosina, Willy Tarreau, Joe Perches
Properly handle commits like:
commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"")
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
scripts/checkpatch.pl | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 461d4221e4a4..cf31e8c994d3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3200,20 +3200,20 @@ sub process {
$long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i);
$space = 0 if ($line =~ /\bcommit [0-9a-f]/i);
$case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
- if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
+ if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)"\)/i) {
$orig_desc = $1;
$hasparens = 1;
} elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
defined $rawlines[$linenr] &&
- $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
+ $rawlines[$linenr] =~ /^\s*\("(.+)"\)/) {
$orig_desc = $1;
$hasparens = 1;
- } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
+ } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\(".+$/i &&
defined $rawlines[$linenr] &&
- $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
- $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
+ $rawlines[$linenr] =~ /^\s*.+"\)/) {
+ $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)$/i;
$orig_desc = $1;
- $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
+ $rawlines[$linenr] =~ /^\s*(.+)"\)/;
$orig_desc .= " " . $1;
$hasparens = 1;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-18 15:46 ` [RFC PATCH 1/5] checkpatch: improve handling of revert commits Denis Efremov
@ 2021-08-18 16:00 ` Joe Perches
2021-08-18 16:21 ` Denis Efremov
0 siblings, 1 reply; 21+ messages in thread
From: Joe Perches @ 2021-08-18 16:00 UTC (permalink / raw)
To: Denis Efremov, linux-kselftest
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On Wed, 2021-08-18 at 18:46 +0300, Denis Efremov wrote:
> Properly handle commits like:
> commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"")
Try this one:
https://lore.kernel.org/lkml/7f55d9d0369f1ea840fab83eeb77f9f3601fee41.camel@perches.com/
>
> Cc: Joe Perches <joe@perches.com>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> scripts/checkpatch.pl | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 461d4221e4a4..cf31e8c994d3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3200,20 +3200,20 @@ sub process {
> $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i);
> $space = 0 if ($line =~ /\bcommit [0-9a-f]/i);
> $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
> - if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
> + if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)"\)/i) {
> $orig_desc = $1;
> $hasparens = 1;
> } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
> defined $rawlines[$linenr] &&
> - $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
> + $rawlines[$linenr] =~ /^\s*\("(.+)"\)/) {
> $orig_desc = $1;
> $hasparens = 1;
> - } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
> + } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\(".+$/i &&
> defined $rawlines[$linenr] &&
> - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
> - $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
> + $rawlines[$linenr] =~ /^\s*.+"\)/) {
> + $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)$/i;
> $orig_desc = $1;
> - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
> + $rawlines[$linenr] =~ /^\s*(.+)"\)/;
> $orig_desc .= " " . $1;
> $hasparens = 1;
> }
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-18 16:00 ` Joe Perches
@ 2021-08-18 16:21 ` Denis Efremov
2021-08-18 20:21 ` Joe Perches
2021-08-18 21:22 ` Joe Perches
0 siblings, 2 replies; 21+ messages in thread
From: Denis Efremov @ 2021-08-18 16:21 UTC (permalink / raw)
To: Joe Perches, linux-kselftest
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On 8/18/21 7:00 PM, Joe Perches wrote:
> On Wed, 2021-08-18 at 18:46 +0300, Denis Efremov wrote:
>> Properly handle commits like:
>> commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"")
>
> Try this one:
>
> https://lore.kernel.org/lkml/7f55d9d0369f1ea840fab83eeb77f9f3601fee41.camel@perches.com/
>
It works but why not to use .+? then?
I'm not sure that non-greedy patterns will properly handle commits like:
$ git log --oneline | fgrep '")'
e.g.
commit ece2619fe8ed ("extcon: arizona: Fix flags parameter to the gpiod_get("wlf,micd-pol") call")
>>
>> Cc: Joe Perches <joe@perches.com>
>> Signed-off-by: Denis Efremov <efremov@linux.com>
>> ---
>> scripts/checkpatch.pl | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 461d4221e4a4..cf31e8c994d3 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -3200,20 +3200,20 @@ sub process {
>> $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i);
>> $space = 0 if ($line =~ /\bcommit [0-9a-f]/i);
>> $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
>> - if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
>> + if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)"\)/i) {
>> $orig_desc = $1;
>> $hasparens = 1;
>> } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
>> defined $rawlines[$linenr] &&
>> - $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
>> + $rawlines[$linenr] =~ /^\s*\("(.+)"\)/) {
>> $orig_desc = $1;
>> $hasparens = 1;
>> - } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
>> + } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\(".+$/i &&
>> defined $rawlines[$linenr] &&
>> - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
>> - $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
>> + $rawlines[$linenr] =~ /^\s*.+"\)/) {
>> + $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)$/i;
>> $orig_desc = $1;
>> - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
>> + $rawlines[$linenr] =~ /^\s*(.+)"\)/;
>> $orig_desc .= " " . $1;
>> $hasparens = 1;
>> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-18 16:21 ` Denis Efremov
@ 2021-08-18 20:21 ` Joe Perches
2021-08-18 20:35 ` Andrew Morton
2021-08-18 21:22 ` Joe Perches
1 sibling, 1 reply; 21+ messages in thread
From: Joe Perches @ 2021-08-18 20:21 UTC (permalink / raw)
To: Denis Efremov, linux-kselftest, Andrew Morton
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On Wed, 2021-08-18 at 19:21 +0300, Denis Efremov wrote:
>
> On 8/18/21 7:00 PM, Joe Perches wrote:
> > On Wed, 2021-08-18 at 18:46 +0300, Denis Efremov wrote:
> > > Properly handle commits like:
> > > commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"")
> >
> > Try this one:
> >
> > https://lore.kernel.org/lkml/7f55d9d0369f1ea840fab83eeb77f9f3601fee41.camel@perches.com/
> >
>
> It works but why not to use .+? then?
> I'm not sure that non-greedy patterns will properly handle commits like:
> $ git log --oneline | fgrep '")'
>
> e.g.
> commit ece2619fe8ed ("extcon: arizona: Fix flags parameter to the gpiod_get("wlf,micd-pol") call")
The only way to handle that is to use the $balanced_parens test but
it wouldn't work on Andrew's perl version 5.8
Andrew? Do you still use perl 5.8? It's almost 20 years old now.
Does anyone still use perl versions 5.8 or lower?
From checkpatch:
# Using $balanced_parens, $LvalOrFunc, or $FuncArg
# requires at least perl version v5.10.0
# Any use must be runtime checked with $^V
our $balanced_parens = qr/(\((?:[^\(\)]++|(?-1))*\))/;
our $LvalOrFunc = qr{((?:[\&\*]\s*)?$Lval)\s*($balanced_parens{0,1})\s*};
our $FuncArg = qr{$Typecast{0,1}($LvalOrFunc|$Constant|$String)};
So maybe:
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > > @@ -3200,20 +3200,20 @@ sub process {
> > > $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i);
> > > $space = 0 if ($line =~ /\bcommit [0-9a-f]/i);
> > > $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
> > > - if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
> > > + if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)"\)/i) {
So that could be:
if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+($balanced_parens)/i
> > > $orig_desc = $1;
> > > $hasparens = 1;
> > > } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
> > > defined $rawlines[$linenr] &&
> > > - $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
> > > + $rawlines[$linenr] =~ /^\s*\("(.+)"\)/) {
and
} elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
defined $rawlines[$linenr] &&
"$line $rawlines[$linenr]" =~ /\bcommit\s+[0-9a-f]{5,}\s+($balanced_parens)/i
> > > $orig_desc = $1;
> > > $hasparens = 1;
> > > - } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
> > > + } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\(".+$/i &&
> > > defined $rawlines[$linenr] &&
> > > - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
> > > - $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
> > > + $rawlines[$linenr] =~ /^\s*.+"\)/) {
> > > + $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("(.+)$/i;
etc...
> > > $orig_desc = $1;
> > > - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
> > > + $rawlines[$linenr] =~ /^\s*(.+)"\)/;
> > > $orig_desc .= " " . $1;
> > > $hasparens = 1;
> > > }
> >
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-18 20:21 ` Joe Perches
@ 2021-08-18 20:35 ` Andrew Morton
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Morton @ 2021-08-18 20:35 UTC (permalink / raw)
To: Joe Perches
Cc: Denis Efremov, linux-kselftest, linux-kernel, linux-block,
Jens Axboe, Jiri Kosina, Willy Tarreau
On Wed, 18 Aug 2021 13:21:16 -0700 Joe Perches <joe@perches.com> wrote:
> Andrew? Do you still use perl 5.8? It's almost 20 years old now.
5.26 is the oldest I appear to be using.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-18 16:21 ` Denis Efremov
2021-08-18 20:21 ` Joe Perches
@ 2021-08-18 21:22 ` Joe Perches
2021-08-19 19:52 ` Denis Efremov
1 sibling, 1 reply; 21+ messages in thread
From: Joe Perches @ 2021-08-18 21:22 UTC (permalink / raw)
To: Denis Efremov, linux-kselftest
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
Hey Denis:
Try this one please and let me know what you think...
---
scripts/checkpatch.pl | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 161ce7fe5d1e5..4e2e79eff9b8c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3196,26 +3196,21 @@ sub process {
$orig_commit = lc($1);
}
- $short = 0 if ($line =~ /\bcommit\s+[0-9a-f]{12,40}/i);
- $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i);
- $space = 0 if ($line =~ /\bcommit [0-9a-f]/i);
- $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
- if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
- $orig_desc = $1;
- $hasparens = 1;
- } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
- defined $rawlines[$linenr] &&
- $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
- $orig_desc = $1;
- $hasparens = 1;
- } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
- defined $rawlines[$linenr] &&
- $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
- $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
+ my $input = $line;
+ for (my $n = 0; $n < 2; $n++) {
+ $input .= " $rawlines[$linenr + $n]" if ($#lines >= $linenr + $n);
+ }
+
+ $short = 0 if ($input =~ /\bcommit\s+[0-9a-f]{12,40}/i);
+ $long = 1 if ($input =~ /\bcommit\s+[0-9a-f]{41,}/i);
+ $space = 0 if ($input =~ /\bcommit [0-9a-f]/i);
+ $case = 0 if ($input =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
+ if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s+($balanced_parens)/i) {
$orig_desc = $1;
- $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
- $orig_desc .= " " . $1;
$hasparens = 1;
+ # Always strip leading/trailing parens then double quotes if existing
+ $orig_desc = substr($orig_desc, 1, -1);
+ $orig_desc = substr($orig_desc, 1, -1) if ($orig_desc =~ /^".*"$/);
}
($id, $description) = git_commit_info($orig_commit,
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-18 21:22 ` Joe Perches
@ 2021-08-19 19:52 ` Denis Efremov
2021-08-19 21:44 ` Joe Perches
2021-08-19 22:17 ` Joe Perches
0 siblings, 2 replies; 21+ messages in thread
From: Denis Efremov @ 2021-08-19 19:52 UTC (permalink / raw)
To: Joe Perches, linux-kselftest
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
Hi,
On 8/19/21 12:22 AM, Joe Perches wrote:
> Hey Denis:
>
> Try this one please and let me know what you think...
Looks good to me. Couple of nitpicks below
>
> ---
> scripts/checkpatch.pl | 31 +++++++++++++------------------
> 1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 161ce7fe5d1e5..4e2e79eff9b8c 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3196,26 +3196,21 @@ sub process {
> $orig_commit = lc($1);
> }
>
> - $short = 0 if ($line =~ /\bcommit\s+[0-9a-f]{12,40}/i);
> - $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i);
> - $space = 0 if ($line =~ /\bcommit [0-9a-f]/i);
> - $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
> - if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
> - $orig_desc = $1;
> - $hasparens = 1;
> - } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
> - defined $rawlines[$linenr] &&
> - $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
> - $orig_desc = $1;
> - $hasparens = 1;
> - } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
> - defined $rawlines[$linenr] &&
> - $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
> - $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
> + my $input = $line;
> + for (my $n = 0; $n < 2; $n++) {
> + $input .= " $rawlines[$linenr + $n]" if ($#lines >= $linenr + $n);
> + }
> +
> + $short = 0 if ($input =~ /\bcommit\s+[0-9a-f]{12,40}/i);
> + $long = 1 if ($input =~ /\bcommit\s+[0-9a-f]{41,}/i);
> + $space = 0 if ($input =~ /\bcommit [0-9a-f]/i);
> + $case = 0 if ($input =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
> + if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s+($balanced_parens)/i) {
> $orig_desc = $1;
> - $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
> - $orig_desc .= " " . $1;
> $hasparens = 1;
> + # Always strip leading/trailing parens then double quotes if existing
> + $orig_desc = substr($orig_desc, 1, -1);
> + $orig_desc = substr($orig_desc, 1, -1) if ($orig_desc =~ /^".*"$/);
Why do you want to add "if ($orig_desc =~ /^".*"$/);" here? and not just substr($orig_desc, 2, -2);?
> }
>
> ($id, $description) = git_commit_info($orig_commit,
>
In your previous patch with '.*?' you added a branch to allow also newlines between commit and shas:
```
commit
c3f157259438 (Revert "floppy: reintroduce O_NDELAY fix")
```
Maybe something like this will work (adding a last word from a prevline if line doesn't start from
commit)
+ my $input = $line;
if ($line =~ /\b(c)ommit\s+([0-9a-f]{5,})\b/i) {
$init_char = $1;
$orig_commit = lc($2);
} elsif ($line =~ /\b([0-9a-f]{12,40})\b/i) {
$orig_commit = lc($1);
+ $prevline =~ /(\w+)$/;
+ $line = $1 . " " . $prevline;
}
- my $input = $line;
for (my $n = 0; $n < 2; $n++) {
$input .= " $rawlines[$linenr + $n]" if ($#lines >= $linenr + $n);
}
Thanks,
Denis
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-19 19:52 ` Denis Efremov
@ 2021-08-19 21:44 ` Joe Perches
2021-08-19 22:17 ` Joe Perches
1 sibling, 0 replies; 21+ messages in thread
From: Joe Perches @ 2021-08-19 21:44 UTC (permalink / raw)
To: Denis Efremov, linux-kselftest
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On Thu, 2021-08-19 at 22:52 +0300, Denis Efremov wrote:
> Hi,
>
> On 8/19/21 12:22 AM, Joe Perches wrote:
> > Hey Denis:
> >
> > Try this one please and let me know what you think...
>
> Looks good to me. Couple of nitpicks below
yeah, thanks.
How about this one:
---
scripts/checkpatch.pl | 72 ++++++++++++++++++++++++++++++---------------------
1 file changed, 43 insertions(+), 29 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 161ce7fe5d1e5..4988515a0dfb3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1181,7 +1181,8 @@ sub git_commit_info {
# git log --format='%H %s' -1 $line |
# echo "commit $(cut -c 1-12,41-)"
# done
- } elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./) {
+ } elsif ($lines[0] =~ /^fatal: ambiguous argument '$commit': unknown revision or path not in the working tree\./ ||
+ $lines[0] =~ /^fatal: bad object $commit/) {
$id = undef;
} else {
$id = substr($lines[0], 0, 12);
@@ -2587,6 +2588,8 @@ sub process {
my $reported_maintainer_file = 0;
my $non_utf8_charset = 0;
+ my $last_git_commit_id_linenr = -1;
+
my $last_blank_line = 0;
my $last_coalesced_string_linenr = -1;
@@ -3173,7 +3176,8 @@ sub process {
if ($in_commit_log && !$commit_log_possible_stack_dump &&
$line !~ /^\s*(?:Link|Patchwork|http|https|BugLink|base-commit):/i &&
$line !~ /^This reverts commit [0-9a-f]{7,40}/ &&
- ($line =~ /\bcommit\s+[0-9a-f]{5,}\b/i ||
+ (($line =~ /\bcommit\s+[0-9a-f]{5,}\b/i ||
+ ($line =~ /\bcommit\s*$/i && defined($rawlines[$linenr]) && $rawlines[$linenr] =~ /^\s*[0-9a-f]{5,}\b/i)) ||
($line =~ /(?:\s|^)[0-9a-f]{12,40}(?:[\s"'\(\[]|$)/i &&
$line !~ /[\<\[][0-9a-f]{12,40}[\>\]]/i &&
$line !~ /\bfixes:\s*[0-9a-f]{12,40}/i))) {
@@ -3183,49 +3187,59 @@ sub process {
my $long = 0;
my $case = 1;
my $space = 1;
- my $hasdesc = 0;
my $hasparens = 0;
my $id = '0123456789ab';
my $orig_desc = "commit description";
my $description = "";
+ my $herectx = $herecurr;
+ my $has_parens = 0;
+
+ my $input = $line;
+ if ($line =~ /(?:\bcommit\s+[0-9a-f]{5,}|\bcommit\s*$)/i) {
+ for (my $n = 0; $n < 2; $n++) {
+ if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s*$balanced_parens/i) {
+ $has_parens = 1;
+ last;
+ }
+ last if ($#lines < $linenr + $n);
+ $input .= " " . trim($rawlines[$linenr + $n]);
+ $herectx .= "$rawlines[$linenr + $n]\n";
+ }
+ $herectx = $herecurr if (!$has_parens);
+ }
- if ($line =~ /\b(c)ommit\s+([0-9a-f]{5,})\b/i) {
+ if ($input =~ /\b(c)ommit\s+([0-9a-f]{5,})\b/i) {
$init_char = $1;
$orig_commit = lc($2);
- } elsif ($line =~ /\b([0-9a-f]{12,40})\b/i) {
+ $short = 0 if ($input =~ /\bcommit\s+[0-9a-f]{12,40}/i);
+ $long = 1 if ($input =~ /\bcommit\s+[0-9a-f]{41,}/i);
+ $space = 0 if ($input =~ /\bcommit [0-9a-f]/i);
+ $case = 0 if ($input =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
+
+ if ($input =~ /\bcommit\s+[0-9a-f]{5,}\s+($balanced_parens)/i) {
+ $orig_desc = $1;
+ # Always strip leading/trailing parens then double quotes if existing
+ $orig_desc = substr($orig_desc, 1, -1);
+ if ($orig_desc =~ /^".*"$/) {
+ $orig_desc = substr($orig_desc, 1, -1);
+ $hasparens = 1;
+ }
+ }
+ } elsif ($input =~ /\b([0-9a-f]{12,40})\b/i) {
$orig_commit = lc($1);
}
- $short = 0 if ($line =~ /\bcommit\s+[0-9a-f]{12,40}/i);
- $long = 1 if ($line =~ /\bcommit\s+[0-9a-f]{41,}/i);
- $space = 0 if ($line =~ /\bcommit [0-9a-f]/i);
- $case = 0 if ($line =~ /\b[Cc]ommit\s+[0-9a-f]{5,40}[^A-F]/);
- if ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)"\)/i) {
- $orig_desc = $1;
- $hasparens = 1;
- } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s*$/i &&
- defined $rawlines[$linenr] &&
- $rawlines[$linenr] =~ /^\s*\("([^"]+)"\)/) {
- $orig_desc = $1;
- $hasparens = 1;
- } elsif ($line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("[^"]+$/i &&
- defined $rawlines[$linenr] &&
- $rawlines[$linenr] =~ /^\s*[^"]+"\)/) {
- $line =~ /\bcommit\s+[0-9a-f]{5,}\s+\("([^"]+)$/i;
- $orig_desc = $1;
- $rawlines[$linenr] =~ /^\s*([^"]+)"\)/;
- $orig_desc .= " " . $1;
- $hasparens = 1;
- }
-
($id, $description) = git_commit_info($orig_commit,
$id, $orig_desc);
if (defined($id) &&
- ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens)) {
+ ($short || $long || $space || $case || ($orig_desc ne $description) || !$hasparens) &&
+ $last_git_commit_id_linenr != $linenr - 1) {
ERROR("GIT_COMMIT_ID",
- "Please use git commit description style 'commit <12+ chars of sha1> (\"<title line>\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herecurr);
+ "Please use git commit description style 'commit <12+ chars of sha1> (\"<title line>\")' - ie: '${init_char}ommit $id (\"$description\")'\n" . $herectx);
}
+ #don't report the next line if this line ends in commit and the sha1 hash is the next line
+ $last_git_commit_id_linenr = $linenr if ($line =~ /\bcommit\s*$/i);
}
# Check for added, moved or deleted files
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-19 19:52 ` Denis Efremov
2021-08-19 21:44 ` Joe Perches
@ 2021-08-19 22:17 ` Joe Perches
2021-08-21 6:47 ` Denis Efremov
1 sibling, 1 reply; 21+ messages in thread
From: Joe Perches @ 2021-08-19 22:17 UTC (permalink / raw)
To: Denis Efremov, linux-kselftest, Andrew Morton
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On Thu, 2021-08-19 at 22:52 +0300, Denis Efremov wrote:
> Hi,
>
> Why do you want to add "if ($orig_desc =~ /^".*"$/);" here? and not just substr($orig_desc, 2, -2);?
Because commit descriptions sometimes to not have quotes like
commit <deadbeef> (Multiple word description)
btw:
I tested the last proposal with this script:
$ git log --grep="commit [0-9a-f]" -i --format=%h -1000 | \
while read commit ; do \
echo $commit; \
./scripts/checkpatch.pl --git --no-summary --quiet --types=GIT_COMMIT_ID $commit ; \
done
and there are still a fair number of ERRORs.
And I'm not sure if this particular ERROR is that useful overall.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-19 22:17 ` Joe Perches
@ 2021-08-21 6:47 ` Denis Efremov
2021-08-21 7:12 ` Joe Perches
0 siblings, 1 reply; 21+ messages in thread
From: Denis Efremov @ 2021-08-21 6:47 UTC (permalink / raw)
To: Joe Perches, linux-kselftest, Andrew Morton
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On 8/20/21 1:17 AM, Joe Perches wrote:
>
> And I'm not sure if this particular ERROR is that useful overall.
I find it useful to check commit-id and that it matches a title.
It's easy to make a typo in commit-id and get an invalid one.
Denis
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-21 6:47 ` Denis Efremov
@ 2021-08-21 7:12 ` Joe Perches
2021-08-21 7:35 ` Denis Efremov
0 siblings, 1 reply; 21+ messages in thread
From: Joe Perches @ 2021-08-21 7:12 UTC (permalink / raw)
To: Denis Efremov, linux-kselftest, Andrew Morton
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On Sat, 2021-08-21 at 09:47 +0300, Denis Efremov wrote:
>
> On 8/20/21 1:17 AM, Joe Perches wrote:
> >
> > And I'm not sure if this particular ERROR is that useful overall.
>
> I find it useful to check commit-id and that it matches a title.
> It's easy to make a typo in commit-id and get an invalid one.
That's true, but I meant requiring the sha1 hash to contain both
the word "commit" and use ("title").
Looking at checkpatch's errors produced by this GIT_COMMIT_ID
test makes the required form seem a bit too inflexible to me.
For instance: a sha1 hash may be repeated in a commit message where
the first instance has the correct form but the second use is just
the hash and the warning is still produced.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH 1/5] checkpatch: improve handling of revert commits
2021-08-21 7:12 ` Joe Perches
@ 2021-08-21 7:35 ` Denis Efremov
0 siblings, 0 replies; 21+ messages in thread
From: Denis Efremov @ 2021-08-21 7:35 UTC (permalink / raw)
To: Joe Perches, linux-kselftest, Andrew Morton
Cc: linux-kernel, linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On 8/21/21 10:12 AM, Joe Perches wrote:
> On Sat, 2021-08-21 at 09:47 +0300, Denis Efremov wrote:
>>
>> On 8/20/21 1:17 AM, Joe Perches wrote:
>>>
>>> And I'm not sure if this particular ERROR is that useful overall.
>>
>> I find it useful to check commit-id and that it matches a title.
>> It's easy to make a typo in commit-id and get an invalid one.
>
> That's true, but I meant requiring the sha1 hash to contain both
> the word "commit" and use ("title").
>
> Looking at checkpatch's errors produced by this GIT_COMMIT_ID
> test makes the required form seem a bit too inflexible to me.
>
> For instance: a sha1 hash may be repeated in a commit message where
> the first instance has the correct form but the second use is just
> the hash and the warning is still produced.
>
I agree with you. There is also another example with list of commits:
- commit <id-1> ("Title1")
- commit <id-2> ("Title2")
...
I see no reason in writing "commit" on each line.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio
2021-08-18 15:46 [RFC PATCH 0/5] selftests: floppy: basic tests Denis Efremov
2021-08-18 15:46 ` [RFC PATCH 1/5] checkpatch: improve handling of revert commits Denis Efremov
@ 2021-08-18 15:46 ` Denis Efremov
2021-08-18 21:47 ` kernel test robot
2021-08-19 0:24 ` Masahiro Yamada
2021-08-18 15:46 ` [RFC PATCH 3/5] selftests: floppy: add basic tests for opening an empty device Denis Efremov
` (2 subsequent siblings)
4 siblings, 2 replies; 21+ messages in thread
From: Denis Efremov @ 2021-08-18 15:46 UTC (permalink / raw)
To: linux-kselftest
Cc: Denis Efremov, linux-kernel, linux-block, Jens Axboe,
Jiri Kosina, Willy Tarreau, Masahiro Yamada
Use absolute path to call gen_init_cpio. This allows one
to use gen_initramfs.sh from any directory.
Cc: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
usr/gen_initramfs.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/usr/gen_initramfs.sh b/usr/gen_initramfs.sh
index 63476bb70b41..2e4a86181c79 100755
--- a/usr/gen_initramfs.sh
+++ b/usr/gen_initramfs.sh
@@ -244,4 +244,4 @@ if test -n "$KBUILD_BUILD_TIMESTAMP"; then
timestamp="-t $timestamp"
fi
fi
-usr/gen_init_cpio $timestamp $cpio_list > $output
+"$(dirname "$0")"/gen_init_cpio $timestamp $cpio_list > $output
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio
2021-08-18 15:46 ` [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio Denis Efremov
@ 2021-08-18 21:47 ` kernel test robot
2021-08-19 0:24 ` Masahiro Yamada
1 sibling, 0 replies; 21+ messages in thread
From: kernel test robot @ 2021-08-18 21:47 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]
Hi Denis,
[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on kselftest/next]
[also build test ERROR on linux/master linus/master v5.14-rc6 next-20210818]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Denis-Efremov/selftests-floppy-basic-tests/20210818-235242
base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
config: x86_64-randconfig-c022-20210818 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/9794c683e44ce85ae80f3d3b018466d3bc1b5807
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Denis-Efremov/selftests-floppy-basic-tests/20210818-235242
git checkout 9794c683e44ce85ae80f3d3b018466d3bc1b5807
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> usr/gen_initramfs.sh: 247: usr/gen_initramfs.sh: usr/gen_init_cpio: not found
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32777 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio
2021-08-18 15:46 ` [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio Denis Efremov
2021-08-18 21:47 ` kernel test robot
@ 2021-08-19 0:24 ` Masahiro Yamada
2021-08-19 20:51 ` Denis Efremov
1 sibling, 1 reply; 21+ messages in thread
From: Masahiro Yamada @ 2021-08-19 0:24 UTC (permalink / raw)
To: Denis Efremov
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On Thu, Aug 19, 2021 at 12:47 AM Denis Efremov <efremov@linux.com> wrote:
>
> Use absolute path to call gen_init_cpio. This allows one
> to use gen_initramfs.sh from any directory.
I do not mind this, but $(dirname "$0")
is not necessarily an absolute path, is it?
I added test code:
echo dirname is $(dirname $0)
in this script, and I saw
dirname is usr
>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> usr/gen_initramfs.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/usr/gen_initramfs.sh b/usr/gen_initramfs.sh
> index 63476bb70b41..2e4a86181c79 100755
> --- a/usr/gen_initramfs.sh
> +++ b/usr/gen_initramfs.sh
> @@ -244,4 +244,4 @@ if test -n "$KBUILD_BUILD_TIMESTAMP"; then
> timestamp="-t $timestamp"
> fi
> fi
> -usr/gen_init_cpio $timestamp $cpio_list > $output
> +"$(dirname "$0")"/gen_init_cpio $timestamp $cpio_list > $output
> --
> 2.31.1
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio
2021-08-19 0:24 ` Masahiro Yamada
@ 2021-08-19 20:51 ` Denis Efremov
2021-08-20 2:19 ` Masahiro Yamada
0 siblings, 1 reply; 21+ messages in thread
From: Denis Efremov @ 2021-08-19 20:51 UTC (permalink / raw)
To: Masahiro Yamada
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On 8/19/21 3:24 AM, Masahiro Yamada wrote:
> On Thu, Aug 19, 2021 at 12:47 AM Denis Efremov <efremov@linux.com> wrote:
>>
>> Use absolute path to call gen_init_cpio. This allows one
>> to use gen_initramfs.sh from any directory.
>
> I do not mind this, but $(dirname "$0")
> is not necessarily an absolute path, is it?
>
>
> I added test code:
>
> echo dirname is $(dirname $0)
>
> in this script, and I saw
>
> dirname is usr
Oh, sorry, commit message is wrong. Would that be ok for you if I will change
it in v2 to something like:
Prepend gen_init_cpio call with the same path as gen_initramfs.sh called. This
allows one to use gen_initramfs.sh from any directory, not only from the
kernel's topdir.
>
>
>
>
>>
>> Cc: Masahiro Yamada <masahiroy@kernel.org>
>> Signed-off-by: Denis Efremov <efremov@linux.com>
>> ---
>> usr/gen_initramfs.sh | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/usr/gen_initramfs.sh b/usr/gen_initramfs.sh
>> index 63476bb70b41..2e4a86181c79 100755
>> --- a/usr/gen_initramfs.sh
>> +++ b/usr/gen_initramfs.sh
>> @@ -244,4 +244,4 @@ if test -n "$KBUILD_BUILD_TIMESTAMP"; then
>> timestamp="-t $timestamp"
>> fi
>> fi
>> -usr/gen_init_cpio $timestamp $cpio_list > $output
>> +"$(dirname "$0")"/gen_init_cpio $timestamp $cpio_list > $output
>> --
>> 2.31.1
>>
>
>
> --
> Best Regards
> Masahiro Yamada
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio
2021-08-19 20:51 ` Denis Efremov
@ 2021-08-20 2:19 ` Masahiro Yamada
0 siblings, 0 replies; 21+ messages in thread
From: Masahiro Yamada @ 2021-08-20 2:19 UTC (permalink / raw)
To: Denis Efremov
Cc: open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List,
linux-block, Jens Axboe, Jiri Kosina, Willy Tarreau
On Fri, Aug 20, 2021 at 5:51 AM Denis Efremov <efremov@linux.com> wrote:
>
>
>
> On 8/19/21 3:24 AM, Masahiro Yamada wrote:
> > On Thu, Aug 19, 2021 at 12:47 AM Denis Efremov <efremov@linux.com> wrote:
> >>
> >> Use absolute path to call gen_init_cpio. This allows one
> >> to use gen_initramfs.sh from any directory.
> >
> > I do not mind this, but $(dirname "$0")
> > is not necessarily an absolute path, is it?
> >
> >
> > I added test code:
> >
> > echo dirname is $(dirname $0)
> >
> > in this script, and I saw
> >
> > dirname is usr
>
> Oh, sorry, commit message is wrong. Would that be ok for you if I will change
> it in v2 to something like:
>
> Prepend gen_init_cpio call with the same path as gen_initramfs.sh called. This
> allows one to use gen_initramfs.sh from any directory, not only from the
> kernel's topdir.
I am fine with it.
This patch is prefixed with 2/5, so I assume
you expect another person to pick up
the entire series.
With the commit message updated,
Reviewed-by: Masahiro Yamada <masahiroy@kernel.org>
> >
> >
> >
> >
> >>
> >> Cc: Masahiro Yamada <masahiroy@kernel.org>
> >> Signed-off-by: Denis Efremov <efremov@linux.com>
> >> ---
> >> usr/gen_initramfs.sh | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/usr/gen_initramfs.sh b/usr/gen_initramfs.sh
> >> index 63476bb70b41..2e4a86181c79 100755
> >> --- a/usr/gen_initramfs.sh
> >> +++ b/usr/gen_initramfs.sh
> >> @@ -244,4 +244,4 @@ if test -n "$KBUILD_BUILD_TIMESTAMP"; then
> >> timestamp="-t $timestamp"
> >> fi
> >> fi
> >> -usr/gen_init_cpio $timestamp $cpio_list > $output
> >> +"$(dirname "$0")"/gen_init_cpio $timestamp $cpio_list > $output
> >> --
> >> 2.31.1
> >>
> >
> >
> > --
> > Best Regards
> > Masahiro Yamada
> >
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC PATCH 3/5] selftests: floppy: add basic tests for opening an empty device
2021-08-18 15:46 [RFC PATCH 0/5] selftests: floppy: basic tests Denis Efremov
2021-08-18 15:46 ` [RFC PATCH 1/5] checkpatch: improve handling of revert commits Denis Efremov
2021-08-18 15:46 ` [RFC PATCH 2/5] gen_initramfs.sh: use absolute path for gen_init_cpio Denis Efremov
@ 2021-08-18 15:46 ` Denis Efremov
2021-08-18 15:46 ` [RFC PATCH 4/5] selftests: floppy: add basic tests for a readonly disk Denis Efremov
2021-08-18 15:46 ` [RFC PATCH 5/5] selftests: floppy: add basic rdwr tests Denis Efremov
4 siblings, 0 replies; 21+ messages in thread
From: Denis Efremov @ 2021-08-18 15:46 UTC (permalink / raw)
To: linux-kselftest
Cc: Denis Efremov, linux-kernel, linux-block, Jens Axboe,
Jiri Kosina, Willy Tarreau
Add basic tests for O_NDELAY and O_ACCMODE open of an empty floppy device.
"empty" test works under assumption that there is no disk in "/dev/fd0".
It's possible to run it on a real hardware. "run_empty.sh" automates the
testing process by creating a minimal initrd image with "init" and "test"
binaries and running the kernel in QEMU.
The tests cover these regressions:
- commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"")
- commit ff06db1efb2a ("floppy: fix open(O_ACCMODE) for ioctl-only open")
Signed-off-by: Denis Efremov <efremov@linux.com>
---
MAINTAINERS | 1 +
tools/testing/selftests/floppy/.gitignore | 5 ++
tools/testing/selftests/floppy/Makefile | 10 ++++
tools/testing/selftests/floppy/config | 1 +
tools/testing/selftests/floppy/empty.c | 58 +++++++++++++++++++++
tools/testing/selftests/floppy/init.c | 43 +++++++++++++++
tools/testing/selftests/floppy/lib.sh | 57 ++++++++++++++++++++
tools/testing/selftests/floppy/run_empty.sh | 16 ++++++
8 files changed, 191 insertions(+)
create mode 100644 tools/testing/selftests/floppy/.gitignore
create mode 100644 tools/testing/selftests/floppy/Makefile
create mode 100644 tools/testing/selftests/floppy/config
create mode 100644 tools/testing/selftests/floppy/empty.c
create mode 100644 tools/testing/selftests/floppy/init.c
create mode 100644 tools/testing/selftests/floppy/lib.sh
create mode 100755 tools/testing/selftests/floppy/run_empty.sh
diff --git a/MAINTAINERS b/MAINTAINERS
index fd25e4ecf0b9..f2a08b793b54 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7227,6 +7227,7 @@ M: Denis Efremov <efremov@linux.com>
L: linux-block@vger.kernel.org
S: Odd Fixes
F: drivers/block/floppy.c
+F: tools/testing/selftests/floppy/
FLYSKY FSIA6B RC RECEIVER
M: Markus Koch <markus@notsyncing.net>
diff --git a/tools/testing/selftests/floppy/.gitignore b/tools/testing/selftests/floppy/.gitignore
new file mode 100644
index 000000000000..a9fdf1dbaa97
--- /dev/null
+++ b/tools/testing/selftests/floppy/.gitignore
@@ -0,0 +1,5 @@
+init
+cpio_list
+initrd
+
+empty
diff --git a/tools/testing/selftests/floppy/Makefile b/tools/testing/selftests/floppy/Makefile
new file mode 100644
index 000000000000..83e18cd63210
--- /dev/null
+++ b/tools/testing/selftests/floppy/Makefile
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0
+CFLAGS := -static -I../../../../usr/include
+
+TEST_PROGS := run_empty.sh
+TEST_GEN_FILES := init empty
+TEST_FILES := lib.sh
+
+include ../lib.mk
+
+EXTRA_CLEAN := initrd cpio_list
diff --git a/tools/testing/selftests/floppy/config b/tools/testing/selftests/floppy/config
new file mode 100644
index 000000000000..d0c2cd8edfb2
--- /dev/null
+++ b/tools/testing/selftests/floppy/config
@@ -0,0 +1 @@
+CONFIG_BLK_DEV_FD=y
diff --git a/tools/testing/selftests/floppy/empty.c b/tools/testing/selftests/floppy/empty.c
new file mode 100644
index 000000000000..d7e2e6a74132
--- /dev/null
+++ b/tools/testing/selftests/floppy/empty.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <fcntl.h>
+#include <unistd.h>
+#include "../kselftest_harness.h"
+
+FIXTURE(floppy) {
+ const char *dev;
+ int fd;
+};
+
+FIXTURE_VARIANT(floppy) {
+ int flags;
+};
+
+/*
+ * See ff06db1efb2a ("floppy: fix open(O_ACCMODE) for ioctl-only open")
+ * fdutils use O_ACCMODE for probing and ioctl-only open
+ */
+FIXTURE_VARIANT_ADD(floppy, ACCMODE) {
+ .flags = O_ACCMODE,
+};
+
+FIXTURE_VARIANT_ADD(floppy, NACCMODE) {
+ .flags = O_ACCMODE|O_NDELAY,
+};
+
+FIXTURE_VARIANT_ADD(floppy, NRD) {
+ .flags = O_RDONLY|O_NDELAY,
+};
+
+FIXTURE_VARIANT_ADD(floppy, NWR) {
+ .flags = O_WRONLY|O_NDELAY,
+};
+
+FIXTURE_VARIANT_ADD(floppy, NRDWR) {
+ .flags = O_RDWR|O_NDELAY,
+};
+
+FIXTURE_SETUP(floppy)
+{
+ self->dev = "/dev/fd0";
+ if (access(self->dev, F_OK))
+ ksft_exit_skip("No floppy device found\n");
+};
+
+FIXTURE_TEARDOWN(floppy)
+{
+ ASSERT_EQ(close(self->fd), 0);
+}
+
+TEST_F(floppy, open)
+{
+ self->fd = open(self->dev, variant->flags);
+ ASSERT_GT(self->fd, 0);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/floppy/init.c b/tools/testing/selftests/floppy/init.c
new file mode 100644
index 000000000000..d4df4fcfd21f
--- /dev/null
+++ b/tools/testing/selftests/floppy/init.c
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <sys/reboot.h>
+
+__attribute__((noreturn)) static void poweroff(void)
+{
+ fflush(stdout);
+ fflush(stderr);
+ reboot(RB_POWER_OFF);
+ sleep(10);
+ fprintf(stderr, "\nFailed to power off\n");
+ exit(1);
+}
+
+static void panic(const char *what)
+{
+ fprintf(stderr, "\nPANIC %s: %s\n", what, strerror(errno));
+ poweroff();
+}
+
+int main(int argc, char *argv[])
+{
+ pid_t pid;
+
+ pid = fork();
+ if (pid == -1)
+ panic("fork");
+ else if (pid == 0) {
+ execl("/test", "test", NULL);
+ panic("exec");
+ }
+ if (waitpid(pid, NULL, 0) < 0)
+ panic("waitpid");
+
+ poweroff();
+ return 1;
+}
diff --git a/tools/testing/selftests/floppy/lib.sh b/tools/testing/selftests/floppy/lib.sh
new file mode 100644
index 000000000000..22b711ec32ee
--- /dev/null
+++ b/tools/testing/selftests/floppy/lib.sh
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: GPL-2.0
+
+TOP_DIR="$(realpath "$(dirname "$0")"/../../../../)"
+
+QEMU=qemu-system-x86_64
+KERNEL="$TOP_DIR"/arch/x86/boot/bzImage
+INITRD=initrd
+
+gen_cpio_list() {
+ echo "file /init init 755 0 0" > cpio_list
+ echo "file /test $1 755 0 0" >> cpio_list
+ echo "dir /dev 755 0 0" >> cpio_list
+ echo "nod /dev/console 644 0 0 c 5 1" >> cpio_list
+ echo "nod /dev/fd0 666 0 0 b 2 0" >> cpio_list
+ echo "dir /mnt 755 0 0" >> cpio_list
+}
+
+gen_initrd() {
+ "$TOP_DIR"/usr/gen_initramfs.sh -o initrd ./cpio_list
+}
+
+get_selftest_log() {
+ perl -ne '$begin = $_ =~ /^TAP version/ unless $begin;
+ if ($begin && !$end) {
+ $_ =~ s/^\s+|\s+$|\[.+//g;
+ print($_ . "\n") if $_;
+ }
+ $end = $_ =~ /^# Totals:/ unless $end;'
+}
+
+run_qemu() {
+ $QEMU -enable-kvm -nodefaults -nographic \
+ -kernel "$KERNEL" -initrd "$INITRD" \
+ -append "console=ttyS0 earlyprintk=serial" \
+ -serial stdio -monitor none -no-reboot "$@"
+}
+
+run_qemu_debug() {
+ run_qemu "$@"
+}
+
+run_qemu_nodebug() {
+ run_qemu "$@" | get_selftest_log
+}
+
+detect_debug() {
+ if [ "x$1" = "x" ]; then
+ run=run_qemu_nodebug
+ else
+ run=run_qemu_debug
+ fi
+}
+
+run_qemu_empty() {
+ detect_debug "$1"
+ $run -drive index=0,if=floppy
+}
diff --git a/tools/testing/selftests/floppy/run_empty.sh b/tools/testing/selftests/floppy/run_empty.sh
new file mode 100755
index 000000000000..f018107dc6a5
--- /dev/null
+++ b/tools/testing/selftests/floppy/run_empty.sh
@@ -0,0 +1,16 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+source "$(dirname $0)"/lib.sh
+
+while getopts d flag; do
+ case "${flag}" in
+ d) debug=1;;
+ esac
+done
+
+gen_cpio_list empty
+gen_initrd empty
+run_qemu_empty $debug
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC PATCH 4/5] selftests: floppy: add basic tests for a readonly disk
2021-08-18 15:46 [RFC PATCH 0/5] selftests: floppy: basic tests Denis Efremov
` (2 preceding siblings ...)
2021-08-18 15:46 ` [RFC PATCH 3/5] selftests: floppy: add basic tests for opening an empty device Denis Efremov
@ 2021-08-18 15:46 ` Denis Efremov
2021-08-18 15:46 ` [RFC PATCH 5/5] selftests: floppy: add basic rdwr tests Denis Efremov
4 siblings, 0 replies; 21+ messages in thread
From: Denis Efremov @ 2021-08-18 15:46 UTC (permalink / raw)
To: linux-kselftest
Cc: Denis Efremov, linux-kernel, linux-block, Jens Axboe,
Jiri Kosina, Willy Tarreau
Add basic tests for a floppy with a readonly disk. "rdonly" test
works under following assumptions:
- there is a readonly disk in "/dev/fd0"
- the disk is VFAT formatted
- there is "test" file with "TEST" in it on the disk
"run_rdonly.sh" script simulates the conditions and automates the
testing process.
The tests cover the regression:
- commit f2791e7eadf4 ("Revert "floppy: refactor open() flags handling"")
Signed-off-by: Denis Efremov <efremov@linux.com>
---
tools/testing/selftests/floppy/.gitignore | 2 +
tools/testing/selftests/floppy/Makefile | 6 +-
tools/testing/selftests/floppy/lib.sh | 6 ++
tools/testing/selftests/floppy/rdonly.c | 99 ++++++++++++++++++++
tools/testing/selftests/floppy/run_rdonly.sh | 22 +++++
5 files changed, 132 insertions(+), 3 deletions(-)
create mode 100644 tools/testing/selftests/floppy/rdonly.c
create mode 100755 tools/testing/selftests/floppy/run_rdonly.sh
diff --git a/tools/testing/selftests/floppy/.gitignore b/tools/testing/selftests/floppy/.gitignore
index a9fdf1dbaa97..7642dc0ef281 100644
--- a/tools/testing/selftests/floppy/.gitignore
+++ b/tools/testing/selftests/floppy/.gitignore
@@ -1,5 +1,7 @@
init
cpio_list
initrd
+testdir/
empty
+rdonly
diff --git a/tools/testing/selftests/floppy/Makefile b/tools/testing/selftests/floppy/Makefile
index 83e18cd63210..ed8fdeb79aea 100644
--- a/tools/testing/selftests/floppy/Makefile
+++ b/tools/testing/selftests/floppy/Makefile
@@ -1,10 +1,10 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS := -static -I../../../../usr/include
-TEST_PROGS := run_empty.sh
-TEST_GEN_FILES := init empty
+TEST_PROGS := run_empty.sh run_rdonly.sh
+TEST_GEN_FILES := init empty rdonly
TEST_FILES := lib.sh
include ../lib.mk
-EXTRA_CLEAN := initrd cpio_list
+EXTRA_CLEAN := initrd cpio_list testdir
diff --git a/tools/testing/selftests/floppy/lib.sh b/tools/testing/selftests/floppy/lib.sh
index 22b711ec32ee..9988be187bc9 100644
--- a/tools/testing/selftests/floppy/lib.sh
+++ b/tools/testing/selftests/floppy/lib.sh
@@ -55,3 +55,9 @@ run_qemu_empty() {
detect_debug "$1"
$run -drive index=0,if=floppy
}
+
+run_qemu_rdonly_fat() {
+ detect_debug "$2"
+ $run -drive file=fat:floppy:"$1",index=0,if=floppy,readonly
+}
+
diff --git a/tools/testing/selftests/floppy/rdonly.c b/tools/testing/selftests/floppy/rdonly.c
new file mode 100644
index 000000000000..fa60d069540b
--- /dev/null
+++ b/tools/testing/selftests/floppy/rdonly.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <errno.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/mount.h>
+#include <sys/ioctl.h>
+#include <linux/fd.h>
+#include "../kselftest_harness.h"
+
+FIXTURE(floppy) {
+ const char *dev;
+};
+
+FIXTURE_SETUP(floppy)
+{
+ int fd;
+ struct floppy_drive_params params;
+
+ self->dev = "/dev/fd0";
+ if (access(self->dev, F_OK))
+ ksft_exit_skip("No floppy device found\n");
+ if (access(self->dev, R_OK))
+ ksft_exit_skip("Floppy is not read accessible\n");
+
+ fd = open(self->dev, O_ACCMODE|O_NDELAY);
+ EXPECT_EQ(0, ioctl(fd, FDGETDRVPRM, ¶ms));
+ params.flags |= FTD_MSG|FD_DEBUG;
+ EXPECT_EQ(0, ioctl(fd, FDSETDRVPRM, ¶ms));
+ close(fd);
+}
+
+FIXTURE_TEARDOWN(floppy)
+{
+}
+
+TEST_F(floppy, read)
+{
+ int fd, test;
+
+ fd = open(self->dev, O_RDONLY);
+ ASSERT_GT(fd, 0);
+ ASSERT_EQ(read(fd, &test, sizeof(test)), sizeof(test));
+ ASSERT_EQ(close(fd), 0);
+}
+
+TEST_F(floppy, open_write_fail)
+{
+ ASSERT_LT(open(self->dev, O_WRONLY), 0);
+}
+
+TEST_F(floppy, open_rdwr_fail)
+{
+ ASSERT_LT(open(self->dev, O_RDWR), 0);
+}
+
+TEST_F(floppy, ioctl_disk_writable)
+{
+ int fd;
+ struct floppy_drive_struct drive;
+
+ fd = open(self->dev, O_RDONLY|O_NDELAY);
+ ASSERT_GT(fd, 0);
+ ASSERT_EQ(0, ioctl(fd, FDGETDRVSTAT, &drive));
+ ASSERT_FALSE(drive.flags & FD_DISK_WRITABLE);
+ ASSERT_EQ(close(fd), 0);
+}
+
+TEST_F(floppy, mount)
+{
+ int fd;
+ char test[5] = {};
+
+ mount(self->dev, "/mnt", "vfat", MS_RDONLY, NULL);
+ ASSERT_EQ(0, errno);
+
+ fd = open("/mnt/test", O_RDONLY);
+ read(fd, &test, sizeof(test));
+ ASSERT_EQ(0, strncmp(test, "TEST", 4));
+}
+
+TEST_F(floppy, open_ndelay_write_fail)
+{
+#define TEST_DATA "TEST_FAIL_WRITE"
+ int fd;
+ char test[] = TEST_DATA;
+
+ fd = open(self->dev, O_RDWR|O_NDELAY);
+ ASSERT_GT(fd, 0);
+
+ write(fd, test, sizeof(test));
+ read(fd, test, sizeof(test));
+ ASSERT_NE(0, strncmp(TEST_DATA, test, sizeof(TEST_DATA)));
+
+ ASSERT_EQ(close(fd), 0);
+#undef TEST_DATA
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/floppy/run_rdonly.sh b/tools/testing/selftests/floppy/run_rdonly.sh
new file mode 100755
index 000000000000..a358a1b6e58d
--- /dev/null
+++ b/tools/testing/selftests/floppy/run_rdonly.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+source "$(dirname $0)"/lib.sh
+
+while getopts d flag; do
+ case "${flag}" in
+ d) debug=1;;
+ esac
+done
+
+if [ -z $debug ]; then
+ trap "rm -rf testdir" EXIT
+fi
+mkdir -p testdir
+echo -n TEST > testdir/test
+
+gen_cpio_list rdonly
+gen_initrd rdonly
+run_qemu_rdonly_fat testdir $debug
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [RFC PATCH 5/5] selftests: floppy: add basic rdwr tests
2021-08-18 15:46 [RFC PATCH 0/5] selftests: floppy: basic tests Denis Efremov
` (3 preceding siblings ...)
2021-08-18 15:46 ` [RFC PATCH 4/5] selftests: floppy: add basic tests for a readonly disk Denis Efremov
@ 2021-08-18 15:46 ` Denis Efremov
4 siblings, 0 replies; 21+ messages in thread
From: Denis Efremov @ 2021-08-18 15:46 UTC (permalink / raw)
To: linux-kselftest
Cc: Denis Efremov, linux-kernel, linux-block, Jens Axboe,
Jiri Kosina, Willy Tarreau
Add basic tests for a floppy with writable disk. "rdwr" test
works under following assumptions:
- writable disk in "/dev/fd0"
To simulate the conditions and automate the testing process there is
"run_rdwr.sh".
Signed-off-by: Denis Efremov <efremov@linux.com>
---
tools/testing/selftests/floppy/.gitignore | 1 +
tools/testing/selftests/floppy/Makefile | 4 +-
tools/testing/selftests/floppy/lib.sh | 4 ++
tools/testing/selftests/floppy/rdwr.c | 67 ++++++++++++++++++++++
tools/testing/selftests/floppy/run_rdwr.sh | 22 +++++++
5 files changed, 96 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/floppy/rdwr.c
create mode 100755 tools/testing/selftests/floppy/run_rdwr.sh
diff --git a/tools/testing/selftests/floppy/.gitignore b/tools/testing/selftests/floppy/.gitignore
index 7642dc0ef281..f53e70197edd 100644
--- a/tools/testing/selftests/floppy/.gitignore
+++ b/tools/testing/selftests/floppy/.gitignore
@@ -5,3 +5,4 @@ testdir/
empty
rdonly
+rdwr
diff --git a/tools/testing/selftests/floppy/Makefile b/tools/testing/selftests/floppy/Makefile
index ed8fdeb79aea..c5d010dd4445 100644
--- a/tools/testing/selftests/floppy/Makefile
+++ b/tools/testing/selftests/floppy/Makefile
@@ -1,8 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS := -static -I../../../../usr/include
-TEST_PROGS := run_empty.sh run_rdonly.sh
-TEST_GEN_FILES := init empty rdonly
+TEST_PROGS := run_empty.sh run_rdonly.sh run_rdwr.sh
+TEST_GEN_FILES := init empty rdonly rdwr
TEST_FILES := lib.sh
include ../lib.mk
diff --git a/tools/testing/selftests/floppy/lib.sh b/tools/testing/selftests/floppy/lib.sh
index 9988be187bc9..0eab702b355a 100644
--- a/tools/testing/selftests/floppy/lib.sh
+++ b/tools/testing/selftests/floppy/lib.sh
@@ -61,3 +61,7 @@ run_qemu_rdonly_fat() {
$run -drive file=fat:floppy:"$1",index=0,if=floppy,readonly
}
+run_qemu_rdwr_img() {
+ detect_debug "$2"
+ $run -drive file="$1",index=0,if=floppy,format=raw
+}
diff --git a/tools/testing/selftests/floppy/rdwr.c b/tools/testing/selftests/floppy/rdwr.c
new file mode 100644
index 000000000000..44ad18701530
--- /dev/null
+++ b/tools/testing/selftests/floppy/rdwr.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/mount.h>
+#include <errno.h>
+#include <linux/fd.h>
+#include "../kselftest_harness.h"
+
+FIXTURE(floppy) {
+ const char *dev;
+};
+
+FIXTURE_SETUP(floppy)
+{
+ int fd;
+ struct floppy_drive_params params;
+
+ self->dev = "/dev/fd0";
+ if (access(self->dev, F_OK))
+ ksft_exit_skip("No floppy device found\n");
+ if (access(self->dev, R_OK))
+ ksft_exit_skip("Floppy is not read accessible\n");
+ if (access(self->dev, W_OK))
+ ksft_exit_skip("Floppy is not write accessible\n");
+
+ fd = open("/dev/fd0", O_ACCMODE|O_NDELAY);
+ EXPECT_EQ(0, ioctl(fd, FDGETDRVPRM, ¶ms));
+ params.flags |= FD_DEBUG;
+ EXPECT_EQ(0, ioctl(fd, FDSETDRVPRM, ¶ms));
+ close(fd);
+}
+
+FIXTURE_TEARDOWN(floppy)
+{
+}
+
+TEST_F(floppy, write)
+{
+#define TEST_DATA "TEST_WRITE"
+ int fd;
+ char test[] = TEST_DATA;
+
+ fd = open(self->dev, O_RDWR);
+ ASSERT_GT(fd, 0);
+
+ ASSERT_EQ(sizeof(test), write(fd, test, sizeof(test)));
+ ASSERT_EQ(sizeof(test), read(fd, test, sizeof(test)));
+ ASSERT_NE(0, strncmp(TEST_DATA, test, sizeof(TEST_DATA)));
+
+ ASSERT_EQ(close(fd), 0);
+#undef TEST_DATA
+}
+
+TEST_F(floppy, ioctl_disk_writable)
+{
+ int fd;
+ struct floppy_drive_struct drive;
+
+ fd = open(self->dev, O_RDONLY|O_NDELAY);
+ ASSERT_GT(fd, 0);
+ ASSERT_EQ(0, ioctl(fd, FDGETDRVSTAT, &drive));
+ ASSERT_TRUE(drive.flags & FD_DISK_WRITABLE);
+ ASSERT_EQ(close(fd), 0);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/floppy/run_rdwr.sh b/tools/testing/selftests/floppy/run_rdwr.sh
new file mode 100755
index 000000000000..0ebe8bd6bc69
--- /dev/null
+++ b/tools/testing/selftests/floppy/run_rdwr.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+
+set -e
+
+source "$(dirname $0)"/lib.sh
+
+while getopts d flag; do
+ case "${flag}" in
+ d) debug=1;;
+ esac
+done
+
+if [ -z $debug ]; then
+ trap "rm -rf testdir" EXIT
+fi
+mkdir -p testdir
+head -c 1474560 /dev/zero > testdir/floppy.raw
+
+gen_cpio_list rdwr
+gen_initrd rdwr
+run_qemu_rdwr_img testdir/floppy.raw $debug
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread