Re: [PATCH v7 1/2] Add xpread() and xpwrite()

[Junio C Hamano <gitster@pobox.com>]

Yiannis Marangos <yiannis.marangos@gmail.com> writes:

> xpread() and xpwrite() pay attention to EAGAIN/EINTR, so they will resume
> automatically on interrupted call.

We do not even use pwrite(); please don't add anything unnecessary
and unexercised, like xpwrite(), as potential bugs in it will go
unnoticed long after its introduction until it first gets used.

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index b9f6e12..1bac0f5 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -542,7 +542,7 @@ static void *unpack_data(struct object_entry *obj,
>  
>  	do {
>  		ssize_t n = (len < 64*1024) ? len : 64*1024;
> -		n = pread(pack_fd, inbuf, n, from);
> +		n = xpread(pack_fd, inbuf, n, from);
>  		if (n < 0)
>  			die_errno(_("cannot pread pack file"));
>  		if (!n)

OK.

> diff --git a/compat/mmap.c b/compat/mmap.c
> index c9d46d1..7f662fe 100644
> --- a/compat/mmap.c
> +++ b/compat/mmap.c
> @@ -14,7 +14,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
>  	}
>  
>  	while (n < length) {
> -		ssize_t count = pread(fd, (char *)start + n, length - n, offset + n);
> +		ssize_t count = xpread(fd, (char *)start + n, length - n, offset + n);
>  
>  		if (count == 0) {
>  			memset((char *)start+n, 0, length-n);
> @@ -22,8 +22,6 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of
>  		}
>  
>  		if (count < 0) {
> -			if (errno == EAGAIN || errno == EINTR)
> -				continue;
>  			free(start);
>  			errno = EACCES;
>  			return MAP_FAILED;

OK.

> diff --git a/wrapper.c b/wrapper.c
> index 0cc5636..25b7419 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -174,6 +174,42 @@ ssize_t xwrite(int fd, const void *buf, size_t len)
>  	}
>  }
>  
> +/*
> + * xpread() is the same as pread(), but it automatically restarts pread()
> + * operations with a recoverable error (EAGAIN and EINTR). xpread() DOES
> + * NOT GUARANTEE that "len" bytes is read even if the data is available.
> + */
> +ssize_t xpread(int fd, void *buf, size_t len, off_t offset)
> +{
> +	ssize_t nr;
> +	if (len > MAX_IO_SIZE)
> +	    len = MAX_IO_SIZE;
> +	while (1) {
> +		nr = pread(fd, buf, len, offset);
> +		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
> +			continue;
> +		return nr;
> +	}
> +}

OK.

Thanks.