Thomas Gummerer <t.gummerer@xxxxxxxxx> writes:
> /* remember to discard_cache() before reading a different cache! */
> int read_index_from(struct index_state *istate, const char *path)
> {
> ...
> mmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> - close(fd);
> if (mmap == MAP_FAILED)
> die_errno("unable to map index file");
>
> hdr = mmap;
> - if (verify_hdr(hdr, mmap_size) < 0)
> + if (verify_hdr_version(istate, hdr, mmap_size) < 0)
> goto unmap;
> ...
> + if (istate->ops->verify_hdr(mmap, mmap_size) < 0)
> + goto unmap;
>
> + istate->ops->read_index(istate, mmap, mmap_size, fd);
> ...
> + close(fd);
This looks utterly wrong.
You already have mapped the whole thing, so there is nothing to be
read from fd. You have everything in-core. Leaving fd open and
pass it around looks like it is asking for trouble and confusion.
If you found that an entry you read halfway has an inconsistent crc,
and if you suspect that is because somebody else was writing to the
same index, it is a _sure_ sign that you are not alone, and all the
entries you read so far to the core, even if they weren't touched by
that sombody else when you read them, may be stale, and worse yet,
what you are going to read may be inconsistent with what you've read
and have in-core (e.g. you may have read "f" before somebody else
that is racing with you have turned it into a directory, and your
next read may find "f/d" in the index without crc error).
One sane way to avoid reading such an inconsistent state may be to
redo this whole function, starting from the part that calls mmap().
IOW,
do {
fd = open()
mmap = xmmap(fd);
close(fd);
verify_various_fields(mmap);
status = istate->ops->read_index(istate, mmap, mmap_size));
} while (status == READ_AGAIN);
I do not think the "pass fd around so that we can redo the mapping
deep inside the callchain" is either a good idea or necessary.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html