[PATCH v1 0/3] Regarding using 'bool' appropriately

[PATCH v1 0/3] Regarding using 'bool' appropriately

[Wei Wang]

When I'm working on patch 3 to change WARN/WARN_ON to use bool for
__ret_warn_on according to the documentation in CodingStyle about using
'bool', compiler reports an error from tpm2_key_encode(), and the root
cause is that it names a variable 'bool' which conficts with the data
type name 'bool'. So fix it and add a rule in CodingStyle to avoid such
naming that causes confusion. This is also the reason that the three
patches are grouped into one patchset.

Wei Wang (3):
  security: keys: don't use data type as variable name
  Documentation/CodingStyle: do not use data type names as variable
    names
  bug: use bool for __ret_warn_on in WARN/WARN_ON

 Documentation/process/coding-style.rst    |  3 +++
 include/asm-generic/bug.h                 | 12 ++++++------
 security/keys/trusted-keys/trusted_tpm2.c |  5 +++--
 tools/include/asm/bug.h                   | 10 +++++-----
 4 files changed, 17 insertions(+), 13 deletions(-)

-- 
2.27.0

[PATCH v1 1/3] security: keys: don't use data type as variable name

[Wei Wang]

'bool' is a specific name for the data type that is an alias for
the C99 _Bool type. It shoudn't be used as variable names as that causes
too much confusion either for the reader or the compilier.

