| Message ID | 20230602085902.59006-1-franziska.naepelt@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] certs/extract-cert: Fix checkpatch issues | expand |
On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote: > The following issues are fixed: > - WARNING: Missing or malformed SPDX-License-Identifier tag > - ERROR: trailing statements should be on next line > - WARNING: braces {} are not necessary for single statement blocks > - ERROR: space required before the open parenthesis '(' > - ERROR: code indent should use tabs where possible > - WARNING: please, no spaces at the start of a line > - WARNING: Missing a blank line after declarations Again, write the patch description in imperative mood (e.g. "Do foo"). > +// SPDX-License-Identifier: LGPL-2.1 > /* Extract X.509 certificate in DER form from PKCS#11 or PEM. > * > * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved. Nope. The license boilerplate says LGPL 2.1 or any later version, so the corresponding SPDX tag should have been: ``` // SPDX-License-Identifier: LGPL-2.1-or-later ``` And please also delete the boilerplate and separate this SPDX conversion into its own patch. Thanks.
On Fri Jun 2, 2023 at 11:59 AM EEST, Franziska Naepelt wrote: > The following issues are fixed: > - WARNING: Missing or malformed SPDX-License-Identifier tag > - ERROR: trailing statements should be on next line > - WARNING: braces {} are not necessary for single statement blocks > - ERROR: space required before the open parenthesis '(' > - ERROR: code indent should use tabs where possible > - WARNING: please, no spaces at the start of a line > - WARNING: Missing a blank line after declarations > > Closes: https://lore.kernel.org/oe-kbuild-all/202306021040.UTvXfH5J-lkp@intel.com/ > Closes: https://lore.kernel.org/oe-kbuild-all/202306021102.zQU95cMI-lkp@intel.com/ > Remove the empty line. > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Franziska Naepelt <franziska.naepelt@gmail.com> Fixes tag? > --- > v2: > - revert noreturn changes to fix build issues > --- > certs/extract-cert.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/certs/extract-cert.c b/certs/extract-cert.c > index 70e9ec89d87d..96c0728bf4d1 100644 > --- a/certs/extract-cert.c > +++ b/certs/extract-cert.c > @@ -1,3 +1,4 @@ > +// SPDX-License-Identifier: LGPL-2.1 > /* Extract X.509 certificate in DER form from PKCS#11 or PEM. > * > * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved. > @@ -63,7 +64,8 @@ static void drain_openssl_errors(void) > > if (ERR_peek_error() == 0) > return; > - while (ERR_get_error_line(&file, &line)) {} > + while (ERR_get_error_line(&file, &line)) > + ; > } > > #define ERR(cond, fmt, ...) \ > @@ -73,7 +75,7 @@ static void drain_openssl_errors(void) > if (__cond) { \ > err(1, fmt, ## __VA_ARGS__); \ > } \ > - } while(0) > + } while (0) > > static const char *key_pass; > static BIO *wb; > @@ -107,7 +109,7 @@ int main(int argc, char **argv) > if (verbose_env && strchr(verbose_env, '1')) > verbose = true; > > - key_pass = getenv("KBUILD_SIGN_PIN"); > + key_pass = getenv("KBUILD_SIGN_PIN"); > > if (argc != 3) > format(); > @@ -118,6 +120,7 @@ int main(int argc, char **argv) > if (!cert_src[0]) { > /* Invoked with no input; create empty file */ > FILE *f = fopen(cert_dst, "wb"); > + > ERR(!f, "%s", cert_dst); > fclose(f); > exit(0); > @@ -155,6 +158,7 @@ int main(int argc, char **argv) > x509 = PEM_read_bio_X509(b, NULL, NULL, NULL); > if (wb && !x509) { > unsigned long err = ERR_peek_last_error(); > + > if (ERR_GET_LIB(err) == ERR_LIB_PEM && > ERR_GET_REASON(err) == PEM_R_NO_START_LINE) { > ERR_clear_error(); > > base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4 > -- > 2.39.2 (Apple Git-143) IMHO should be split to separate commits with fixes tags for trackability sake. My guess is that fixes tag is missing because this commit is bundling a pile of stuff. BR, Jarkko
On Tue Jun 6, 2023 at 3:38 PM EEST, Jarkko Sakkinen wrote: > On Fri Jun 2, 2023 at 11:59 AM EEST, Franziska Naepelt wrote: > > The following issues are fixed: > > - WARNING: Missing or malformed SPDX-License-Identifier tag > > - ERROR: trailing statements should be on next line > > - WARNING: braces {} are not necessary for single statement blocks > > - ERROR: space required before the open parenthesis '(' > > - ERROR: code indent should use tabs where possible > > - WARNING: please, no spaces at the start of a line > > - WARNING: Missing a blank line after declarations > > > > Closes: https://lore.kernel.org/oe-kbuild-all/202306021040.UTvXfH5J-lkp@intel.com/ > > Closes: https://lore.kernel.org/oe-kbuild-all/202306021102.zQU95cMI-lkp@intel.com/ > > > > Remove the empty line. > > > Reported-by: kernel test robot <lkp@intel.com> > > Signed-off-by: Franziska Naepelt <franziska.naepelt@gmail.com> > > Fixes tag? > > > --- > > v2: > > - revert noreturn changes to fix build issues > > --- > > certs/extract-cert.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/certs/extract-cert.c b/certs/extract-cert.c > > index 70e9ec89d87d..96c0728bf4d1 100644 > > --- a/certs/extract-cert.c > > +++ b/certs/extract-cert.c > > @@ -1,3 +1,4 @@ > > +// SPDX-License-Identifier: LGPL-2.1 > > /* Extract X.509 certificate in DER form from PKCS#11 or PEM. > > * > > * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved. > > @@ -63,7 +64,8 @@ static void drain_openssl_errors(void) > > > > if (ERR_peek_error() == 0) > > return; > > - while (ERR_get_error_line(&file, &line)) {} > > + while (ERR_get_error_line(&file, &line)) > > + ; > > } > > > > #define ERR(cond, fmt, ...) \ > > @@ -73,7 +75,7 @@ static void drain_openssl_errors(void) > > if (__cond) { \ > > err(1, fmt, ## __VA_ARGS__); \ > > } \ > > - } while(0) > > + } while (0) > > > > static const char *key_pass; > > static BIO *wb; > > @@ -107,7 +109,7 @@ int main(int argc, char **argv) > > if (verbose_env && strchr(verbose_env, '1')) > > verbose = true; > > > > - key_pass = getenv("KBUILD_SIGN_PIN"); > > + key_pass = getenv("KBUILD_SIGN_PIN"); > > > > if (argc != 3) > > format(); > > @@ -118,6 +120,7 @@ int main(int argc, char **argv) > > if (!cert_src[0]) { > > /* Invoked with no input; create empty file */ > > FILE *f = fopen(cert_dst, "wb"); > > + > > ERR(!f, "%s", cert_dst); > > fclose(f); > > exit(0); > > @@ -155,6 +158,7 @@ int main(int argc, char **argv) > > x509 = PEM_read_bio_X509(b, NULL, NULL, NULL); > > if (wb && !x509) { > > unsigned long err = ERR_peek_last_error(); > > + > > if (ERR_GET_LIB(err) == ERR_LIB_PEM && > > ERR_GET_REASON(err) == PEM_R_NO_START_LINE) { > > ERR_clear_error(); > > > > base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4 > > -- > > 2.39.2 (Apple Git-143) > > IMHO should be split to separate commits with fixes tags for > trackability sake. > > My guess is that fixes tag is missing because this commit is > bundling a pile of stuff. Why? I mean I do get it might sound cutting hairs, so here's a big longer explanation. When you look up for a victim commit for a bug that actually screws up run-time behaviour in a way or another, exactly these "random selection of fixes" really can make you use an inappropriate vocabulary, and you *really* have to meditate not to spill that garbage online :-) Exactly because of this carefully localized fixes are very important. If you don't do it, your fix is counter-productive for the codebase IMHO. BR, Jarkko
On Tue Jun 6, 2023 at 3:44 PM EEST, Jarkko Sakkinen wrote: > On Tue Jun 6, 2023 at 3:38 PM EEST, Jarkko Sakkinen wrote: > > On Fri Jun 2, 2023 at 11:59 AM EEST, Franziska Naepelt wrote: > > > The following issues are fixed: > > > - WARNING: Missing or malformed SPDX-License-Identifier tag > > > - ERROR: trailing statements should be on next line > > > - WARNING: braces {} are not necessary for single statement blocks > > > - ERROR: space required before the open parenthesis '(' > > > - ERROR: code indent should use tabs where possible > > > - WARNING: please, no spaces at the start of a line > > > - WARNING: Missing a blank line after declarations > > > > > > Closes: https://lore.kernel.org/oe-kbuild-all/202306021040.UTvXfH5J-lkp@intel.com/ > > > Closes: https://lore.kernel.org/oe-kbuild-all/202306021102.zQU95cMI-lkp@intel.com/ > > > > > > > Remove the empty line. > > > > > Reported-by: kernel test robot <lkp@intel.com> > > > Signed-off-by: Franziska Naepelt <franziska.naepelt@gmail.com> > > > > Fixes tag? > > > > > --- > > > v2: > > > - revert noreturn changes to fix build issues > > > --- > > > certs/extract-cert.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/certs/extract-cert.c b/certs/extract-cert.c > > > index 70e9ec89d87d..96c0728bf4d1 100644 > > > --- a/certs/extract-cert.c > > > +++ b/certs/extract-cert.c > > > @@ -1,3 +1,4 @@ > > > +// SPDX-License-Identifier: LGPL-2.1 > > > /* Extract X.509 certificate in DER form from PKCS#11 or PEM. > > > * > > > * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved. > > > @@ -63,7 +64,8 @@ static void drain_openssl_errors(void) > > > > > > if (ERR_peek_error() == 0) > > > return; > > > - while (ERR_get_error_line(&file, &line)) {} > > > + while (ERR_get_error_line(&file, &line)) > > > + ; > > > } > > > > > > #define ERR(cond, fmt, ...) \ > > > @@ -73,7 +75,7 @@ static void drain_openssl_errors(void) > > > if (__cond) { \ > > > err(1, fmt, ## __VA_ARGS__); \ > > > } \ > > > - } while(0) > > > + } while (0) > > > > > > static const char *key_pass; > > > static BIO *wb; > > > @@ -107,7 +109,7 @@ int main(int argc, char **argv) > > > if (verbose_env && strchr(verbose_env, '1')) > > > verbose = true; > > > > > > - key_pass = getenv("KBUILD_SIGN_PIN"); > > > + key_pass = getenv("KBUILD_SIGN_PIN"); > > > > > > if (argc != 3) > > > format(); > > > @@ -118,6 +120,7 @@ int main(int argc, char **argv) > > > if (!cert_src[0]) { > > > /* Invoked with no input; create empty file */ > > > FILE *f = fopen(cert_dst, "wb"); > > > + > > > ERR(!f, "%s", cert_dst); > > > fclose(f); > > > exit(0); > > > @@ -155,6 +158,7 @@ int main(int argc, char **argv) > > > x509 = PEM_read_bio_X509(b, NULL, NULL, NULL); > > > if (wb && !x509) { > > > unsigned long err = ERR_peek_last_error(); > > > + > > > if (ERR_GET_LIB(err) == ERR_LIB_PEM && > > > ERR_GET_REASON(err) == PEM_R_NO_START_LINE) { > > > ERR_clear_error(); > > > > > > base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4 > > > -- > > > 2.39.2 (Apple Git-143) > > > > IMHO should be split to separate commits with fixes tags for > > trackability sake. > > > > My guess is that fixes tag is missing because this commit is > > bundling a pile of stuff. > > Why? I mean I do get it might sound cutting hairs, so here's a > big longer explanation. > > When you look up for a victim commit for a bug that actually screws up > run-time behaviour in a way or another, exactly these "random selection > of fixes" really can make you use an inappropriate vocabulary, and you > *really* have to meditate not to spill that garbage online :-) > > Exactly because of this carefully localized fixes are very important. > If you don't do it, your fix is counter-productive for the codebase > IMHO. ... not to mention downstream distribution kernels. We maintain upstream and do not obviously proactively support downstream *but* at the same time we probably do not want to make backporting bug fixes more difficult than it is necessary. BR, Jarkko
On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote: > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote: > > The following issues are fixed: > > - WARNING: Missing or malformed SPDX-License-Identifier tag > > - ERROR: trailing statements should be on next line > > - WARNING: braces {} are not necessary for single statement blocks > > - ERROR: space required before the open parenthesis '(' > > - ERROR: code indent should use tabs where possible > > - WARNING: please, no spaces at the start of a line > > - WARNING: Missing a blank line after declarations > > Again, write the patch description in imperative mood (e.g. "Do foo"). > Why do you care about imperative tense? Imperative tense doesn't matter. What matters is that you can understand the issue and how it looks like to the user. I was working with a group of foreign students and it was painful to see the contortions that they went through to make a commit message imperative. It's like saying "Bake a cake", "Ok, now bake it while juggling." The cake ends up worse. And the commit message end up worse when we force nonsense rules like this. That said the rest of your comments are correct. Franziska, I kind of feel like you should start in drivers/staging/ because you're sending beginner patches to kind of core parts of the kernel where people are busy. Most likely your going to have to send a bunch of iterations of the patch. In drivers/staging/ we don't mind reviewing newbie patches. regards, dan carpenter
Jarkko Sakkinen <jarkko@kernel.org> wrote: > Fixes tag? That's not really necessary in this case. It's not fixing any bugs so much as keeping checkpatch happy, so there's not much point backporting the patches. > IMHO should be split to separate commits with fixes tags for > trackability sake. I think a single patch is fine for what it's doing. Let's not add a bunch of individual one-liner keeping-checkpatch-happy patches. David
On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote: > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote: > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote: > > > The following issues are fixed: > > > - WARNING: Missing or malformed SPDX-License-Identifier tag > > > - ERROR: trailing statements should be on next line > > > - WARNING: braces {} are not necessary for single statement blocks > > > - ERROR: space required before the open parenthesis '(' > > > - ERROR: code indent should use tabs where possible > > > - WARNING: please, no spaces at the start of a line > > > - WARNING: Missing a blank line after declarations > > > > Again, write the patch description in imperative mood (e.g. "Do foo"). > > > > Why do you care about imperative tense? Imperative tense doesn't > matter. What matters is that you can understand the issue and how it > looks like to the user. I was working with a group of foreign students > and it was painful to see the contortions that they went through to make > a commit message imperative. It's like saying "Bake a cake", "Ok, now > bake it while juggling." The cake ends up worse. And the commit > message end up worse when we force nonsense rules like this. How about a simple and stupid reason? Usually I write commit message without caring about this. Then I rewrite the commit message and 9/10 it gets shorter. Based on empirical experience, imperative form has minimum amount of extra words. I came up with this convention first when submitting patches to x86, and over time it has started to make sense to me. Especially for multi patch sets too verbose language tends to be super tiring for non-native English speaker. One should optimize the language in those: say *exactly* what is needed, and not more. If this not the case, I tend to move these patch sets very last in my queue. It's not a "punishment". It's more like that I really have to take the time to read the prose... BR, Jarkko
On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote: > On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote: > > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote: > > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote: > > > > The following issues are fixed: > > > > - WARNING: Missing or malformed SPDX-License-Identifier tag > > > > - ERROR: trailing statements should be on next line > > > > - WARNING: braces {} are not necessary for single statement blocks > > > > - ERROR: space required before the open parenthesis '(' > > > > - ERROR: code indent should use tabs where possible > > > > - WARNING: please, no spaces at the start of a line > > > > - WARNING: Missing a blank line after declarations > > > > > > Again, write the patch description in imperative mood (e.g. "Do foo"). > > > > > > > Why do you care about imperative tense? Imperative tense doesn't > > matter. What matters is that you can understand the issue and how it > > looks like to the user. I was working with a group of foreign students > > and it was painful to see the contortions that they went through to make > > a commit message imperative. It's like saying "Bake a cake", "Ok, now > > bake it while juggling." The cake ends up worse. And the commit > > message end up worse when we force nonsense rules like this. > > How about a simple and stupid reason? > > Usually I write commit message without caring about this. Then I rewrite > the commit message and 9/10 it gets shorter. Based on empirical > experience, imperative form has minimum amount of extra words. > I'm looking through the git log to see if it's true the imperative tense commit message are shorter and better and neither one of those things is obvious to me. This patch had an imperative subject already so it was already kind of imperative. Does every sentence have to be imperative or can you just add a "Fix it." to the end? I don't want to belittle the challenges you face around the English language but I think students were less fluent than you are. So maybe imperative tense works for you but it definitely made their commit messages far worse. regards, dan carpenter
On Tue Jun 6, 2023 at 6:25 PM EEST, Dan Carpenter wrote: > On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote: > > On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote: > > > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote: > > > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote: > > > > > The following issues are fixed: > > > > > - WARNING: Missing or malformed SPDX-License-Identifier tag > > > > > - ERROR: trailing statements should be on next line > > > > > - WARNING: braces {} are not necessary for single statement blocks > > > > > - ERROR: space required before the open parenthesis '(' > > > > > - ERROR: code indent should use tabs where possible > > > > > - WARNING: please, no spaces at the start of a line > > > > > - WARNING: Missing a blank line after declarations > > > > > > > > Again, write the patch description in imperative mood (e.g. "Do foo"). > > > > > > > > > > Why do you care about imperative tense? Imperative tense doesn't > > > matter. What matters is that you can understand the issue and how it > > > looks like to the user. I was working with a group of foreign students > > > and it was painful to see the contortions that they went through to make > > > a commit message imperative. It's like saying "Bake a cake", "Ok, now > > > bake it while juggling." The cake ends up worse. And the commit > > > message end up worse when we force nonsense rules like this. > > > > How about a simple and stupid reason? > > > > Usually I write commit message without caring about this. Then I rewrite > > the commit message and 9/10 it gets shorter. Based on empirical > > experience, imperative form has minimum amount of extra words. > > > > I'm looking through the git log to see if it's true the imperative tense > commit message are shorter and better and neither one of those things is > obvious to me. > > This patch had an imperative subject already so it was already kind of > imperative. Does every sentence have to be imperative or can you just > add a "Fix it." to the end? > > I don't want to belittle the challenges you face around the English > language but I think students were less fluent than you are. So maybe > imperative tense works for you but it definitely made their commit > messages far worse. Yeah, I was not trying to oppose, just reasoning why I like it more. For a single patch, this does not really matter anyway :-) BR, Jarkko
Am Di., 6. Juni 2023 um 18:03 Uhr schrieb Jarkko Sakkinen <jarkko@kernel.org>: > On Tue Jun 6, 2023 at 6:25 PM EEST, Dan Carpenter wrote: > > On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote: > > > On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote: > > > > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote: > > > > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote: > > > > > > The following issues are fixed: > > > > > > - WARNING: Missing or malformed SPDX-License-Identifier tag > > > > > > - ERROR: trailing statements should be on next line > > > > > > - WARNING: braces {} are not necessary for single statement blocks > > > > > > - ERROR: space required before the open parenthesis '(' > > > > > > - ERROR: code indent should use tabs where possible > > > > > > - WARNING: please, no spaces at the start of a line > > > > > > - WARNING: Missing a blank line after declarations > > > > > > > > > > Again, write the patch description in imperative mood (e.g. "Do foo"). > > > > > > > > > > > > > Why do you care about imperative tense? Imperative tense doesn't > > > > matter. What matters is that you can understand the issue and how it > > > > looks like to the user. I was working with a group of foreign students > > > > and it was painful to see the contortions that they went through to make > > > > a commit message imperative. It's like saying "Bake a cake", "Ok, now > > > > bake it while juggling." The cake ends up worse. And the commit > > > > message end up worse when we force nonsense rules like this. > > > > > > How about a simple and stupid reason? > > > > > > Usually I write commit message without caring about this. Then I rewrite > > > the commit message and 9/10 it gets shorter. Based on empirical > > > experience, imperative form has minimum amount of extra words. > > > > > > > I'm looking through the git log to see if it's true the imperative tense > > commit message are shorter and better and neither one of those things is > > obvious to me. > > > > This patch had an imperative subject already so it was already kind of > > imperative. Does every sentence have to be imperative or can you just > > add a "Fix it." to the end? > > > > I don't want to belittle the challenges you face around the English > > language but I think students were less fluent than you are. So maybe > > imperative tense works for you but it definitely made their commit > > messages far worse. > > Yeah, I was not trying to oppose, just reasoning why I like it more. > > For a single patch, this does not really matter anyway :-) > > BR, Jarkko I'm a bit puzzled now since there are different opinions on my patch. I'm struggling to draw a conclusion whether to split the patch into smaller single line patches or not. I'd propose to split it into two patches: * One for SPDX license tag fix * One for spacing, tab, blank line, unnecessary braces etc. And fix the remarks related to SPDX license tag and the use of imperative. If you agree I'm happy to provide two new patches. Anyway, as per Dan's proposal I'll continue to work in drivers/staging. Thanks, Franziska
On Tue, Jun 06, 2023 at 07:59:02PM +0200, Franziska Näpelt wrote: > Am Di., 6. Juni 2023 um 18:03 Uhr schrieb Jarkko Sakkinen <jarkko@kernel.org>: > > On Tue Jun 6, 2023 at 6:25 PM EEST, Dan Carpenter wrote: > > > On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote: > > > > On Tue Jun 6, 2023 at 4:38 PM EEST, Dan Carpenter wrote: > > > > > On Tue, Jun 06, 2023 at 07:28:52PM +0700, Bagas Sanjaya wrote: > > > > > > On Fri, Jun 02, 2023 at 10:59:02AM +0200, Franziska Naepelt wrote: > > > > > > > The following issues are fixed: > > > > > > > - WARNING: Missing or malformed SPDX-License-Identifier tag > > > > > > > - ERROR: trailing statements should be on next line > > > > > > > - WARNING: braces {} are not necessary for single statement blocks > > > > > > > - ERROR: space required before the open parenthesis '(' > > > > > > > - ERROR: code indent should use tabs where possible > > > > > > > - WARNING: please, no spaces at the start of a line > > > > > > > - WARNING: Missing a blank line after declarations > > > > > > [ snip ] > > I'm a bit puzzled now since there are different opinions on my patch. > I'm struggling to draw a conclusion whether to split the patch into smaller > single line patches or not. > > I'd propose to split it into two patches: > * One for SPDX license tag fix > * One for spacing, tab, blank line, unnecessary braces etc. You should definitely pull the SPDX change into its own patch because it's sightly controversial and important. In drivers/staging/ we would say pull each type of checkpatch warning into its own patch so it would be something like 6 patches. But I don't know how it's done in this subsystem. I feel like Greg maybe goes overboard on splitting patches up, but the advantage of Greg's system is that it's easy to explain the rules to newbies. There is a lot about staging/ which designed around newbies. If I'm totally honest, in a lot of subsystems the policy is just leave it alone. Don't bother cleaning up checkpatch stuff because it just creates more work and makes the git log noisier. regards, dan carpenter
On Tue, Jun 06, 2023 at 18:25:24 +0300, Dan Carpenter wrote: > I'm looking through the git log to see if it's true the imperative tense > commit message are shorter and better and neither one of those things is > obvious to me. > > This patch had an imperative subject already so it was already kind of > imperative. Does every sentence have to be imperative or can you just > add a "Fix it." to the end? I don't know about the length argument, but it feels like it reads better when skimming summaries with the imperative mood. The way I think about it is that the subject should complete the phrase: When applied, this patch will… The body then gives more context and description as necessary. I don't really worry so much about the mood/tense/whatever in the body except that I try to use the present tense for anything the patch is doing and past for any historical context. I understand that kernel maintainers may care a lot more about it though. Basically, a patch, on its own, does nothing (just like a recipe). It is only when it is applied that anything actually happens. I read it as "`git apply`, please $summary". --Ben
On Wed Jun 7, 2023 at 12:43 AM EEST, Ben Boeckel wrote: > On Tue, Jun 06, 2023 at 18:25:24 +0300, Dan Carpenter wrote: > > I'm looking through the git log to see if it's true the imperative tense > > commit message are shorter and better and neither one of those things is > > obvious to me. > > > > This patch had an imperative subject already so it was already kind of > > imperative. Does every sentence have to be imperative or can you just > > add a "Fix it." to the end? > > I don't know about the length argument, but it feels like it reads > better when skimming summaries with the imperative mood. The way I think > about it is that the subject should complete the phrase: > > When applied, this patch will… > > The body then gives more context and description as necessary. I don't > really worry so much about the mood/tense/whatever in the body except > that I try to use the present tense for anything the patch is doing and > past for any historical context. I understand that kernel maintainers > may care a lot more about it though. > > Basically, a patch, on its own, does nothing (just like a recipe). It is > only when it is applied that anything actually happens. I read it as > "`git apply`, please $summary". > > --Ben +1 BR, Jarkko
On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote: > It's not a "punishment". It's more like that I really have to take the > time to read the prose... The thing about imperative tense is that it was used as a punishment on me once five years ago. I wrote a quite bad commit message and a senior maintainer told me to re-write it properly and I realized that it was true. My commit message was bad. So I wrote a proper commit message. And then he yelled at me, "Can't you follow simple directions and write it in imperative tense like the documentation says? Are you a shithead?" So then I swore I would never talk to him again or to anyone who enforced the imperative tense rule. That has only happened once in the intervening years. I told the maintainer, "Fine. Re-write the commit message however you like and give me Reported-by credit." This was a cheeky response and it made the maintainer enraged. I guess he thought that my boss would force me to fix the bug or something? I felt bad for the Intel developer who had to fix my bug instead because I knew that the maintainer was going to be super angry if he gave me reported-by credit so I had put him in a bind. I almost re-wrote the commit message so that he wouldn't have to deal with that. Maybe this is how mothers feel when they try to take abuse from an angry husband instead of letting their kids suffer. But I am a bad mother and I left. My boss would never have forced me to deal with that. When he left for a different company he said, "Dan, I'm transitioning and XXX is taking over me and I have told him all your weirdness so he is prepared." And it was a huge comfort to me because I know what my weakness are. You people on this thread all seem super nice. And you're right that we should always try to be improve every aspect of our craft. When Jarkko talked about people who write too long commit messages, I thought about one developer in particular who writes too long commit messages. He writes in imperative tense. He takes everything so seriously and he's never seen a rule without following it. His patches are always right. People have told him that his commit messages are bad and too long and those people are right. But they need to shut up. The good things that he does and the bad things that he does are all part of the same package. He can't change and I don't want him to feel anything but welcome. It's hard to be a good kernel developer without being at least slightly obsessive. Both developers and maintainers are that way. And I deal with a lot of people and accomodating maintainers you disagree with is part of the job. So long as everyone is kind to each other. That's the main thing. regards, dan carpenter
On Fri Jun 9, 2023 at 5:01 PM EEST, Dan Carpenter wrote: > On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote: > > It's not a "punishment". It's more like that I really have to take the > > time to read the prose... > > The thing about imperative tense is that it was used as a punishment on > me once five years ago. I wrote a quite bad commit message and a senior > maintainer told me to re-write it properly and I realized that it was > true. My commit message was bad. So I wrote a proper commit message. > And then he yelled at me, "Can't you follow simple directions and write > it in imperative tense like the documentation says? Are you a > shithead?" Wow :-O I'm totally against name calling or any sort of shittiness like that, and all for co-operation. Just told my personal thoughts on the matter. I'm sorry that this happened to you. > > So then I swore I would never talk to him again or to anyone who > enforced the imperative tense rule. That has only happened once in the > intervening years. I told the maintainer, "Fine. Re-write the commit > message however you like and give me Reported-by credit." This was a > cheeky response and it made the maintainer enraged. I guess he thought > that my boss would force me to fix the bug or something? I felt bad for > the Intel developer who had to fix my bug instead because I knew that > the maintainer was going to be super angry if he gave me reported-by > credit so I had put him in a bind. I almost re-wrote the commit message > so that he wouldn't have to deal with that. Maybe this is how mothers > feel when they try to take abuse from an angry husband instead of > letting their kids suffer. But I am a bad mother and I left. > > My boss would never have forced me to deal with that. When he left for > a different company he said, "Dan, I'm transitioning and XXX is taking > over me and I have told him all your weirdness so he is prepared." And > it was a huge comfort to me because I know what my weakness are. > > You people on this thread all seem super nice. And you're right that we > should always try to be improve every aspect of our craft. > > When Jarkko talked about people who write too long commit messages, I > thought about one developer in particular who writes too long commit > messages. He writes in imperative tense. He takes everything so > seriously and he's never seen a rule without following it. His patches > are always right. People have told him that his commit messages are bad > and too long and those people are right. But they need to shut up. The > good things that he does and the bad things that he does are all part of > the same package. He can't change and I don't want him to feel anything > but welcome. > > It's hard to be a good kernel developer without being at least slightly > obsessive. Both developers and maintainers are that way. And I deal > with a lot of people and accomodating maintainers you disagree with is > part of the job. > > So long as everyone is kind to each other. That's the main thing. I 110% agree with this. I even bookmarked this response :-) > regards, > dan carpenter BR, Jarkko
On Fri Jun 9, 2023 at 6:37 PM EEST, Jarkko Sakkinen wrote: > On Fri Jun 9, 2023 at 5:01 PM EEST, Dan Carpenter wrote: > > On Tue, Jun 06, 2023 at 05:51:09PM +0300, Jarkko Sakkinen wrote: > > > It's not a "punishment". It's more like that I really have to take the > > > time to read the prose... > > > > The thing about imperative tense is that it was used as a punishment on > > me once five years ago. I wrote a quite bad commit message and a senior > > maintainer told me to re-write it properly and I realized that it was > > true. My commit message was bad. So I wrote a proper commit message. > > And then he yelled at me, "Can't you follow simple directions and write > > it in imperative tense like the documentation says? Are you a > > shithead?" > > Wow :-O I'm totally against name calling or any sort of shittiness like > that, and all for co-operation. Just told my personal thoughts on the > matter. I'm sorry that this happened to you. > > > > > So then I swore I would never talk to him again or to anyone who > > enforced the imperative tense rule. That has only happened once in the > > intervening years. I told the maintainer, "Fine. Re-write the commit > > message however you like and give me Reported-by credit." This was a > > cheeky response and it made the maintainer enraged. I guess he thought > > that my boss would force me to fix the bug or something? I felt bad for > > the Intel developer who had to fix my bug instead because I knew that > > the maintainer was going to be super angry if he gave me reported-by > > credit so I had put him in a bind. I almost re-wrote the commit message > > so that he wouldn't have to deal with that. Maybe this is how mothers > > feel when they try to take abuse from an angry husband instead of > > letting their kids suffer. But I am a bad mother and I left. > > > > My boss would never have forced me to deal with that. When he left for > > a different company he said, "Dan, I'm transitioning and XXX is taking > > over me and I have told him all your weirdness so he is prepared." And > > it was a huge comfort to me because I know what my weakness are. > > > > You people on this thread all seem super nice. And you're right that we > > should always try to be improve every aspect of our craft. > > > > When Jarkko talked about people who write too long commit messages, I > > thought about one developer in particular who writes too long commit > > messages. He writes in imperative tense. He takes everything so > > seriously and he's never seen a rule without following it. His patches > > are always right. People have told him that his commit messages are bad > > and too long and those people are right. But they need to shut up. The > > good things that he does and the bad things that he does are all part of > > the same package. He can't change and I don't want him to feel anything > > but welcome. > > > > It's hard to be a good kernel developer without being at least slightly > > obsessive. Both developers and maintainers are that way. And I deal > > with a lot of people and accomodating maintainers you disagree with is > > part of the job. > > > > So long as everyone is kind to each other. That's the main thing. > > I 110% agree with this. I even bookmarked this response :-) To add: it is super hard and non-trivial issue to keep the balance between "nice" and "obsessive". If you are too nice, the stuff does not get fixed. If you are too obssesive, well... then you act like a jerk. So you have to try to say the truth as clearly as possible, but still keep the tone constructive. Not always easy to manage. Personally, I keep bullshit threshold from other people. There are better and worse days and that does affect your communication. So if you get one nastier email from someone 9/10 it is better just ignore the bullshit and focus on matter. Of course when it is a trend, then it is better to vocally ask for better communication. BR, Jarkko
diff --git a/certs/extract-cert.c b/certs/extract-cert.c index 70e9ec89d87d..96c0728bf4d1 100644 --- a/certs/extract-cert.c +++ b/certs/extract-cert.c @@ -1,3 +1,4 @@ +// SPDX-License-Identifier: LGPL-2.1 /* Extract X.509 certificate in DER form from PKCS#11 or PEM. * * Copyright © 2014-2015 Red Hat, Inc. All Rights Reserved. @@ -63,7 +64,8 @@ static void drain_openssl_errors(void) if (ERR_peek_error() == 0) return; - while (ERR_get_error_line(&file, &line)) {} + while (ERR_get_error_line(&file, &line)) + ; } #define ERR(cond, fmt, ...) \ @@ -73,7 +75,7 @@ static void drain_openssl_errors(void) if (__cond) { \ err(1, fmt, ## __VA_ARGS__); \ } \ - } while(0) + } while (0) static const char *key_pass; static BIO *wb; @@ -107,7 +109,7 @@ int main(int argc, char **argv) if (verbose_env && strchr(verbose_env, '1')) verbose = true; - key_pass = getenv("KBUILD_SIGN_PIN"); + key_pass = getenv("KBUILD_SIGN_PIN"); if (argc != 3) format(); @@ -118,6 +120,7 @@ int main(int argc, char **argv) if (!cert_src[0]) { /* Invoked with no input; create empty file */ FILE *f = fopen(cert_dst, "wb"); + ERR(!f, "%s", cert_dst); fclose(f); exit(0); @@ -155,6 +158,7 @@ int main(int argc, char **argv) x509 = PEM_read_bio_X509(b, NULL, NULL, NULL); if (wb && !x509) { unsigned long err = ERR_peek_last_error(); + if (ERR_GET_LIB(err) == ERR_LIB_PEM && ERR_GET_REASON(err) == PEM_R_NO_START_LINE) { ERR_clear_error();
The following issues are fixed: - WARNING: Missing or malformed SPDX-License-Identifier tag - ERROR: trailing statements should be on next line - WARNING: braces {} are not necessary for single statement blocks - ERROR: space required before the open parenthesis '(' - ERROR: code indent should use tabs where possible - WARNING: please, no spaces at the start of a line - WARNING: Missing a blank line after declarations Closes: https://lore.kernel.org/oe-kbuild-all/202306021040.UTvXfH5J-lkp@intel.com/ Closes: https://lore.kernel.org/oe-kbuild-all/202306021102.zQU95cMI-lkp@intel.com/ Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Franziska Naepelt <franziska.naepelt@gmail.com> --- v2: - revert noreturn changes to fix build issues --- certs/extract-cert.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) base-commit: 7877cb91f1081754a1487c144d85dc0d2e2e7fc4 -- 2.39.2 (Apple Git-143)