# [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. 8 messages from 2007-01-09 to 2007-01-10. Participants: Stefan-W. Hahn, Andy Whitcroft, Shawn O. Pearce, Johannes Schindelin, Junio C Hamano, Nicolas Pitre. Thread: https://gitlist.dev/t/6316 ## Stefan-W. Hahn, 2007-01-09 21:04 Subject: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. Message-ID: <13884.2552820735$1168376671@news.gmane.org> URL: https://gitlist.dev/e/13884.2552820735%241168376671%40news.gmane.org In-Reply-To: <11683766523955-git-send-email-> ``` From: Stefan-W. Hahn 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 --- 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 ``` ## Andy Whitcroft, 2007-01-09 21:41 Subject: Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. Message-ID: <45A40C15.1070200@shadowen.org> URL: https://gitlist.dev/e/45A40C15.1070200%40shadowen.org In-Reply-To: <11683766521544-git-send-email-> ``` Stefan-W. Hahn wrote: > From: Stefan-W. Hahn > > 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 > --- > 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 ``` ## Shawn O. Pearce, 2007-01-09 23:25 Subject: Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. Message-ID: <20070109232540.GA30023@spearce.org> URL: https://gitlist.dev/e/20070109232540.GA30023%40spearce.org In-Reply-To: <45A40C15.1070200@shadowen.org> ``` Andy Whitcroft 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. ``` ## Johannes Schindelin, 2007-01-09 23:42 Subject: Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0701100041310.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <45A40C15.1070200@shadowen.org> ``` 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 ``` ## Junio C Hamano, 2007-01-10 00:21 Subject: Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. Message-ID: <7v7ivvivej.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v7ivvivej.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20070109232540.GA30023@spearce.org> ``` "Shawn O. Pearce" 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. ``` ## Johannes Schindelin, 2007-01-10 00:30 Subject: Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.63.0701100128410.22628%40wbgn013.biozentrum.uni-wuerzburg.de In-Reply-To: <7v7ivvivej.fsf@assigned-by-dhcp.cox.net> ``` 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 ``` ## Nicolas Pitre, 2007-01-10 00:59 Subject: Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0701091958170.4964%40xanadu.home In-Reply-To: ``` 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 ``` ## Nicolas Pitre, 2007-01-10 01:12 Subject: Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0701092010090.4964%40xanadu.home In-Reply-To: <20070109232540.GA30023@spearce.org> ``` On Tue, 9 Jan 2007, Shawn O. Pearce wrote: > Andy Whitcroft 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 ```