Re: [RFC PATCH 1/2] reftable: remove the "return_block" abstraction

["René Scharfe" <l.s.r@web.de>]

Am 15.04.22 um 12:21 schrieb Ævar Arnfjörð Bjarmason:
> This abstraction added in 1214aa841bc (reftable: add blocksource, an
> abstraction for random access reads, 2021-10-07) has the caller
> provide a "blockp->data", so there's not point in having the vtable
> have a custom free() function.
>
> In addition this had what looked like a poor man's SANITIZE=address
> doing a memset() to 0xff just before the data was free'd.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  reftable/block.c                |  4 +---
>  reftable/blocksource.c          | 28 +---------------------------
>  reftable/reftable-blocksource.h |  2 --
>  3 files changed, 2 insertions(+), 32 deletions(-)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index 34d4d073692..bb17cc32372 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -442,9 +442,7 @@ void block_writer_release(struct block_writer *bw)
>
>  void reftable_block_done(struct reftable_block *blockp)
>  {
> -	struct reftable_block_source source = blockp->source;
> -	if (blockp && source.ops)
> -		source.ops->return_block(source.arg, blockp);

I don't understand this interface.  Why is source.arg passed to the
function?  Is this perhaps done in preparation for some kind of backend
that is planned to be added later which makes use of it?  I suspect we'd
better postpone cleanups until the full functionality is integrated.

> +	FREE_AND_NULL(blockp->data);

If you do that...

>  	blockp->data = NULL;

... then this line becomes redundant.

>  	blockp->len = 0;
>  	blockp->source.ops = NULL;
> diff --git a/reftable/blocksource.c b/reftable/blocksource.c
> index 2605371c28d..d9e47cc316b 100644
> --- a/reftable/blocksource.c
> +++ b/reftable/blocksource.c
> @@ -13,12 +13,6 @@ license that can be found in the LICENSE file or at
>  #include "reftable-blocksource.h"
>  #include "reftable-error.h"
>
> -static void strbuf_return_block(void *b, struct reftable_block *dest)
> -{
> -	memset(dest->data, 0xff, dest->len);
> -	reftable_free(dest->data);
> -}
> -
>  static void strbuf_close(void *b)
>  {
>  }
> @@ -42,7 +36,6 @@ static uint64_t strbuf_size(void *b)
>  static struct reftable_block_source_vtable strbuf_vtable = {
>  	.size = &strbuf_size,
>  	.read_block = &strbuf_read_block,
> -	.return_block = &strbuf_return_block,
>  	.close = &strbuf_close,
>  };
>
> @@ -54,19 +47,7 @@ void block_source_from_strbuf(struct reftable_block_source *bs,
>  	bs->arg = buf;
>  }
>
> -static void malloc_return_block(void *b, struct reftable_block *dest)
> -{
> -	memset(dest->data, 0xff, dest->len);
> -	reftable_free(dest->data);
> -}
> -
> -static struct reftable_block_source_vtable malloc_vtable = {
> -	.return_block = &malloc_return_block,
> -};
> -
> -static struct reftable_block_source malloc_block_source_instance = {
> -	.ops = &malloc_vtable,
> -};
> +static struct reftable_block_source malloc_block_source_instance = { 0 };
>
>  struct reftable_block_source malloc_block_source(void)
>  {
> @@ -83,12 +64,6 @@ static uint64_t file_size(void *b)
>  	return ((struct file_block_source *)b)->size;
>  }
>
> -static void file_return_block(void *b, struct reftable_block *dest)
> -{
> -	memset(dest->data, 0xff, dest->len);
> -	reftable_free(dest->data);
> -}
> -
>  static void file_close(void *b)
>  {
>  	int fd = ((struct file_block_source *)b)->fd;
> @@ -115,7 +90,6 @@ static int file_read_block(void *v, struct reftable_block *dest, uint64_t off,
>  static struct reftable_block_source_vtable file_vtable = {
>  	.size = &file_size,
>  	.read_block = &file_read_block,
> -	.return_block = &file_return_block,
>  	.close = &file_close,
>  };
>
> diff --git a/reftable/reftable-blocksource.h b/reftable/reftable-blocksource.h
> index 5aa3990a573..7b7cb280b73 100644
> --- a/reftable/reftable-blocksource.h
> +++ b/reftable/reftable-blocksource.h
> @@ -35,8 +35,6 @@ struct reftable_block_source_vtable {
>  	   beyond the end of the block */
>  	int (*read_block)(void *source, struct reftable_block *dest,
>  			  uint64_t off, uint32_t size);
> -	/* mark the block as read; may return the data back to malloc */
> -	void (*return_block)(void *source, struct reftable_block *blockp);
>
>  	/* release all resources associated with the block source */
>  	void (*close)(void *source);