On Sat, Sep 21, 2024 at 11:26:09PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > -int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest,
> > +int indexed_table_ref_iter_new(struct indexed_table_ref_iter **dest,
> > struct reftable_reader *r, uint8_t *oid,
> > int oid_len, uint64_t *offsets, int offset_len)
> > {
> > struct indexed_table_ref_iter empty = INDEXED_TABLE_REF_ITER_INIT;
> > - struct indexed_table_ref_iter *itr = reftable_calloc(1, sizeof(*itr));
> > + struct indexed_table_ref_iter *itr;
> > int err = 0;
> >
> > + itr = reftable_calloc(1, sizeof(*itr));
> > + if (!itr) {
> > + err = REFTABLE_OUT_OF_MEMORY_ERROR;
> > + goto out;
> > + }
> > +
> > *itr = empty;
> > itr->r = r;
> > strbuf_add(&itr->oid, oid, oid_len);
> > @@ -197,11 +203,15 @@ int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest,
> > itr->offset_len = offset_len;
> >
> > err = indexed_table_ref_iter_next_block(itr);
> > - if (err < 0) {
> > + if (err < 0)
> > + goto out;
> > +
> > + *dest = itr;
> > + err = 0;
> > +
> > +out:
> > + if (err < 0)
> > reftable_free(itr);
> > - } else {
> > - *dest = itr;
> > - }
> > return err;
> > }
>
> Unless the service the helper function offers is to upgrade an
> existing resource (e.g., realloc() taking a pointer and give an
> enlarged piece of memory), it may be a safer calling convention to
> promise that *dest is cleared to NULL when the function fails,
> instead of promising that *dest is left intact. The caller, when it
> needs to evantually release the resource acquired here, has to
> remember what the returned value (i.e., err) was, in order to decide
> if it needs to call the release helper on *dest it obtained from us.
>
> The only caller seems to initialize *dest to NULL itself, so it does
> not matter in the current code, though.
I don't really see it as much of a problem here, mostly because this
function is internal to the reftable library anyway. Also, callers
essentially have to NULL-initialize the variable anyway once there are
multiple error paths and if they want to free it, because otherwise they
could end up freeing a uninitialized pointer.
On the other hand it doesn't hurt much to assign `NULL` on the error
path either, so I'll just do that.
Patrick
