From: Karthik Nayak Date: Wed, 19 Nov 2025 10:11:40 GMT Subject: Re: [PATCH 05/18] streaming: allocate stream inside the backend-specific logic Message-ID: In-Reply-To: <20251119-b4-pks-odb-read-stream-v1-5-adacf03c2ccf@pks.im> Patrick Steinhardt writes: > When creating a new stream we first allocate it and then call into > backend-specific logic to populate the stream. This design requires that > the stream itself contains a `union` with backend-specific members that > then ultimately get populated by the backend-specific logic. > > This works, but it's awkward in the context of pluggable object > databases. Each backend will need its own member in that union, and as > the structure itself is completely opaque (it's only defined in > "streamgin.c") it also has the consequence that we must have the logic s/streamgin/streaming > that is specific to backends in "streaming.c". > > Ideally though, the infrastructure would be reversed: we have a generic > `struct odb_read_stream` and some helper functions in "streaming.c", > whereas the backend-specific logic sits in the backend's subsystem > itself. > Will this also mean that we move the backend specific functions like `open_istream_loose()` away from 'streaming.c'? Let's read on. > This can be realized by using a design that is similar to how we handle > reference databases: instead of having a union of members, we instead > have backend-specific structures with a `struct odb_read_stream base` > as its first member. The backends would thus hand out the pointer to the > base, but internally they know to cast back to the backend-specific > type. > Right. > This means though that we need to allocate different structures > depending on the backend. To prepare for this, move allocation of the > structure into the backend-specific functions that open a new stream. > Subsequent commits will then create those new backend-specific structs. > Who's in charge of free'ing these structs? I see that `close_istream()` calls the assigned `close()` function. So this could be handled on the backend level. But it also does `free(st)`. > Signed-off-by: Patrick Steinhardt > --- > streaming.c | 99 +++++++++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 63 insertions(+), 36 deletions(-) > > diff --git a/streaming.c b/streaming.c > index d7db446d25..b8ce82483f 100644 > --- a/streaming.c > +++ b/streaming.c > @@ -222,27 +222,34 @@ static int close_istream_loose(struct odb_read_stream *st) > return 0; > } > > -static int open_istream_loose(struct odb_read_stream *st, struct repository *r, > +static int open_istream_loose(struct odb_read_stream **out, > + struct repository *r, We take in a double pointer now, since the allocation will be handled inside the function. > const struct object_id *oid) > { > struct object_info oi = OBJECT_INFO_INIT; > + struct odb_read_stream *st; > struct odb_source *source; > - > - oi.sizep = &st->size; > - oi.typep = &st->type; > + unsigned long mapsize; > + void *mapped; > > odb_prepare_alternates(r->objects); > for (source = r->objects->sources; source; source = source->next) { > - st->u.loose.mapped = odb_source_loose_map_object(source, oid, > - &st->u.loose.mapsize); > - if (st->u.loose.mapped) > + mapped = odb_source_loose_map_object(source, oid, &mapsize); > + if (mapped) > break; > } > - if (!st->u.loose.mapped) > + if (!mapped) > return -1; > > - switch (unpack_loose_header(&st->z, st->u.loose.mapped, > - st->u.loose.mapsize, st->u.loose.hdr, > + /* > + * Note: we must allocate this structure early even though we may still > + * fail. This is because we need to initialize the zlib stream, and it > + * is not possible to copy the stream around after the fact because it > + * has self-referencing pointers. > + */ > + CALLOC_ARRAY(st, 1); > + > + switch (unpack_loose_header(&st->z, mapped, mapsize, st->u.loose.hdr, > sizeof(st->u.loose.hdr))) { > case ULHR_OK: > break; > @@ -250,19 +257,28 @@ static int open_istream_loose(struct odb_read_stream *st, struct repository *r, > case ULHR_TOO_LONG: > goto error; > } > + > + oi.sizep = &st->size; > + oi.typep = &st->type; > + > if (parse_loose_header(st->u.loose.hdr, &oi) < 0 || st->type < 0) > goto error; > > + st->u.loose.mapped = mapped; > + st->u.loose.mapsize = mapsize; > st->u.loose.hdr_used = strlen(st->u.loose.hdr) + 1; > st->u.loose.hdr_avail = st->z.total_out; > st->z_state = z_used; > st->close = close_istream_loose; > st->read = read_istream_loose; > > + *out = st; > + > return 0; > error: > git_inflate_end(&st->z); > munmap(st->u.loose.mapped, st->u.loose.mapsize); > + free(st); > return -1; > } > > @@ -338,12 +354,16 @@ static int close_istream_pack_non_delta(struct odb_read_stream *st) > return 0; > } > > -static int open_istream_pack_non_delta(struct odb_read_stream *st, > +static int open_istream_pack_non_delta(struct odb_read_stream **out, > struct repository *r UNUSED, > const struct object_id *oid UNUSED, > struct packed_git *pack, > off_t offset) > { > + struct odb_read_stream stream = { > + .close = close_istream_pack_non_delta, > + .read = read_istream_pack_non_delta, > + }; So this is now statically defined. Won't this cause an issue? The rest looks good. Thanks