[LTP] [PATCH v2 0/2] doc: Add Maintainer Patch Review Checklist

[LTP] [PATCH v2 0/2] doc: Add Maintainer Patch Review Checklist

[Petr Vorel]

changes v1->v2:
* doc: Document test tags (new commit)
* mention patchwork, LTP wiki, Travis CI, test tags, put links to scripts

output
https://github.com/pevik/ltp/wiki/TEST#2238-test-tags
https://github.com/pevik/ltp/wiki/Maintainer-Patch-Review-Checklist

Kind regards,
Petr

Petr Vorel (2):
  doc: Document test tags
  doc: Add Maintainer Patch Review Checklist

 doc/maintainer-patch-review-checklist.txt | 53 +++++++++++++++++++
 doc/test-writing-guidelines.txt           | 62 ++++++++++++++++++++---
 2 files changed, 109 insertions(+), 6 deletions(-)
 create mode 100644 doc/maintainer-patch-review-checklist.txt

-- 
2.30.2

[LTP] [PATCH v2 1/2] doc: Document test tags

[Petr Vorel]

+ add URL for checkpatch.pl script and slightly reword related text.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 doc/test-writing-guidelines.txt | 62 +++++++++++++++++++++++++++++----
 1 file changed, 56 insertions(+), 6 deletions(-)

diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 50696e14a..16715b841 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -47,10 +47,12 @@ LTP adopted Linux kernel coding style. If you aren't familiar with its rules
 locate 'linux/Documentation/CodingStyle' in the kernel sources and read it,
 it's a well written introduction.
 
-There is also a checkpatch (see 'linux/scripts/checkpatch.pl') script that can
-be used to check your patches before the submission.
+There is also
+https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
+script from kernel git tree which can be used to check your patches before the
+submission.  Please use reasonably recent one.
 
-NOTE: If checkpatch does not report any problems, the code still may be wrong
+NOTE: If checkpatch.pl does not report any problems, the code still may be wrong
       as the tool only looks for common mistakes.
 
 1.3.2 Shell coding style
@@ -2213,6 +2215,53 @@ struct tst_test test = {
 Some tests require more than specific number of CPU. It can be defined with
 `.min_cpus = N`.
 
+2.2.38 Test tags
+^^^^^^^^^^^^^^^^
+
+Test tags are name-value pairs that can hold any test metadata.
+
+We have additional support for CVE entries, or git commit in mainline kernel,
+stable kernel or glibc git repository.  If a test is a regression test it
+should include these tags.  They are printed when test fails and added in
+docparse documentation.
+
+CVE, mainline and stable kernel git commits in a regression test for a kernel bug:
+[source,c]
+-------------------------------------------------------------------------------
+struct tst_test test = {
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "9392a27d88b9"},
+		{"linux-git", "ff002b30181d"},
+		{"linux-stable-git", "c4a23c852e80"},
+		{"CVE", "2020-29373"},
+		{}
+	}
+};
+-------------------------------------------------------------------------------
+
+Glibc git commit in a regression test for a glibc bug:
+[source,c]
+-------------------------------------------------------------------------------
+struct tst_test test = {
+	...
+	.tags = (const struct tst_tag[]) {
+		{"glibc-git", "574500a108be"},
+		{}
+	}
+};
+-------------------------------------------------------------------------------
+
+[source,c]
+-------------------------------------------------------------------------------
+struct tst_test test = {
+	...
+	.tags = (const struct tst_tag[]) {
+		{"glibc-git", "574500a108be"},
+		{}
+	}
+};
+-------------------------------------------------------------------------------
+
 2.3 Writing a testcase in shell
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
@@ -3005,10 +3054,11 @@ skips 'atexit(3)' callbacks.
 4. Test Contribution Checklist
 ------------------------------
 
-1. Test compiles and runs fine (check with -i 10 too)
-2. Checkpatch does not report any errors
+1. Test compiles and runs fine (check with `-i 10` too)
+2. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
+   does not report any errors
 3. The runtest entires are in place
-4. Test files are added into corresponding .gitignore files
+4. Test files are added into corresponding '.gitignore' files
 5. Patches apply over the latest git
 
 
-- 
2.30.2

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Petr Vorel]

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 doc/maintainer-patch-review-checklist.txt | 53 +++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 doc/maintainer-patch-review-checklist.txt

