[PATCH] smb: client: Move an error code assignment in smb3_init_transform_rq()

[PATCH] smb: client: Move an error code assignment in smb3_init_transform_rq()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 8 Oct 2025 18:48:28 +0200

Convert an initialisation for the variable “rc” into an error code
assignment at the end of this function implementation.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/smb/client/smb2ops.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 7c3e96260fd4..2513270ac596 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -4596,7 +4596,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
 {
 	struct smb2_transform_hdr *tr_hdr = new_rq[0].rq_iov[0].iov_base;
 	unsigned int orig_len = 0;
-	int rc = -ENOMEM;
+	int rc;
 
 	for (int i = 1; i < num_rqst; i++) {
 		struct smb_rqst *old = &old_rq[i - 1];
@@ -4611,7 +4611,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
 		if (size > 0) {
 			buffer = cifs_alloc_folioq_buffer(size);
 			if (!buffer)
-				goto err_free;
+				goto e_nomem;
 
 			new->rq_buffer = buffer;
 			iov_iter_folio_queue(&new->rq_iter, ITER_SOURCE,
@@ -4634,6 +4634,8 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
 
 	return rc;
 
+e_nomem:
+	rc = -ENOMEM;
 err_free:
 	smb3_free_compound_rqst(num_rqst - 1, &new_rq[1]);
 	return rc;
-- 
2.51.0

Re: [PATCH] smb: client: Move an error code assignment in smb3_init_transform_rq()

[Henrique Carvalho]

Hi Markus,

On 10/8/25 2:04 PM, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 8 Oct 2025 18:48:28 +0200
> 
> Convert an initialisation for the variable “rc” into an error code
> assignment at the end of this function implementation.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/smb/client/smb2ops.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index 7c3e96260fd4..2513270ac596 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -4596,7 +4596,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
>  {
>  	struct smb2_transform_hdr *tr_hdr = new_rq[0].rq_iov[0].iov_base;
>  	unsigned int orig_len = 0;
> -	int rc = -ENOMEM;
> +	int rc;
>  
>  	for (int i = 1; i < num_rqst; i++) {
>  		struct smb_rqst *old = &old_rq[i - 1];
> @@ -4611,7 +4611,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
>  		if (size > 0) {
>  			buffer = cifs_alloc_folioq_buffer(size);
>  			if (!buffer)
> -				goto err_free;
> +				goto e_nomem;
>  
>  			new->rq_buffer = buffer;
>  			iov_iter_folio_queue(&new->rq_iter, ITER_SOURCE,
> @@ -4634,6 +4634,8 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
>  
>  	return rc;
>  
> +e_nomem:
> +	rc = -ENOMEM;
>  err_free:
>  	smb3_free_compound_rqst(num_rqst - 1, &new_rq[1]);
>  	return rc;

I don't think this change improves readability.

I understand that making the assignment explicit is good, but why not
simply set rc to -ENOMEM if !buffer and then goto err_free?

Also, I think its a bit confusing having inconsistent naming styles `e_`
`err_`...

-- 
Henrique
SUSE Labs

Re: [PATCH] smb: client: Move an error code assignment in smb3_init_transform_rq()

[Markus Elfring]

>> Convert an initialisation for the variable “rc” into an error code
>> assignment at the end of this function implementation.
…>> +++ b/fs/smb/client/smb2ops.c
…>> @@ -4611,7 +4611,7 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
>>  		if (size > 0) {
>>  			buffer = cifs_alloc_folioq_buffer(size);
>>  			if (!buffer)
>> -				goto err_free;
>> +				goto e_nomem;
>>  
>>  			new->rq_buffer = buffer;
>>  			iov_iter_folio_queue(&new->rq_iter, ITER_SOURCE,
>> @@ -4634,6 +4634,8 @@ smb3_init_transform_rq(struct TCP_Server_Info *server, int num_rqst,
>>  
>>  	return rc;
>>  
>> +e_nomem:
>> +	rc = -ENOMEM;
>>  err_free:
>>  	smb3_free_compound_rqst(num_rqst - 1, &new_rq[1]);
>>  	return rc;
> 
> I don't think this change improves readability.
> 
> I understand that making the assignment explicit is good,

Thanks for this constructive feedback.


> but why not simply set rc to -ENOMEM if !buffer and then goto err_free?

I proposed to adjust the affected if branch in this way
because there is no need to add curly brackets then.


> Also, I think its a bit confusing having inconsistent naming styles `e_`
> `err_`...

Which naming approach would you find more helpful for the marking
of a variable assignment instead of a subsequent function call?

Regards,
Markus

Re: [PATCH] smb: client: Move an error code assignment in smb3_init_transform_rq()

[Markus Elfring]

> Also when a patch doesn't shrink code, but even grows it by one line, and doesn't make it much clearer, it is probably not worth changing (it complicates future backports of real fixes in the future eg). Let's limit these to those that shrink code and make code clearer (or ideally fix potential bugs)

Do you insist to keep an extra initialisation for the local variable “rc”
despite of the implementation detail that the value “-ENOMEM” is needed
only for a single if branch here?

Regards,
Markus