CC: James.Bottomley@HansenPartnership.com
CC: jarkko@kernel.org
Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 security/keys/trusted-keys/trusted_tpm2.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
index 2b2c8eb258d5..390d7314f5a6 100644
--- a/security/keys/trusted-keys/trusted_tpm2.c
+++ b/security/keys/trusted-keys/trusted_tpm2.c
@@ -54,12 +54,13 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
 			       asn1_oid_len(tpm2key_oid));
 
 	if (options->blobauth_len == 0) {
-		unsigned char bool[3], *w = bool;
+		unsigned char bool_val[3], *w = bool_val;
 		/* tag 0 is emptyAuth */
 		w = asn1_encode_boolean(w, w + sizeof(bool), true);
 		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
 			return PTR_ERR(w);
-		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
+		work = asn1_encode_tag(work, end_work, 0,
+				       bool_val, w - bool_val);
 	}
 
 	/*
-- 
2.27.0

Re: [PATCH v1 1/3] security: keys: don't use data type as variable name

[Jarkko Sakkinen]

On Sat, Mar 04, 2023 at 12:19:30PM +0800, Wei Wang wrote:
> 'bool' is a specific name for the data type that is an alias for
> the C99 _Bool type. It shoudn't be used as variable names as that causes
> too much confusion either for the reader or the compilier.
> 
> CC: James.Bottomley@HansenPartnership.com
> CC: jarkko@kernel.org
> Fixes: f2219745250f ("security: keys: trusted: use ASN.1 TPM2 key format for the blobs")
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  security/keys/trusted-keys/trusted_tpm2.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 2b2c8eb258d5..390d7314f5a6 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -54,12 +54,13 @@ static int tpm2_key_encode(struct trusted_key_payload *payload,
>  			       asn1_oid_len(tpm2key_oid));
>  
>  	if (options->blobauth_len == 0) {
> -		unsigned char bool[3], *w = bool;
> +		unsigned char bool_val[3], *w = bool_val;
>  		/* tag 0 is emptyAuth */
>  		w = asn1_encode_boolean(w, w + sizeof(bool), true);
>  		if (WARN(IS_ERR(w), "BUG: Boolean failed to encode"))
>  			return PTR_ERR(w);
> -		work = asn1_encode_tag(work, end_work, 0, bool, w - bool);
> +		work = asn1_encode_tag(work, end_work, 0,
> +				       bool_val, w - bool_val);
>  	}
>  
>  	/*
> -- 
> 2.27.0
> 

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@kernel.org>

BR, Jarkko

[PATCH v1 2/3] Documentation/CodingStyle: do not use data type names as variable names

[Wei Wang]

Observed some merged code uses "bool" as variable name. This is
confusion either for the reader or compilier. Add a rule to have
programmers avoid using data types as variable names.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 Documentation/process/coding-style.rst | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
index 007e49ef6cec..6d7f4069d55d 100644
--- a/Documentation/process/coding-style.rst
+++ b/Documentation/process/coding-style.rst
@@ -356,6 +356,9 @@ specification that mandates those terms. For new specifications
 translate specification usage of the terminology to the kernel coding
 standard where possible.
 
+"bool", "int", "long" etc. are specific names for data types, C
+programmers should not use them as variable names.
+
 5) Typedefs
 -----------
 
-- 
2.27.0

Re: [PATCH v1 2/3] Documentation/CodingStyle: do not use data type names as variable names

[Jonathan Corbet]

Wei Wang <wei.w.wang@intel.com> writes:

> Observed some merged code uses "bool" as variable name. This is
> confusion either for the reader or compilier. Add a rule to have
> programmers avoid using data types as variable names.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  Documentation/process/coding-style.rst | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/process/coding-style.rst b/Documentation/process/coding-style.rst
> index 007e49ef6cec..6d7f4069d55d 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -356,6 +356,9 @@ specification that mandates those terms. For new specifications
>  translate specification usage of the terminology to the kernel coding
>  standard where possible.
>  
> +"bool", "int", "long" etc. are specific names for data types, C
> +programmers should not use them as variable names.

It seems you found one place where bool was being misused.  Fixing it
was certainly the right thing to do, but I'm not convinced we need to
add clutter to the documentation for this.

Thanks,

jon

RE: [PATCH v1 2/3] Documentation/CodingStyle: do not use data type names as variable names

[Wang, Wei W]

On Saturday, March 4, 2023 11:02 PM, Jonathan Corbet wrote:
> Wei Wang <wei.w.wang@intel.com> writes:
> 
> > Observed some merged code uses "bool" as variable name. This is
> > confusion either for the reader or compilier. Add a rule to have
> > programmers avoid using data types as variable names.
> >
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > ---
> >  Documentation/process/coding-style.rst | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/process/coding-style.rst
> > b/Documentation/process/coding-style.rst
> > index 007e49ef6cec..6d7f4069d55d 100644
> > --- a/Documentation/process/coding-style.rst
> > +++ b/Documentation/process/coding-style.rst
> > @@ -356,6 +356,9 @@ specification that mandates those terms. For new
> > specifications  translate specification usage of the terminology to
> > the kernel coding  standard where possible.
> >
> > +"bool", "int", "long" etc. are specific names for data types, C
> > +programmers should not use them as variable names.
> 
> It seems you found one place where bool was being misused.  Fixing it was
> certainly the right thing to do, but I'm not convinced we need to add clutter
> to the documentation for this.

OK. I thought people would not name it in this way. But it indeed happened,
and I don't find we have such rules officially documented anywhere.
Fine if you (or most people) think that's not necessary, though.

[PATCH v1 3/3] bug: use bool for __ret_warn_on in WARN/WARN_ON

[Wei Wang]

coding-style.rst documents below:
bool function return types and stack variables are always fine to use
whenever appropriate. Use of bool is encouraged to improve readability
and is often a better option than 'int' for storing boolean values.

__ret_warn_on is essentially used as boolean in WARN/WARN_ON, so change
its definition from 'int' to 'bool'.

Signed-off-by: Wei Wang <wei.w.wang@intel.com>
---
 include/asm-generic/bug.h | 12 ++++++------
 tools/include/asm/bug.h   | 10 +++++-----
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 4050b191e1a9..3a316be73f0e 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -107,7 +107,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
 		instrumentation_end();					\
 	} while (0)
 #define WARN_ON_ONCE(condition) ({				\
-	int __ret_warn_on = !!(condition);			\
+	bool __ret_warn_on = !!(condition);			\
 	if (unlikely(__ret_warn_on))				\
 		__WARN_FLAGS(BUGFLAG_ONCE |			\
 			     BUGFLAG_TAINT(TAINT_WARN));	\
@@ -119,7 +119,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
 
 #ifndef WARN_ON
 #define WARN_ON(condition) ({						\
-	int __ret_warn_on = !!(condition);				\
+	bool __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on))					\
 		__WARN();						\
 	unlikely(__ret_warn_on);					\
@@ -128,7 +128,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
 
 #ifndef WARN
 #define WARN(condition, format...) ({					\
-	int __ret_warn_on = !!(condition);				\
+	bool __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on))					\
 		__WARN_printf(TAINT_WARN, format);			\
 	unlikely(__ret_warn_on);					\
@@ -136,7 +136,7 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
 #endif
 
 #define WARN_TAINT(condition, taint, format...) ({			\
-	int __ret_warn_on = !!(condition);				\
+	bool __ret_warn_on = !!(condition);				\
 	if (unlikely(__ret_warn_on))					\
 		__WARN_printf(taint, format);				\
 	unlikely(__ret_warn_on);					\
@@ -164,14 +164,14 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
 
 #ifndef HAVE_ARCH_WARN_ON
 #define WARN_ON(condition) ({						\
-	int __ret_warn_on = !!(condition);				\
+	bool __ret_warn_on = !!(condition);				\
 	unlikely(__ret_warn_on);					\
 })
 #endif
 
 #ifndef WARN
 #define WARN(condition, format...) ({					\
-	int __ret_warn_on = !!(condition);				\
+	bool __ret_warn_on = !!(condition);				\
 	no_printk(format);						\
 	unlikely(__ret_warn_on);					\
 })
diff --git a/tools/include/asm/bug.h b/tools/include/asm/bug.h
index 550223f0a6e6..c1f72071303b 100644
--- a/tools/include/asm/bug.h
+++ b/tools/include/asm/bug.h
@@ -8,14 +8,14 @@
 #define __WARN_printf(arg...)	do { fprintf(stderr, arg); } while (0)
 
 #define WARN(condition, format...) ({		\
-	int __ret_warn_on = !!(condition);	\
+	bool __ret_warn_on = !!(condition);	\
 	if (unlikely(__ret_warn_on))		\
 		__WARN_printf(format);		\
 	unlikely(__ret_warn_on);		\
 })
 
 #define WARN_ON(condition) ({					\
-	int __ret_warn_on = !!(condition);			\
+	bool __ret_warn_on = !!(condition);			\
 	if (unlikely(__ret_warn_on))				\
 		__WARN_printf("assertion failed at %s:%d\n",	\
 				__FILE__, __LINE__);		\
@@ -23,8 +23,8 @@
 })
 
 #define WARN_ON_ONCE(condition) ({			\
-	static int __warned;				\
-	int __ret_warn_once = !!(condition);		\
+	static bool __warned;				\
+	bool __ret_warn_once = !!(condition);		\
 							\
 	if (unlikely(__ret_warn_once && !__warned)) {	\
 		__warned = true;			\
@@ -35,7 +35,7 @@
 
 #define WARN_ONCE(condition, format...)	({	\
 	static int __warned;			\
-	int __ret_warn_once = !!(condition);	\
+	bool __ret_warn_once = !!(condition);	\
 						\
 	if (unlikely(__ret_warn_once))		\
 		if (WARN(!__warned, format)) 	\
-- 
2.27.0