diff --git a/doc/maintainer-patch-review-checklist.txt b/doc/maintainer-patch-review-checklist.txt
new file mode 100644
index 000000000..2587287aa
--- /dev/null
+++ b/doc/maintainer-patch-review-checklist.txt
@@ -0,0 +1,53 @@
+# Maintainer Patch Review Checklist
+
+Patchset should be tested locally and ideally also in maintainer's fork in
+https://travis-ci.org/[Travis CI].
+
+NOTE: Travis does only build testing, passing the CI means only that the
+      test compiles fine on variety of different distributions and
+      releases.
+
+The test should be executed at least once locally and should PASS as well.
+
+Commit messages should have
+
+* Author's `Signed-off-by` tag
+* Committer's `Reviewed-by` or `Signed-off-by` tag
+* Check also mailing lists for other reviewers / testers
+* `Fixes: hash` if it fixes particular commit
+* `Fixes: #N` if it fixes N github issue, so it's automatically closed
+
+After patch is accepted or rejected, set correct state and archive in
+https://patchwork.ozlabs.org/project/ltp/list/[LTP patchwork instance].
+
+Also update LTP WIKI (git URL https://github.com/linux-test-project/ltp.wiki.git)
+if touch 'doc/*.txt'.
+
+## New tests
+New test should
+
+* Have a record in runtest file
+* Test should work fine with more than one iteration
+  (e.g. run with `-i 100`)
+* Have a brief description
+
+### C tests
+* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API]
+* Test files are added into corresponding '.gitignore' files
+* Check coding style with
+  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
+  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#131-c-coding-style[C coding style])
+* Docparse documentation
+* If a test is a regression test it should include tags
+  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2238-test-tags[Test tags])
+
+### Shell tests
+* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell[shell API]
+* Check coding style with
+  https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl[checkbashism.pl]
+  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style[Shell coding style])
+* If a test is a regression test it should include related kernel or glibc commits as a comment
+
+## LTP library
+For patchset touching library please check also
+https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].
-- 
2.30.2

[LTP] [PATCH v2 1/2] doc: Document test tags

[Li Wang]

Petr Vorel <pvorel@suse.cz> wrote:

+Glibc git commit in a regression test for a glibc bug:
> +[source,c]
>
> +-------------------------------------------------------------------------------
> +struct tst_test test = {
> +       ...
> +       .tags = (const struct tst_tag[]) {
> +               {"glibc-git", "574500a108be"},
> +               {}
> +       }
> +};
>
> +-------------------------------------------------------------------------------
> +
> +[source,c]
>
> +-------------------------------------------------------------------------------
> +struct tst_test test = {
> +       ...
> +       .tags = (const struct tst_tag[]) {
> +               {"glibc-git", "574500a108be"},
> +               {}
> +       }
> +};
>

I go through many times for the above two structures, but am not see any
difference:).

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210319/7ce81cd2/attachment-0001.htm>

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Li Wang]

On Fri, Mar 19, 2021 at 2:17 AM Petr Vorel <pvorel@suse.cz> wrote:

> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  doc/maintainer-patch-review-checklist.txt | 53 +++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 doc/maintainer-patch-review-checklist.txt
>
> diff --git a/doc/maintainer-patch-review-checklist.txt
> b/doc/maintainer-patch-review-checklist.txt
> new file mode 100644
> index 000000000..2587287aa
> --- /dev/null
> +++ b/doc/maintainer-patch-review-checklist.txt
> @@ -0,0 +1,53 @@
> +# Maintainer Patch Review Checklist
> +
> +Patchset should be tested locally and ideally also in maintainer's fork in
> +https://travis-ci.org/[Travis CI].
> +
> +NOTE: Travis does only build testing, passing the CI means only that the
> +      test compiles fine on variety of different distributions and
> +      releases.
> +
> +The test should be executed at least once locally and should PASS as well.
> +
> +Commit messages should have
> +
> +* Author's `Signed-off-by` tag
> +* Committer's `Reviewed-by` or `Signed-off-by` tag
>

Or adding `Acked-by:` also acceptable?

I'm not sure what's the difference between Acked-by and Reviewed-by,
but personally feel the Acked-by has more confidence in the reviewing.

In my mind,

`Acked-by` means:
code review OK, compiled OK, executed OK, has confidence it OK for all
arches

