Re: [PATCH 7/7] Replace mmap with xmmap, better handling MAP_FAILED.

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Dec 24, 2006, 13:22 UTC
Message-ID: <Pine.LNX.4.63.0612241410400.19693@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <20061224054723.GG8146@spearce.org>

Hi,

On Sun, 24 Dec 2006, Shawn O. Pearce wrote:

Show 13 quoted lines

> diff --git a/diff.c b/diff.c
> index f14288b..244292a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1341,10 +1341,8 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
>  		fd = open(s->path, O_RDONLY);
>  		if (fd < 0)
>  			goto err_empty;
> -		s->data = mmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
> +		s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
>  		close(fd);
> -		if (s->data == MAP_FAILED)
> -			goto err_empty;

The only gripe I have here is that the old code could actually say where the problem occurred ("cannot read data blob for <blabla>"), but that probably does not matter so much, now that we can hardly run out of memory on a decent machine, even using big packfiles.

Show 13 quoted lines

> diff --git a/read-cache.c b/read-cache.c
> index b8d83cc..ca3efbb 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -798,7 +798,7 @@ int read_cache_from(const char *path)
>  		cache_mmap_size = st.st_size;
>  		errno = EINVAL;
>  		if (cache_mmap_size >= sizeof(struct cache_header) + 20)
> -			cache_mmap = mmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
> +			cache_mmap = xmmap(NULL, cache_mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0);
>  	}
>  	close(fd);
>  	if (cache_mmap == MAP_FAILED)

This MAP_FAILED no longer has anything to do with MAP_FAILED, but rather with fstat failed, so you probably want to move that into an else construct just before "close(fd);".

All in all it is a good change -- for the builtin programs.

But it is less good for the libification. Maybe it is time for a discussion about the possible strategies to avoid dying in libgit.a?

Ciao, Dscho