patchReplacing the system call pread() with lseek()/xread()/lseek() sequence.

> 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);

> +        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);

> Stefan-W. Hahn wrote:
> > 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.
> > +
> > +        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.

> With the exception of this style difference, the patch looked
> pretty good.  Nice work Stefan.  Andy's right, we do tend to prefer
> "rc = read_in_full" over "rc=read_in_full".  Quite a bit actually,
> though Junio is the final decider on all such matters as he gets
> to choose to accept or reject the patch.  ;-)

>> > +
>> > +        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.
>
> Andy's right actually.  If we are using pread() we aren't relying
> on the current file pointer.  Which means its unnecessary to get
> the current pointer before seeking to the requested offset, and its
> unnecessary to restore it before the git_pread() function returns.

> Andy Whitcroft <apw@shadowen.org> wrote:
> > 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.
> 
> Andy's right actually.  If we are using pread() we aren't relying
> on the current file pointer.  Which means its unnecessary to get
> the current pointer before seeking to the requested offset, and its
> unnecessary to restore it before the git_pread() function returns.

> Stefan-W. Hahn wrote:
>
> > +        if (current_offset != lseek(fd, current_offset, SEEK_SET))
> > +                return -1;
> 
> How likely are we ever to be in the right place here?

> Hi,
> 
> On Tue, 9 Jan 2007, Andy Whitcroft wrote:
> 
> > Stefan-W. Hahn wrote:
> >
> > > +        if (current_offset != lseek(fd, current_offset, SEEK_SET))
> > > +                return -1;
> > 
> > How likely are we ever to be in the right place here?
> 
> You mean something like
> 
> 	if (current_offset != offset + count &&
> 			current_offset != lseek(fd, current_offset, SEEK_SET))
> 		return -1;
> 
> instead? Seems cheap enough.