`Reviewed-by` means:
code review OK, compiled and executed OK just locally or optional, still room
for improvement/discussion

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210319/11b13771/attachment.htm>

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Li Wang]

Li Wang <liwang@redhat.com> wrote:


> +Commit messages should have
>> +
>> +* Author's `Signed-off-by` tag
>> +* Committer's `Reviewed-by` or `Signed-off-by` tag
>>
>
> Or adding `Acked-by:` also acceptable?
>
> I'm not sure what's the difference between Acked-by and Reviewed-by,
> but personally feel the Acked-by has more confidence in the reviewing.
>
> In my mind,
>
> `Acked-by` means:
> code review OK, compiled OK, executed OK, has confidence it OK for all
> arches
>
> `Reviewed-by` means:
> code review OK, compiled and executed OK just locally or optional, still room
> for improvement/discussion
>

Plz ignore my rough understanding, I find a precise description at kernel
process/submitting-patches.rst.

https://github.com/torvalds/linux/blob/master/Documentation/process/submitting-patches.rst#when-to-use-acked-by-cc-and-co-developed-by

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210319/933026c4/attachment.htm>

[LTP] [PATCH v2 1/2] doc: Document test tags

[Petr Vorel]

> Petr Vorel <pvorel@suse.cz> wrote:

> +Glibc git commit in a regression test for a glibc bug:
> > +[source,c]

> > +-------------------------------------------------------------------------------
> > +struct tst_test test = {
> > +       ...
> > +       .tags = (const struct tst_tag[]) {
> > +               {"glibc-git", "574500a108be"},
> > +               {}
> > +       }
> > +};

> > +-------------------------------------------------------------------------------
> > +
> > +[source,c]

> > +-------------------------------------------------------------------------------
> > +struct tst_test test = {
> > +       ...
> > +       .tags = (const struct tst_tag[]) {
> > +               {"glibc-git", "574500a108be"},
> > +               {}
> > +       }
> > +};


> I go through many times for the above two structures, but am not see any
> difference:).
Lol :). Thanks for pointing copy paste error, I'll remove the second one.

Kind regards,
Petr

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Petr Vorel]

> Li Wang <liwang@redhat.com> wrote:


> > +Commit messages should have
> >> +
> >> +* Author's `Signed-off-by` tag
> >> +* Committer's `Reviewed-by` or `Signed-off-by` tag


> > Or adding `Acked-by:` also acceptable?

> > I'm not sure what's the difference between Acked-by and Reviewed-by,
> > but personally feel the Acked-by has more confidence in the reviewing.

> > In my mind,

> > `Acked-by` means:
> > code review OK, compiled OK, executed OK, has confidence it OK for all
> > arches

> > `Reviewed-by` means:
> > code review OK, compiled and executed OK just locally or optional, still room
> > for improvement/discussion


> Plz ignore my rough understanding, I find a precise description at kernel
> process/submitting-patches.rst.

> https://github.com/torvalds/linux/blob/master/Documentation/process/submitting-patches.rst#when-to-use-acked-by-cc-and-co-developed-by

But you were right, I omit "tags". Maybe this?

- Check also mailing lists for other reviewers / testers
+ Check also mailing lists for other reviewers / testers tags, notes and failure reports

Kind regards,
Petr

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Cyril Hrubis]

Hi!
> ---
>  doc/maintainer-patch-review-checklist.txt | 53 +++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 doc/maintainer-patch-review-checklist.txt
> 
> diff --git a/doc/maintainer-patch-review-checklist.txt b/doc/maintainer-patch-review-checklist.txt
> new file mode 100644
> index 000000000..2587287aa
> --- /dev/null
> +++ b/doc/maintainer-patch-review-checklist.txt
> @@ -0,0 +1,53 @@
> +# Maintainer Patch Review Checklist
> +
> +Patchset should be tested locally and ideally also in maintainer's fork in
> +https://travis-ci.org/[Travis CI].
> +
> +NOTE: Travis does only build testing, passing the CI means only that the
> +      test compiles fine on variety of different distributions and
> +      releases.
> +
> +The test should be executed at least once locally and should PASS as well.
> +
> +Commit messages should have
> +
> +* Author's `Signed-off-by` tag
> +* Committer's `Reviewed-by` or `Signed-off-by` tag
> +* Check also mailing lists for other reviewers / testers
> +* `Fixes: hash` if it fixes particular commit
                                         ^
					 LTP

