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.
