From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 10 Jan 2024 20:54:43 +0100

Another useful pointer was not reassigned to the data structure member
“io_bl” by this function implementation.
Thus omit a redundant call of the function “kfree” at the end.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
A change request by Gabriel Krisman Bertazi was applied here.


 io_uring/io_uring.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 86761ec623f9..c9a63c39cdd0 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -344,7 +344,6 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 err:
 	kfree(ctx->cancel_table.hbs);
 	kfree(ctx->cancel_table_locked.hbs);
-	kfree(ctx->io_bl);
 	xa_destroy(&ctx->io_bl_xa);
 	kfree(ctx);
 	return NULL;
--
2.43.0

Markus Elfring <Markus.Elfring@web.de> writes:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 10 Jan 2024 20:54:43 +0100
>
> Another useful pointer was not reassigned to the data structure member
> “io_bl” by this function implementation.
> Thus omit a redundant call of the function “kfree” at the end.

Perhaps rewrite this to:

ctx->io_bl is initialized later through IORING_OP_PROVIDE_BUFFERS or
IORING_REGISTER_PBUF_RING later on, so there is nothing to free in the
ctx allocation error path.

Other than that, and for this patch only:

Reviewed-by: Gabriel Krisman Bertazi <krisman@suse.de>

thanks,

>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>
> v2:
> A change request by Gabriel Krisman Bertazi was applied here.
>
>
>  io_uring/io_uring.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
> index 86761ec623f9..c9a63c39cdd0 100644
> --- a/io_uring/io_uring.c
> +++ b/io_uring/io_uring.c
> @@ -344,7 +344,6 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>  err:
>  	kfree(ctx->cancel_table.hbs);
>  	kfree(ctx->cancel_table_locked.hbs);
> -	kfree(ctx->io_bl);
>  	xa_destroy(&ctx->io_bl_xa);
>  	kfree(ctx);
>  	return NULL;
> --
> 2.43.0
>

-- 
Gabriel Krisman Bertazi

On 1/12/24 7:25 AM, Gabriel Krisman Bertazi wrote:
> Markus Elfring <Markus.Elfring@web.de> writes:
> 
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Wed, 10 Jan 2024 20:54:43 +0100
>>
>> Another useful pointer was not reassigned to the data structure member
>> ?io_bl? by this function implementation.
>> Thus omit a redundant call of the function ?kfree? at the end.

This is just nonsense...

On top of that, this patch is pointless, and the 2nd patch is even worse
in that it just makes a mess of cleanup. And for what reasoning?
Absolutely none.

There's a reason why I filter emails from this particular author
straight to the trash, there's a long history of this kind of thing and
not understanding feedback.

-- 
Jens Axboe

Jens Axboe <axboe@kernel.dk> writes:

> On 1/12/24 7:25 AM, Gabriel Krisman Bertazi wrote:
>> Markus Elfring <Markus.Elfring@web.de> writes:
>> 
>>> From: Markus Elfring <elfring@users.sourceforge.net>
>>> Date: Wed, 10 Jan 2024 20:54:43 +0100
>>>
>>> Another useful pointer was not reassigned to the data structure member
>>> ?io_bl? by this function implementation.
>>> Thus omit a redundant call of the function ?kfree? at the end.
>
> This is just nonsense...
>
> On top of that, this patch is pointless, and the 2nd patch is even worse
> in that it just makes a mess of cleanup. And for what reasoning?
> Absolutely none.

Ah, The description is non-sense, but the change in this patch seemed
correct to me, even if pointless, which is why I reviewed it.  patch 2
is just garbage.

> There's a reason why I filter emails from this particular author
> straight to the trash, there's a long history of this kind of thing and
> not understanding feedback.

Clearly there is background with this author that I wasn't aware, and
just based on his responses, I can see your point. So I apologize for
giving him space to continue the spamming.  My bad.

-- 
Gabriel Krisman Bertazi

> patch 2 is just garbage.

Such a view can eventually be reconsidered if the support would ever grow
also for the application of additional labels.
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources


> Clearly there is background with this author that I wasn't aware, and
> just based on his responses, I can see your point. So I apologize for
> giving him space to continue the spamming.

The change reluctance can be adapted according to the spectrum of
presented source code adjustment possibilities, can't it?

Regards,
Markus