Let's add this here so that is clear what we mean.

> +* `Fixes: #N` if it fixes N github issue, so it's automatically closed
                              ^
			 This wording is not clear it should be:

	... if it fixes github issue number N ...

Or just:

	.. if it fixed github issue N ...

> +After patch is accepted or rejected, set correct state and archive in
> +https://patchwork.ozlabs.org/project/ltp/list/[LTP patchwork instance].
> +
> +Also update LTP WIKI (git URL https://github.com/linux-test-project/ltp.wiki.git)
> +if touch 'doc/*.txt'.
> +
> +## New tests
> +New test should
> +
> +* Have a record in runtest file
> +* Test should work fine with more than one iteration
> +  (e.g. run with `-i 100`)
> +* Have a brief description
> +
> +### C tests
> +* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API]
> +* Test files are added into corresponding '.gitignore' files
          ^
	  binaries?

> +* Check coding style with
> +  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
> +  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#131-c-coding-style[C coding style])
> +* Docparse documentation
> +* If a test is a regression test it should include tags
> +  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2238-test-tags[Test tags])
> +
> +### Shell tests
> +* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell[shell API]
> +* Check coding style with
> +  https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl[checkbashism.pl]
> +  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style[Shell coding style])
> +* If a test is a regression test it should include related kernel or glibc commits as a comment
> +
> +## LTP library
> +For patchset touching library please check also
> +https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].

Other than these minor comments it looks great.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[xuyang2018.jy]

Hi Petr
Should we add license or copyright check in this checklist? It maybe
removed when converting old case into new api.

Best Regards
Yang Xu

> Signed-off-by: Petr Vorel<pvorel@suse.cz>
> ---
>   doc/maintainer-patch-review-checklist.txt | 53 +++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
>   create mode 100644 doc/maintainer-patch-review-checklist.txt
> 
> diff --git a/doc/maintainer-patch-review-checklist.txt b/doc/maintainer-patch-review-checklist.txt
> new file mode 100644
> index 000000000..2587287aa
> --- /dev/null
> +++ b/doc/maintainer-patch-review-checklist.txt
> @@ -0,0 +1,53 @@
> +# Maintainer Patch Review Checklist
> +
> +Patchset should be tested locally and ideally also in maintainer's fork in
> +https://travis-ci.org/[Travis CI].
> +
> +NOTE: Travis does only build testing, passing the CI means only that the
> +      test compiles fine on variety of different distributions and
> +      releases.
> +
> +The test should be executed at least once locally and should PASS as well.
> +
> +Commit messages should have
> +
> +* Author's `Signed-off-by` tag
> +* Committer's `Reviewed-by` or `Signed-off-by` tag
> +* Check also mailing lists for other reviewers / testers
> +* `Fixes: hash` if it fixes particular commit
> +* `Fixes: #N` if it fixes N github issue, so it's automatically closed
> +
> +After patch is accepted or rejected, set correct state and archive in
> +https://patchwork.ozlabs.org/project/ltp/list/[LTP patchwork instance].
> +
> +Also update LTP WIKI (git URL https://github.com/linux-test-project/ltp.wiki.git)
> +if touch 'doc/*.txt'.
> +
> +## New tests
> +New test should
> +
> +* Have a record in runtest file
> +* Test should work fine with more than one iteration
> +  (e.g. run with `-i 100`)
> +* Have a brief description
> +
> +### C tests
> +* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API]
> +* Test files are added into corresponding '.gitignore' files
> +* Check coding style with
> +  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
> +  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#131-c-coding-style[C coding style])
> +* Docparse documentation
> +* If a test is a regression test it should include tags
> +  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#2238-test-tags[Test tags])
> +
> +### Shell tests
> +* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#23-writing-a-testcase-in-shell[shell API]
> +* Check coding style with
> +  https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl[checkbashism.pl]
> +  (more in https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style[Shell coding style])
> +* If a test is a regression test it should include related kernel or glibc commits as a comment
> +
> +## LTP library
> +For patchset touching library please check also
> +https://github.com/linux-test-project/ltp/wiki/LTP-Library-API-Writing-Guidelines[LTP Library API Writing Guidelines].

