Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.

[Andy Whitcroft <apw@shadowen.org>]

Stefan-W. Hahn wrote:
> From: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
> 
> Using cygwin with cygwin.dll before 1.5.22 the system call pread() is buggy.
> This patch introduces NO_PREAD. If NO_PREAD is set git uses a sequence of
> lseek()/xread()/lseek() to emulate pread.
> 
> Signed-off-by: Stefan-W. Hahn <stefan.hahn@s-hahn.de>
> ---
>  Makefile          |    7 +++++++
>  compat/pread.c    |   18 ++++++++++++++++++
>  git-compat-util.h |    5 +++++
>  3 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 6c12bc6..43113e9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -69,6 +69,9 @@ all:
>  #
>  # Define NO_MMAP if you want to avoid mmap.
>  #
> +# Define NO_PREAD if you have a problem with pread() system call (e.g.
> +# cygwin.dll before v1.5.22).
> +#
>  # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
>  # generally faster on your platform than accessing the working directory.
>  #
> @@ -523,6 +526,10 @@ ifdef NO_MMAP
>  	COMPAT_CFLAGS += -DNO_MMAP
>  	COMPAT_OBJS += compat/mmap.o
>  endif
> +ifdef NO_PREAD
> +	COMPAT_CFLAGS += -DNO_PREAD
> +	COMPAT_OBJS += compat/pread.o
> +endif
>  ifdef NO_FAST_WORKING_DIRECTORY
>  	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
>  endif
> diff --git a/compat/pread.c b/compat/pread.c
> new file mode 100644
> index 0000000..9183c05
> --- /dev/null
> +++ b/compat/pread.c
> @@ -0,0 +1,18 @@
> +#include "../git-compat-util.h"
> +
> +ssize_t git_pread(int fd, void *buf, size_t count, off_t offset)
> +{
> +        off_t current_offset;
> +        ssize_t rc;
> +
> +        current_offset = lseek(fd, 0, SEEK_CUR);
> +
> +        if (lseek(fd, offset, SEEK_SET) < 0)
> +                return -1;
> +
> +        rc=read_in_full(fd, buf, count);

Seems to be style inconsistancy between current_offset = and rc= I
believe the former is preferred.

> +
> +        if (current_offset != lseek(fd, current_offset, SEEK_SET))
> +                return -1;

How likely are we ever to be in the right place here?  Seems vanishingly
small putting us firmly in the four syscalls per call space.  I wonder
if git ever actually cares about the seek location.  ie if we could stop
reading and resetting it.  Probabally not worth working it out I guess
as any _sane_ system has one.

> +        return rc;
> +}
> diff --git a/git-compat-util.h b/git-compat-util.h
> index e023bf1..f8d46d5 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -107,6 +107,11 @@ extern int git_munmap(void *start, size_t length);
>  #define DEFAULT_PACKED_GIT_LIMIT \
>  	((1024L * 1024L) * (sizeof(void*) >= 8 ? 8192 : 256))
>  
> +#ifdef NO_PREAD
> +#define pread git_pread
> +extern ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
> +#endif
> +
>  #ifdef NO_SETENV
>  #define setenv gitsetenv
>  extern int gitsetenv(const char *, const char *, int);

-apw