Re: [PATCH/RFC v3 01/13] Move index v2 specific functions to their own file

> Thomas Gummerer <t.gummerer@gmail.com> 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.