[LTP] [PATCH v2 1/2] doc: Document test tags

[Cyril Hrubis]

Hi!
> + add URL for checkpatch.pl script and slightly reword related text.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  doc/test-writing-guidelines.txt | 62 +++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
> index 50696e14a..16715b841 100644
> --- a/doc/test-writing-guidelines.txt
> +++ b/doc/test-writing-guidelines.txt
> @@ -47,10 +47,12 @@ LTP adopted Linux kernel coding style. If you aren't familiar with its rules
>  locate 'linux/Documentation/CodingStyle' in the kernel sources and read it,
>  it's a well written introduction.
>  
> -There is also a checkpatch (see 'linux/scripts/checkpatch.pl') script that can
> -be used to check your patches before the submission.
> +There is also
> +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
> +script from kernel git tree which can be used to check your patches before the
> +submission.  Please use reasonably recent one.
>  
> -NOTE: If checkpatch does not report any problems, the code still may be wrong
> +NOTE: If checkpatch.pl does not report any problems, the code still may be wrong
>        as the tool only looks for common mistakes.
>  
>  1.3.2 Shell coding style
> @@ -2213,6 +2215,53 @@ struct tst_test test = {
>  Some tests require more than specific number of CPU. It can be defined with
>  `.min_cpus = N`.
>  
> +2.2.38 Test tags
> +^^^^^^^^^^^^^^^^
> +
> +Test tags are name-value pairs that can hold any test metadata.
> +
> +We have additional support for CVE entries, or git commit in mainline kernel,
                                               ^        ^
					       |      commits?
					       |
					       This or should probably
					       be removed since we have
					       another one later in the
					       sentence.

> +stable kernel or glibc git repository.  If a test is a regression test it
> +should include these tags.  They are printed when test fails and added in
> +docparse documentation.
   ^
   Not sure if we should name it docparse, maybe just "... and exported
   into documentation."

> +CVE, mainline and stable kernel git commits in a regression test for a kernel bug:
> +[source,c]
> +-------------------------------------------------------------------------------
> +struct tst_test test = {
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "9392a27d88b9"},
> +		{"linux-git", "ff002b30181d"},
> +		{"linux-stable-git", "c4a23c852e80"},
> +		{"CVE", "2020-29373"},
> +		{}
> +	}
> +};
> +-------------------------------------------------------------------------------
> +
> +Glibc git commit in a regression test for a glibc bug:
> +[source,c]
> +-------------------------------------------------------------------------------
> +struct tst_test test = {
> +	...
> +	.tags = (const struct tst_tag[]) {
> +		{"glibc-git", "574500a108be"},
> +		{}
> +	}
> +};
> +-------------------------------------------------------------------------------
> +
> +[source,c]
> +-------------------------------------------------------------------------------
> +struct tst_test test = {
> +	...
> +	.tags = (const struct tst_tag[]) {
> +		{"glibc-git", "574500a108be"},
> +		{}
> +	}
> +};
> +-------------------------------------------------------------------------------
> +
>  2.3 Writing a testcase in shell
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  
> @@ -3005,10 +3054,11 @@ skips 'atexit(3)' callbacks.
>  4. Test Contribution Checklist
>  ------------------------------
>  
> -1. Test compiles and runs fine (check with -i 10 too)
> -2. Checkpatch does not report any errors
> +1. Test compiles and runs fine (check with `-i 10` too)
> +2. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl[checkpatch.pl]
> +   does not report any errors
>  3. The runtest entires are in place
> -4. Test files are added into corresponding .gitignore files
> +4. Test files are added into corresponding '.gitignore' files
           ^
	   Can we fix this to say binaries since we are fixing things?

>  5. Patches apply over the latest git

Here as well, looks great minus the few minor things.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Cyril Hrubis]

Hi!
> Should we add license or copyright check in this checklist? It maybe
> removed when converting old case into new api.

Good point, we should state the default license for new tests and also
state that the licence for tests should not change unless they are
completely rewritten.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Petr Vorel]

Hi Cyril,

> > +* Author's `Signed-off-by` tag
> > +* Committer's `Reviewed-by` or `Signed-off-by` tag
> > +* Check also mailing lists for other reviewers / testers
> > +* `Fixes: hash` if it fixes particular commit
>                                          ^
> 					 LTP

