On 08/09, Junio C Hamano wrote:
> 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.
Thanks, that looks better. I'll change it for the re-roll.
--
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