[PATCH 01/49] Add support for an owned buffer

Simon Glass
2021-05-05T23:37:07Z

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