Re: [PATCH v3 13/16] pack-revindex: read multi-pack reverse indexes

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 29, 2021, 21:27 UTC
Message-ID: <YGJGXJ7Md0tWtnME@nand.local>
In-Reply-To: <YGHLe0nmh7R8hsVo@coredump.intra.peff.net>

On Mon, Mar 29, 2021 at 08:43:39AM -0400, Jeff King wrote:

Show 26 quoted lines

> On Thu, Mar 11, 2021 at 12:05:29PM -0500, Taylor Blau wrote:
>
> > Implement reading for multi-pack reverse indexes, as described in the
> > previous patch.
>
> Looks good overall. I found a few tiny nits below.
>
> > +int load_midx_revindex(struct multi_pack_index *m)
> > +{
> > +	char *revindex_name;
> > +	int ret;
> > +	if (m->revindex_data)
> > +		return 0;
> > +
> > +	revindex_name = get_midx_rev_filename(m);
> > +
> > +	ret = load_revindex_from_disk(revindex_name,
> > +				      m->num_objects,
> > +				      &m->revindex_map,
> > +				      &m->revindex_len);
> > +	if (ret)
> > +		goto cleanup;
>
> On error, I wondered if m->revindex_map, etc, would be modified. But it
> looks like no, load_revindex_from_disk() is careful not to touch them
> unless it sees a valid revindex. Good.

Yep, that was intentional. Thanks.

Show 19 quoted lines

> > +int close_midx_revindex(struct multi_pack_index *m)
> > +{
> > +	if (!m)
> > +		return 0;
> > +
> > +	if (munmap((void*)m->revindex_map, m->revindex_len))
> > +		return -1;
> > +
> > +	m->revindex_map = NULL;
> > +	m->revindex_data = NULL;
> > +	m->revindex_len = 0;
> > +
> > +	return 0;
> > +}
>
> It's hard to imagine why munmap() would fail. But if it does, we should
> probably clear the struct fields anyway. I note that the matching code
> for a "struct packed_git" does not bother even checking the return value
> of munmap. Perhaps we should just do the same here.

I tend to agree that we should match the behavior of "packfile.c:close_pack_revindex()" and just not check the return value of munmap. Either the call to munmap() worked, and we shouldn't be reading revindex_map anymore, or it didn't, and something else is probably wrong enough with the original mmap call that we probably also shouldn't be reading it.

> The packed_git version also returned early if revindex_map is NULL. Here
> the burden is placed on the caller (it's hard to tell if that matters
> since there aren't any callers yet, but it probably makes sense to push
> the check down into this function).

Yeah, I think that that function actually is doing the worst of both worlds (which is to check p->revindex_map, but not p itself).

I modified the MIDX version to check both m and m->revindex_map (but I agree it's hard to tell with the caller coming in a later series).

Show 18 quoted lines

>
> > +uint32_t pack_pos_to_midx(struct multi_pack_index *m, uint32_t pos)
> > +{
> > +	if (!m->revindex_data)
> > +		BUG("pack_pos_to_midx: reverse index not yet loaded");
> > +	if (m->num_objects <= pos)
> > +		BUG("pack_pos_to_midx: out-of-bounds object at %"PRIu32, pos);
> > +	return get_be32((const char *)m->revindex_data + (pos * sizeof(uint32_t)));
> > +}
>
> OK, this one is just a direct read of the .rev data, like
> pack_pos_to_index() is. I think the final line can be simplified to:
>
>   return get_be32(m->revindex_data + pos);
>
> just like pack_pos_to_index(). (I suspect this is a leftover from the
> earlier version of your .rev series where the pointer was still a "void
> *").

Yes, definitely.

> Probably sizeof(*m->revindex_data) would be slightly nicer in the
> bsearch call (again, I suspect a holdover from when that was a void
> pointer).

Yes, exactly.

Thanks, Taylor