diff mbox series

[v3,09/22] reftable/writer: handle allocation failures in `reftable_new_writer()`

Message ID 9edd1d84cdbd53d966ff5cfe9b75281dd5966b07.1727680272.git.ps@pks.im (mailing list archive)
State New
Headers show
Series refatble: handle allocation errors | expand

Commit Message

Patrick Steinhardt Sept. 30, 2024, 8:08 a.m. UTC
Handle allocation failures in `reftable_new_writer()`. Adapt the
function to return an error code to return such failures. While at it,
rename it to match our code style as we have to touch up every callsite
anyway.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 reftable/reftable-writer.h  | 12 +++++++-----
 reftable/stack.c            | 14 ++++++++++----
 reftable/writer.c           | 22 ++++++++++++++++------
 t/unit-tests/lib-reftable.c |  8 +++++---
 4 files changed, 38 insertions(+), 18 deletions(-)

Comments

René Scharfe Sept. 30, 2024, 5:40 p.m. UTC | #1
Am 30.09.24 um 10:08 schrieb Patrick Steinhardt:
> diff --git a/reftable/writer.c b/reftable/writer.c
> index ed61aaf59c..54ec822e1c 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -117,13 +117,17 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)
>  	w->block_writer->restart_interval = w->opts.restart_interval;
>  }
>
> -struct reftable_writer *
> -reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
> -		    int (*flush_func)(void *),
> -		    void *writer_arg, const struct reftable_write_options *_opts)
> +int reftable_writer_new(struct reftable_writer **out,
> +			ssize_t (*writer_func)(void *, const void *, size_t),
> +			int (*flush_func)(void *),
> +			void *writer_arg, const struct reftable_write_options *_opts)
>  {
> -	struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
>  	struct reftable_write_options opts = {0};
> +	struct reftable_writer *wp;
> +
> +	wp = reftable_calloc(1, sizeof(*wp));
> +	if (!wp)
> +		return REFTABLE_OUT_OF_MEMORY_ERROR;
>
>  	if (_opts)
>  		opts = *_opts;
> @@ -134,13 +138,19 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
>  	strbuf_init(&wp->block_writer_data.last_key, 0);
>  	strbuf_init(&wp->last_key, 0);
>  	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
> +	if (!wp->block) {
> +		free(wp);

Better use reftable_free() to free it, since you use reftable_calloc()
to allocate it above.

Perhaps ban free(3), strdup(3) etc. at the end of reftable/basics.h,
banned.h style?

René
Patrick Steinhardt Sept. 30, 2024, 6:22 p.m. UTC | #2
On Mon, Sep 30, 2024 at 07:40:48PM +0200, René Scharfe wrote:
> Am 30.09.24 um 10:08 schrieb Patrick Steinhardt:
> > diff --git a/reftable/writer.c b/reftable/writer.c
> > index ed61aaf59c..54ec822e1c 100644
> > --- a/reftable/writer.c
> > +++ b/reftable/writer.c
> > @@ -117,13 +117,17 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)
> >  	w->block_writer->restart_interval = w->opts.restart_interval;
> >  }
> >
> > -struct reftable_writer *
> > -reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
> > -		    int (*flush_func)(void *),
> > -		    void *writer_arg, const struct reftable_write_options *_opts)
> > +int reftable_writer_new(struct reftable_writer **out,
> > +			ssize_t (*writer_func)(void *, const void *, size_t),
> > +			int (*flush_func)(void *),
> > +			void *writer_arg, const struct reftable_write_options *_opts)
> >  {
> > -	struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
> >  	struct reftable_write_options opts = {0};
> > +	struct reftable_writer *wp;
> > +
> > +	wp = reftable_calloc(1, sizeof(*wp));
> > +	if (!wp)
> > +		return REFTABLE_OUT_OF_MEMORY_ERROR;
> >
> >  	if (_opts)
> >  		opts = *_opts;
> > @@ -134,13 +138,19 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
> >  	strbuf_init(&wp->block_writer_data.last_key, 0);
> >  	strbuf_init(&wp->last_key, 0);
> >  	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
> > +	if (!wp->block) {
> > +		free(wp);
> 
> Better use reftable_free() to free it, since you use reftable_calloc()
> to allocate it above.
> 
> Perhaps ban free(3), strdup(3) etc. at the end of reftable/basics.h,
> banned.h style?

Ugh. I was relying too much on your review having mentioned all cases,
but I really should've double-checked the other patches, too. Mind you,
I really don't mean to blame you here, I blame myself.

In any case, banning these functions via "reftable/basics.h" certainly
seems like a good idea. It's just too easy to screw up by accident. Will
fix tomorrow.

Thanks!

Patrick
Junio C Hamano Sept. 30, 2024, 7:11 p.m. UTC | #3
Patrick Steinhardt <ps@pks.im> writes:

>> Perhaps ban free(3), strdup(3) etc. at the end of reftable/basics.h,
>> banned.h style?
>
> Ugh. I was relying too much on your review having mentioned all cases,
> but I really should've double-checked the other patches, too. Mind you,
> I really don't mean to blame you here, I blame myself.
>
> In any case, banning these functions via "reftable/basics.h" certainly
> seems like a good idea. It's just too easy to screw up by accident. Will
> fix tomorrow.

Sounds like a good plan to do the banned.h-style annotations.

Thanks.
diff mbox series

Patch

diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h
index 189b1f4144..43623dc7c3 100644
--- a/reftable/reftable-writer.h
+++ b/reftable/reftable-writer.h
@@ -90,11 +90,13 @@  struct reftable_stats {
 	int object_id_len;
 };
 
-/* reftable_new_writer creates a new writer */
-struct reftable_writer *
-reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
-		    int (*flush_func)(void *),
-		    void *writer_arg, const struct reftable_write_options *opts);
+struct reftable_writer;
+
+/* Create a new writer. */
+int reftable_writer_new(struct reftable_writer **out,
+			ssize_t (*writer_func)(void *, const void *, size_t),
+			int (*flush_func)(void *),
+			void *writer_arg, const struct reftable_write_options *opts);
 
 /* Set the range of update indices for the records we will add. When writing a
    table into a stack, the min should be at least
diff --git a/reftable/stack.c b/reftable/stack.c
index 498fae846d..ea21ca6e5f 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -808,8 +808,11 @@  int reftable_addition_add(struct reftable_addition *add,
 	}
 	tab_fd = get_tempfile_fd(tab_file);
 
-	wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush, &tab_fd,
-				 &add->stack->opts);
+	err = reftable_writer_new(&wr, reftable_fd_write, reftable_fd_flush,
+				  &tab_fd, &add->stack->opts);
+	if (err < 0)
+		goto done;
+
 	err = write_table(wr, arg);
 	if (err < 0)
 		goto done;
@@ -898,8 +901,11 @@  static int stack_compact_locked(struct reftable_stack *st,
 		goto done;
 	}
 
-	wr = reftable_new_writer(reftable_fd_write, reftable_fd_flush,
-				 &tab_fd, &st->opts);
+	err = reftable_writer_new(&wr, reftable_fd_write, reftable_fd_flush,
+				  &tab_fd, &st->opts);
+	if (err < 0)
+		goto done;
+
 	err = stack_write_compact(st, wr, first, last, config);
 	if (err < 0)
 		goto done;
diff --git a/reftable/writer.c b/reftable/writer.c
index ed61aaf59c..54ec822e1c 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -117,13 +117,17 @@  static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)
 	w->block_writer->restart_interval = w->opts.restart_interval;
 }
 
-struct reftable_writer *
-reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
-		    int (*flush_func)(void *),
-		    void *writer_arg, const struct reftable_write_options *_opts)
+int reftable_writer_new(struct reftable_writer **out,
+			ssize_t (*writer_func)(void *, const void *, size_t),
+			int (*flush_func)(void *),
+			void *writer_arg, const struct reftable_write_options *_opts)
 {
-	struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
 	struct reftable_write_options opts = {0};
+	struct reftable_writer *wp;
+
+	wp = reftable_calloc(1, sizeof(*wp));
+	if (!wp)
+		return REFTABLE_OUT_OF_MEMORY_ERROR;
 
 	if (_opts)
 		opts = *_opts;
@@ -134,13 +138,19 @@  reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
 	strbuf_init(&wp->block_writer_data.last_key, 0);
 	strbuf_init(&wp->last_key, 0);
 	REFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);
+	if (!wp->block) {
+		free(wp);
+		return REFTABLE_OUT_OF_MEMORY_ERROR;
+	}
 	wp->write = writer_func;
 	wp->write_arg = writer_arg;
 	wp->opts = opts;
 	wp->flush = flush_func;
 	writer_reinit_block_writer(wp, BLOCK_TYPE_REF);
 
-	return wp;
+	*out = wp;
+
+	return 0;
 }
 
 void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,
diff --git a/t/unit-tests/lib-reftable.c b/t/unit-tests/lib-reftable.c
index ab1fa44a28..54c26c43e7 100644
--- a/t/unit-tests/lib-reftable.c
+++ b/t/unit-tests/lib-reftable.c
@@ -22,9 +22,11 @@  static int strbuf_writer_flush(void *arg UNUSED)
 struct reftable_writer *t_reftable_strbuf_writer(struct strbuf *buf,
 						 struct reftable_write_options *opts)
 {
-	return reftable_new_writer(&strbuf_writer_write,
-				   &strbuf_writer_flush,
-				   buf, opts);
+	struct reftable_writer *writer;
+	int ret = reftable_writer_new(&writer, &strbuf_writer_write, &strbuf_writer_flush,
+				      buf, opts);
+	check(!ret);
+	return writer;
 }
 
 void t_reftable_write_to_buf(struct strbuf *buf,