lkml.org 
[lkml]   [2021]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 3/3] bootconfig: Free xbc_data in xbc_destroy_all()
On Wed, 15 Sep 2021 22:19:52 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> @@ -810,6 +811,8 @@ void __init xbc_destroy_all(void)
> * In error cases, @emsg will be updated with an error message and
> * @epos will be updated with the error position which is the byte offset
> * of @buf. If the error is not a parser error, @epos will be -1.
> + * Note that the @buf ownership is transferred, so it will be freed
> + * in xbc_destroy_all().
> */
> int __init xbc_init(char *buf, const char **emsg, int *epos)
> {

I hate this "ownership transfer". Looking at the use case here:

init/main.c:

copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
if (!copy) {
pr_err("Failed to allocate memory for bootconfig\n");
return;
}

memcpy(copy, data, size);
copy[size] = '\0';

ret = xbc_init(copy, &msg, &pos);
if (ret < 0) {

Instead of having xbc_init() return the node count on success, how about
having it allocate the buffer to use and then return it?

That is, move the:

copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);
if (!copy) {
pr_err("Failed to allocate memory for bootconfig\n");
return;
}

memcpy(copy, data, size);
copy[size] = '\0';

into xbc_init(), and have data, and size be passed to it.

Then, have it return the pointer of "copy" or NULL on error?

This will keep the semantics of xbc_* owning the buffer that gets
freed by the destroy.

The xbc_init() could also do the pr_info() that prints the bytes and
node count. There's no other reason to pass that node count to the
caller, is there?

-- Steve

\
 
 \ /
  Last update: 2021-09-15 16:25    [W:1.708 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site