> Let's add this here so that is clear what we mean.

> > +* `Fixes: #N` if it fixes N github issue, so it's automatically closed
>                               ^
> 			 This wording is not clear it should be:

> 	... if it fixes github issue number N ...

> Or just:

> 	.. if it fixed github issue N ...

+1

> > +After patch is accepted or rejected, set correct state and archive in
> > +https://patchwork.ozlabs.org/project/ltp/list/[LTP patchwork instance].
> > +
> > +Also update LTP WIKI (git URL https://github.com/linux-test-project/ltp.wiki.git)
> > +if touch 'doc/*.txt'.
> > +
> > +## New tests
> > +New test should
> > +
> > +* Have a record in runtest file
> > +* Test should work fine with more than one iteration
> > +  (e.g. run with `-i 100`)
> > +* Have a brief description
> > +
> > +### C tests
> > +* Use new https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#22-writing-a-test-in-c[C API]
> > +* Test files are added into corresponding '.gitignore' files
>           ^
> 	  binaries?

+1 (copy pasted from Test Writing Guidelines, thus fixed it also there as you
suggest in next mail).

Kind regards,
Petr

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Petr Vorel]

Hi,

> Hi!
> > Should we add license or copyright check in this checklist? It maybe
> > removed when converting old case into new api.

Great point, thanks.

> Good point, we should state the default license for new tests and also
> state that the licence for tests should not change unless they are
> completely rewritten.


Maybe:

* license: the default license for new tests is GPL v2 or later, use
  GPL-2.0-or-later; the licence for test (e.g. GPL-2.0) should not change unless
  it's completely rewritten
* old copyrights should be kept unless test is completely rewritten

BTW this is important, specially for DMCA from Wipro. Cyril, can you point some
test from Wipro which could be considered as "completely rewritten"?

Kind regards,
Petr

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Cyril Hrubis]

Hi!
> > Good point, we should state the default license for new tests and also
> > state that the licence for tests should not change unless they are
> > completely rewritten.
> 
> 
> Maybe:
> 
> * license: the default license for new tests is GPL v2 or later, use
>   GPL-2.0-or-later; the licence for test (e.g. GPL-2.0) should not change unless
>   it's completely rewritten
> * old copyrights should be kept unless test is completely rewritten
> 
> BTW this is important, specially for DMCA from Wipro. Cyril, can you point some
> test from Wipro which could be considered as "completely rewritten"?

This is tricky topic and I'm not a lawyer, so I guess that we better
consult one.

I would expect that if you remove all the previous test code and start
from a scratch the old copyrights would not apply, but again I'm not
lawyer and I do not have in depth understanding of the copyright laws
and treaties.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Li Wang]

+Cc Alice Wang, who is a volunteer of the GNU lawyer team. I met her
several years ago in Beijing, not sure if I remember the email-address
correctly, so I just CC her in case she can read this and maybe help some
side.

Cyril Hrubis <chrubis@suse.cz> wrote:

Hi!
> > Maybe:
> >
> > * license: the default license for new tests is GPL v2 or later, use
> >   GPL-2.0-or-later; the licence for test (e.g. GPL-2.0) should not
> change unless
> >   it's completely rewritten
> > * old copyrights should be kept unless test is completely rewritten
> >
> > BTW this is important, specially for DMCA from Wipro. Cyril, can you
> point some
> > test from Wipro which could be considered as "completely rewritten"?
>
> This is tricky topic and I'm not a lawyer, so I guess that we better
> consult one.
>
> I would expect that if you remove all the previous test code and start
> from a scratch the old copyrights would not apply, but again I'm not
> lawyer and I do not have in depth understanding of the copyright laws
> and treaties.
>
> --
> Cyril Hrubis
> chrubis@suse.cz
>
>

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20210319/bfeddac3/attachment.htm>

[LTP] [PATCH v2 2/2] doc: Add Maintainer Patch Review Checklist

[Petr Vorel]

Hi all,

> +Cc Alice Wang, who is a volunteer of the GNU lawyer team. I met her
> several years ago in Beijing, not sure if I remember the email-address
> correctly, so I just CC her in case she can read this and maybe help some
> side.

Alice haven't responded, thus I pushed patchset with other fixes and created
page [1]. It'd be great to get info from other lawyer.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/Maintainer-Patch-Review-Checklist