Hi Rasmus,
On Mon, 3 May 2021 at 23:59, Rasmus Villemoes rasmus.villemoes@prevas.dk wrote:
On 04/05/2021 01.10, Simon Glass wrote:
When passing a data buffer back from a function, it is not always clear who owns the buffer, i.e. who is responsible for freeing the memory used. An example of this is where multiple files are decompressed from the firmware image, using a temporary buffer for reading (since the compressed data has to live somewhere) and producing a temporary or permanent buffer with the resuilts.
Where the firmware image can be memory-mapped, as on x86, the compressed data does not need to be buffered, but the complexity of having a buffer which is either allocated or not, makes the code hard to understand.
Introduce a new 'abuf' which supports simple buffer operations:
- encapsulating a buffer and its size
- either allocated with malloc() or not
- able to be reliably freed if necessary
- able to be converted to an allocated buffer if needed
This simple API makes it easier to deal with allocated and memory-mapped buffers.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Add new abuf_init_set() function
include/abuf.h | 148 ++++++++++++++++++++++ lib/Makefile | 1 + lib/abuf.c | 103 ++++++++++++++++ test/lib/Makefile | 1 + test/lib/abuf.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 556 insertions(+) create mode 100644 include/abuf.h create mode 100644 lib/abuf.c create mode 100644 test/lib/abuf.c
[..]
+/* Test abuf_realloc() */ +static int lib_test_abuf_realloc(struct unit_test_state *uts) +{
struct abuf buf;ulong start;void *ptr;/* TODO: crashes on sandbox */return 0;Eh?
I'll expand the comment. It is unrelated to this patch, buft there is a problem in sandbox with memory allocation.
start = ut_check_free();abuf_init(&buf);/* Allocate an empty buffer */ut_asserteq(true, abuf_realloc(&buf, 0));ut_assertnull(buf.data);ut_asserteq(0, buf.size);ut_asserteq(false, buf.alloced);/* Allocate a non-empty abuf */ut_asserteq(true, abuf_realloc(&buf, TEST_DATA_LEN));ut_assertnonnull(buf.data);ut_asserteq(TEST_DATA_LEN, buf.size);ut_asserteq(true, buf.alloced);ptr = buf.data;/* Make it smaller; the pointer should remain the same */ut_asserteq(true, abuf_realloc(&buf, TEST_DATA_LEN - 1));ut_asserteq(TEST_DATA_LEN - 1, buf.size);ut_asserteq(true, buf.alloced);ut_asserteq_ptr(ptr, buf.data);Perhaps this one? In the alloc'ed case, you always do a realloc() whether or not the new size is larger. realloc() is free to return the same pointer, or actually trim the excess off, or do a malloc()+memcpy()+free() under the hood. realloc() isn't even required to succeed if the new size is smaller than the old. So I don't see how you can assert anything about how realloc() will behave.
If you change the abuf_realloc() to be a no-op (except for updating ->size) for the case of new_size < abuf->size, then yes, you can of course check that abuf behaves that way. But as soon as realloc() is invoked, all bets are off.
Yes that is true, but this matches the behaviour of realloc() in U-Boot. I will add a comment.
/* Make it larger, forcing reallocation */ut_asserteq(true, abuf_realloc(&buf, 0x1000));ut_assert(buf.data != ptr);And this one is similarly flawed - if malloc() originally handed out a buffer that is actually (much) larger than you requested, only the malloc implementation knows, but that can very well mean that a later realloc() does not need to actually do a new allocation - or perhaps, the original allocation wasn't much too big, but it turns out that the following chunk of memory is currently on the free list, so the existing allocation can be expanded in-place.
Same here. Going to the next power of 2 causes a realloc.
ut_asserteq(0x1000, buf.size);ut_asserteq(true, buf.alloced);/* Free it */ut_asserteq(true, abuf_realloc(&buf, 0));ut_assertnull(buf.data);ut_asserteq(0, buf.size);ut_asserteq(false, buf.alloced);/* Check for memory leaks */ut_assertok(ut_check_delta(start));return 0;+} +LIB_TEST(lib_test_abuf_realloc, 0);
+/* Test handling of buffers that are too large */ +static int lib_test_abuf_large(struct unit_test_state *uts) +{
struct abuf buf;ulong start;size_t size;int delta;void *ptr;/** This crashes at present due to trying to allocate more memory than* available, which breaks something on sandbox.*/return 0;start = ut_check_free();/* Try an impossible size */abuf_init(&buf);ut_asserteq(false, abuf_realloc(&buf, CONFIG_SYS_MALLOC_LEN));ut_assertnull(buf.data);ut_asserteq(0, buf.size);ut_asserteq(false, buf.alloced);abuf_uninit(&buf);ut_assertnull(buf.data);ut_asserteq(0, buf.size);ut_asserteq(false, buf.alloced);/* Start with a normal size then try to increase it, to check realloc */ut_asserteq(true, abuf_realloc(&buf, TEST_DATA_LEN));ut_assertnonnull(buf.data);ut_asserteq(TEST_DATA_LEN, buf.size);ut_asserteq(true, buf.alloced);ptr = buf.data;delta = ut_check_delta(start);ut_assert(delta > 0);/* try to increase it */ut_asserteq(false, abuf_realloc(&buf, CONFIG_SYS_MALLOC_LEN));ut_asserteq_ptr(ptr, buf.data);ut_asserteq(TEST_DATA_LEN, buf.size);ut_asserteq(true, buf.alloced);ut_asserteq(delta, ut_check_delta(start));/* Check for memory leaks */abuf_uninit(&buf);ut_assertok(ut_check_delta(start));/* Start with a huge unallocated buf and try to move it */abuf_init(&buf);abuf_map_sysmem(&buf, 0, CONFIG_SYS_MALLOC_LEN);ut_asserteq(CONFIG_SYS_MALLOC_LEN, buf.size);ut_asserteq(false, buf.alloced);ut_assertnull(abuf_uninit_move(&buf, &size));/* Check for memory leaks */abuf_uninit(&buf);ut_assertok(ut_check_delta(start));return 0;+} +LIB_TEST(lib_test_abuf_large, 0);
+/* Test abuf_uninit_move() */ +static int lib_test_abuf_uninit_move(struct unit_test_state *uts) +{
void *ptr, *orig_ptr;struct abuf buf;size_t size;ulong start;int delta;start = ut_check_free();/* Move an empty buffer */abuf_init(&buf);ut_assertnull(abuf_uninit_move(&buf, &size));ut_asserteq(0, size);ut_assertnull(abuf_uninit_move(&buf, NULL));/* Move an unallocated buffer */abuf_set(&buf, test_data, TEST_DATA_LEN);Reading this made me think of something that I think is missing from the API: Perhaps I have a chunk of memory that I have malloc'ed myself, and now I want to use an abuf to manage it. How do I gift that to abuf? IOW, perhaps an abuf_set_alloced(), or if it's better to think of it as a companion to abuf_uninit_move, perhaps abuf_init_move()?
OK, thanks, good idea.. Will add.
Regards, Simon