[PATCH 1/2] keys: Remove unnecessary local variable from ca_keys_setup

[PATCH 1/2] keys: Remove unnecessary local variable from ca_keys_setup

[Thorsten Blum]

The variable 'ret', whose name implies a return variable, is only used
to temporarily store the result of __asymmetric_key_hex_to_key_id().
Use the result directly and remove the local variable.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 crypto/asymmetric_keys/restrict.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index afcd4d101ac5..57ee2021fef7 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -29,15 +29,13 @@ static int __init ca_keys_setup(char *str)
 	if (strncmp(str, "id:", 3) == 0) {
 		struct asymmetric_key_id *p = &cakey.id;
 		size_t hexlen = (strlen(str) - 3) / 2;
-		int ret;
 
 		if (hexlen == 0 || hexlen > sizeof(cakey.data)) {
 			pr_err("Missing or invalid ca_keys id\n");
 			return 1;
 		}
 
-		ret = __asymmetric_key_hex_to_key_id(str + 3, p, hexlen);
-		if (ret < 0)
+		if (__asymmetric_key_hex_to_key_id(str + 3, p, hexlen) < 0)
 			pr_err("Unparsable ca_keys id hex string\n");
 		else
 			ca_keyid = p;	/* owner key 'id:xxxxxx' */
-- 
2.51.0

[PATCH 2/2] keys: Remove unnecessary local variable from proc_keys_show

[Thorsten Blum]

The local variable 'rc' is only used to temporary store the result of
calling key_task_permission(). Use the result directly and remove the
local variable.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 security/keys/proc.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/security/keys/proc.c b/security/keys/proc.c
index 4f4e2c1824f1..39af57af2aad 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -160,7 +160,6 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	char xbuf[16];
 	short state;
 	u64 timo;
-	int rc;
 
 	struct keyring_search_context ctx = {
 		.index_key		= key->index_key,
@@ -188,8 +187,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
 	}
 
 	/* check whether the current task is allowed to view the key */
-	rc = key_task_permission(key_ref, ctx.cred, KEY_NEED_VIEW);
-	if (rc < 0)
+	if (key_task_permission(key_ref, ctx.cred, KEY_NEED_VIEW) < 0)
 		return 0;
 
 	now = ktime_get_real_seconds();
-- 
2.51.0

Re: [PATCH 1/2] keys: Remove unnecessary local variable from ca_keys_setup

[Lukas Wunner]

On Thu, Oct 23, 2025 at 04:32:31PM +0200, Thorsten Blum wrote:
> The variable 'ret', whose name implies a return variable, is only used
> to temporarily store the result of __asymmetric_key_hex_to_key_id().
> Use the result directly and remove the local variable.
[...]
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -29,15 +29,13 @@ static int __init ca_keys_setup(char *str)
>  	if (strncmp(str, "id:", 3) == 0) {
>  		struct asymmetric_key_id *p = &cakey.id;
>  		size_t hexlen = (strlen(str) - 3) / 2;
> -		int ret;
>  
>  		if (hexlen == 0 || hexlen > sizeof(cakey.data)) {
>  			pr_err("Missing or invalid ca_keys id\n");
>  			return 1;
>  		}
>  
> -		ret = __asymmetric_key_hex_to_key_id(str + 3, p, hexlen);
> -		if (ret < 0)
> +		if (__asymmetric_key_hex_to_key_id(str + 3, p, hexlen) < 0)
>  			pr_err("Unparsable ca_keys id hex string\n");
>  		else
>  			ca_keyid = p;	/* owner key 'id:xxxxxx' */

Quite honestly I don't think this change constitutes a worthwhile
improvement.

Those "if (ret)" checks are everywhere in the kernel, it's a pattern
that developers have grown accustomed to and immediately understand
when reading the code.  If it takes an extra variable declaration,
so be it.

For people (like me) who frequently have to dig in the git history
with recursive "git blame", changes like this one make life harder
because it introduces an extra step when trying to understand from
which commit a particular line of code originated.

And so changes like this one which are merely motivated by personal
stylistic preferences become a net negative.

Thanks,

Lukas

Re: [PATCH 2/2] keys: Remove unnecessary local variable from proc_keys_show

[Jarkko Sakkinen]

On Thu, Oct 23, 2025 at 04:32:33PM +0200, Thorsten Blum wrote:
> The local variable 'rc' is only used to temporary store the result of
> calling key_task_permission(). Use the result directly and remove the
> local variable.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  security/keys/proc.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/security/keys/proc.c b/security/keys/proc.c
> index 4f4e2c1824f1..39af57af2aad 100644
> --- a/security/keys/proc.c
> +++ b/security/keys/proc.c
> @@ -160,7 +160,6 @@ static int proc_keys_show(struct seq_file *m, void *v)
>  	char xbuf[16];
>  	short state;
>  	u64 timo;
> -	int rc;
>  
>  	struct keyring_search_context ctx = {
>  		.index_key		= key->index_key,
> @@ -188,8 +187,7 @@ static int proc_keys_show(struct seq_file *m, void *v)
>  	}
>  
>  	/* check whether the current task is allowed to view the key */
> -	rc = key_task_permission(key_ref, ctx.cred, KEY_NEED_VIEW);
> -	if (rc < 0)
> +	if (key_task_permission(key_ref, ctx.cred, KEY_NEED_VIEW) < 0)
>  		return 0;

I absolutely dislike combining return value check to the call.

The old version documents that we ignore the return value, which
is convoluted in the new version.

>  
>  	now = ktime_get_real_seconds();
> -- 
> 2.51.0
> 

BR, Jarkko