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

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

[Stefan-W. Hahn]

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    |   15 +++++++++++++++
 git-compat-util.h |    5 +++++
 3 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 6c12bc6..47af0de 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 (i.e.
+# 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..cd1da87
--- /dev/null
+++ b/compat/pread.c
@@ -0,0 +1,15 @@
+#include "../git-compat-util.h"
+
+ssize_t git_pread(int fd,void *buf,size_t count,off_t offset)
+{
+        off_t current_offset = lseek(fd, 0, SEEK_CUR);
+
+        if (lseek(fd, offset, SEEK_SET) < 0)
+                return EINVAL;
+
+        ssize_t rc=xread(fd, buf, count);
+
+        if (current_offset != lseek(fd, current_offset, SEEK_SET))
+                return EINVAL;
+        return rc;
+}
diff --git a/git-compat-util.h b/git-compat-util.h
index e023bf1..00b14df 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);
-- 
1.4.4.4.g46aa

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

[Nicolas Pitre]

On Tue, 9 Jan 2007, Stefan-W. Hahn wrote:

> --- /dev/null
> +++ b/compat/pread.c
> @@ -0,0 +1,15 @@
> +#include "../git-compat-util.h"
> +
> +ssize_t git_pread(int fd,void *buf,size_t count,off_t offset)
> +{
> +        off_t current_offset = lseek(fd, 0, SEEK_CUR);
> +
> +        if (lseek(fd, offset, SEEK_SET) < 0)
> +                return EINVAL;

You certainly wanted to return -1 here.

> +
> +        ssize_t rc=xread(fd, buf, count);
> +
> +        if (current_offset != lseek(fd, current_offset, SEEK_SET))
> +                return EINVAL;

Ditto here.

> +        return rc;
> +}


Nicolas

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

[Junio C Hamano]

"Stefan-W. Hahn" <stefan.hahn@s-hahn.de> writes:

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

Thanks.

> +# Define NO_PREAD if you have a problem with pread() system call (i.e.
> +# cygwin.dll before v1.5.22).
> +#

I would have preferred e.g. not i.e..

> diff --git a/compat/pread.c b/compat/pread.c
> new file mode 100644
> index 0000000..cd1da87
> --- /dev/null
> +++ b/compat/pread.c
> @@ -0,0 +1,15 @@
> +#include "../git-compat-util.h"
> +
> +ssize_t git_pread(int fd,void *buf,size_t count,off_t offset)

Style: s/,/, /; applies to git-compat-util.h as well.

> +{
> +        off_t current_offset = lseek(fd, 0, SEEK_CUR);
> +
> +        if (lseek(fd, offset, SEEK_SET) < 0)
> +                return EINVAL;
> +
> +        ssize_t rc=xread(fd, buf, count);

Style: please don't have declaration after statement.  Yes, it
is in the more recent ANSI, but the rest of git has avoided it
so far and having the decls upfront makes it easier to read.

I do not think you would want to return EINVAL.

Andy's read_in_full() is now on 'master' so please use it
instead to avoid short read.

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

[Stefan-W. Hahn]

Thanks for the hints, here the next try.

Stefan

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

[Stefan-W. Hahn]

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);
+
+        if (current_offset != lseek(fd, current_offset, SEEK_SET))
+                return -1;
+        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);
-- 
1.4.4.4.gfa432

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

[Andy Whitcroft]

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

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

[Shawn O. Pearce]

Andy Whitcroft <apw@shadowen.org> wrote:
> 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.

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.

Though its a possibly unnecessary optimization as like Andy points
out, most sane systems already have a working pread() implementation.
And those that don't, well, probably should be made to be sane.
But we don't need to make Git suffer there if we don't have to.

-- 
Shawn.

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

[Johannes Schindelin]

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.

Ciao,
Dscho

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

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

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

I am a nice guy and do not reject a patch for missing two SP
characters which means I have to --amend it which takes time
away from me.  Maybe I should stop being nice ;-).

>> > +
>> > +        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.

The caller of pread() does not care the current position, but
that is not to mean it does not care the position after pread()
returns.  The current callers do not care, though.

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

[Johannes Schindelin]

Hi,

On Tue, 9 Jan 2007, Junio C Hamano wrote:

> The caller of pread() does not care the current position, but that is 
> not to mean it does not care the position after pread() returns.  The 
> current callers do not care, though.

Not completely true. We fixed in v1.4.4.1~23 a bug which was triggered by 
NO_MMAP. Since this recently became the default for cygwin, 
"git-index-pack --fix-thin" would fail in most cases.

Ciao,
Dscho

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

[Nicolas Pitre]

On Wed, 10 Jan 2007, Johannes Schindelin wrote:

> 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.

In the index-pack case it simply will never happen.


Nicolas

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

[Nicolas Pitre]

On Tue, 9 Jan 2007, Shawn O. Pearce wrote:

> 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.

No this is wrong.  The original offset _has_ to be preserved.  
index-pack counts on it.


Nicolas