{"thread":{"id":"6316","subject":"[PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","startedAt":"2007-01-09T21:04:12Z","lastAt":"2007-01-10T01:12:34Z","messageCount":8,"participants":["Stefan-W. Hahn","Andy Whitcroft","Shawn O. Pearce","Johannes Schindelin","Junio C Hamano","Nicolas Pitre"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"31281","messageId":"13884.2552820735$1168376671@news.gmane.org","threadId":"6316","inReplyTo":"11683766523955-git-send-email-","subject":"[PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","fromName":"Stefan-W. Hahn","fromEmail":"stefan.hahn@s-hahn.de","sentAt":"2007-01-09T21:04:12Z","receivedAt":"2007-01-09T21:04:12Z","isPatch":true,"sender":{"key":"stefan.hahn@s-hahn.de","avatar":null},"body":"From: Stefan-W. Hahn \n\nUsing cygwin with cygwin.dll before 1.5.22 the system call pread() is buggy.\nThis patch introduces NO_PREAD. If NO_PREAD is set git uses a sequence of\nlseek()/xread()/lseek() to emulate pread.\n\nSigned-off-by: Stefan-W. Hahn \n---\n Makefile | 7 +++++++\n compat/pread.c | 18 ++++++++++++++++++\n git-compat-util.h | 5 +++++\n 3 files changed, 30 insertions(+), 0 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6c12bc6..43113e9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -69,6 +69,9 @@ all:\n #\n # Define NO_MMAP if you want to avoid mmap.\n #\n+# Define NO_PREAD if you have a problem with pread() system call (e.g.\n+# cygwin.dll before v1.5.22).\n+#\n # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n # generally faster on your platform than accessing the working directory.\n #\n@@ -523,6 +526,10 @@ ifdef NO_MMAP\n \tCOMPAT_CFLAGS += -DNO_MMAP\n \tCOMPAT_OBJS += compat/mmap.o\n endif\n+ifdef NO_PREAD\n+\tCOMPAT_CFLAGS += -DNO_PREAD\n+\tCOMPAT_OBJS += compat/pread.o\n+endif\n ifdef NO_FAST_WORKING_DIRECTORY\n \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n endif\ndiff --git a/compat/pread.c b/compat/pread.c\nnew file mode 100644\nindex 0000000..9183c05\n--- /dev/null\n+++ b/compat/pread.c\n@@ -0,0 +1,18 @@\n+#include \"../git-compat-util.h\"\n+\n+ssize_t git_pread(int fd, void *buf, size_t count, off_t offset)\n+{\n+ off_t current_offset;\n+ ssize_t rc;\n+\n+ current_offset = lseek(fd, 0, SEEK_CUR);\n+\n+ if (lseek(fd, offset, SEEK_SET) < 0)\n+ return -1;\n+\n+ rc=read_in_full(fd, buf, count);\n+\n+ if (current_offset != lseek(fd, current_offset, SEEK_SET))\n+ return -1;\n+ return rc;\n+}\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e023bf1..f8d46d5 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -107,6 +107,11 @@ extern int git_munmap(void *start, size_t length);\n #define DEFAULT_PACKED_GIT_LIMIT \\\n \t((1024L * 1024L) * (sizeof(void*) >= 8 ? 8192 : 256))\n \n+#ifdef NO_PREAD\n+#define pread git_pread\n+extern ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);\n+#endif\n+\n #ifdef NO_SETENV\n #define setenv gitsetenv\n extern int gitsetenv(const char *, const char *, int);\n-- \n1.4.4.4.gfa432\n"},{"id":"31291","messageId":"45A40C15.1070200@shadowen.org","threadId":"6316","inReplyTo":"11683766521544-git-send-email-","subject":"Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","fromName":"Andy Whitcroft","fromEmail":"apw@shadowen.org","sentAt":"2007-01-09T21:41:41Z","receivedAt":"2007-01-09T21:41:41Z","isPatch":true,"sender":{"key":"apw@shadowen.org","avatar":"https://gravatar.com/avatar/d3088262854661a913ef35cc40fedcc270142d4461791142bc1ea0b2a4e2e147?d=mp&s=160"},"body":"Stefan-W. Hahn wrote:\n> From: Stefan-W. Hahn \n> \n> Using cygwin with cygwin.dll before 1.5.22 the system call pread() is buggy.\n> This patch introduces NO_PREAD. If NO_PREAD is set git uses a sequence of\n> lseek()/xread()/lseek() to emulate pread.\n> \n> Signed-off-by: Stefan-W. Hahn \n> ---\n> Makefile | 7 +++++++\n> compat/pread.c | 18 ++++++++++++++++++\n> git-compat-util.h | 5 +++++\n> 3 files changed, 30 insertions(+), 0 deletions(-)\n> \n> diff --git a/Makefile b/Makefile\n> index 6c12bc6..43113e9 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -69,6 +69,9 @@ all:\n> #\n> # Define NO_MMAP if you want to avoid mmap.\n> #\n> +# Define NO_PREAD if you have a problem with pread() system call (e.g.\n> +# cygwin.dll before v1.5.22).\n> +#\n> # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n> # generally faster on your platform than accessing the working directory.\n> #\n> @@ -523,6 +526,10 @@ ifdef NO_MMAP\n> \tCOMPAT_CFLAGS += -DNO_MMAP\n> \tCOMPAT_OBJS += compat/mmap.o\n> endif\n> +ifdef NO_PREAD\n> +\tCOMPAT_CFLAGS += -DNO_PREAD\n> +\tCOMPAT_OBJS += compat/pread.o\n> +endif\n> ifdef NO_FAST_WORKING_DIRECTORY\n> \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n> endif\n> diff --git a/compat/pread.c b/compat/pread.c\n> new file mode 100644\n> index 0000000..9183c05\n> --- /dev/null\n> +++ b/compat/pread.c\n> @@ -0,0 +1,18 @@\n> +#include \"../git-compat-util.h\"\n> +\n> +ssize_t git_pread(int fd, void *buf, size_t count, off_t offset)\n> +{\n> + off_t current_offset;\n> + ssize_t rc;\n> +\n> + current_offset = lseek(fd, 0, SEEK_CUR);\n> +\n> + if (lseek(fd, offset, SEEK_SET) < 0)\n> + return -1;\n> +\n> + rc=read_in_full(fd, buf, count);\n\nSeems to be style inconsistancy between current_offset = and rc= I\nbelieve the former is preferred.\n\n> +\n> + if (current_offset != lseek(fd, current_offset, SEEK_SET))\n> + return -1;\n\nHow likely are we ever to be in the right place here? Seems vanishingly\nsmall putting us firmly in the four syscalls per call space. I wonder\nif git ever actually cares about the seek location. ie if we could stop\nreading and resetting it. Probabally not worth working it out I guess\nas any _sane_ system has one.\n\n> + return rc;\n> +}\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index e023bf1..f8d46d5 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -107,6 +107,11 @@ extern int git_munmap(void *start, size_t length);\n> #define DEFAULT_PACKED_GIT_LIMIT \\\n> \t((1024L * 1024L) * (sizeof(void*) >= 8 ? 8192 : 256))\n> \n> +#ifdef NO_PREAD\n> +#define pread git_pread\n> +extern ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);\n> +#endif\n> +\n> #ifdef NO_SETENV\n> #define setenv gitsetenv\n> extern int gitsetenv(const char *, const char *, int);\n\n-apw\n"},{"id":"31303","messageId":"20070109232540.GA30023@spearce.org","threadId":"6316","inReplyTo":"45A40C15.1070200@shadowen.org","subject":"Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-01-09T23:25:41Z","receivedAt":"2007-01-09T23:25:41Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Andy Whitcroft wrote:\n> Stefan-W. Hahn wrote:\n> > Using cygwin with cygwin.dll before 1.5.22 the system call pread() is buggy.\n> > This patch introduces NO_PREAD. If NO_PREAD is set git uses a sequence of\n> > lseek()/xread()/lseek() to emulate pread.\n> > +\n> > + rc=read_in_full(fd, buf, count);\n> \n> Seems to be style inconsistancy between current_offset = and rc= I\n> believe the former is preferred.\n\nWith the exception of this style difference, the patch looked\npretty good. Nice work Stefan. Andy's right, we do tend to prefer\n\"rc = read_in_full\" over \"rc=read_in_full\". Quite a bit actually,\nthough Junio is the final decider on all such matters as he gets\nto choose to accept or reject the patch. ;-)\n\n> > +\n> > + if (current_offset != lseek(fd, current_offset, SEEK_SET))\n> > + return -1;\n> \n> How likely are we ever to be in the right place here? Seems vanishingly\n> small putting us firmly in the four syscalls per call space. I wonder\n> if git ever actually cares about the seek location. ie if we could stop\n> reading and resetting it. Probabally not worth working it out I guess\n> as any _sane_ system has one.\n\nAndy's right actually. If we are using pread() we aren't relying\non the current file pointer. Which means its unnecessary to get\nthe current pointer before seeking to the requested offset, and its\nunnecessary to restore it before the git_pread() function returns.\n\nThough its a possibly unnecessary optimization as like Andy points\nout, most sane systems already have a working pread() implementation.\nAnd those that don't, well, probably should be made to be sane.\nBut we don't need to make Git suffer there if we don't have to.\n\n-- \nShawn.\n"},{"id":"31308","messageId":"Pine.LNX.4.63.0701100041310.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6316","inReplyTo":"45A40C15.1070200@shadowen.org","subject":"Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-09T23:42:51Z","receivedAt":"2007-01-09T23:42:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 9 Jan 2007, Andy Whitcroft wrote:\n\n> Stefan-W. Hahn wrote:\n>\n> > + if (current_offset != lseek(fd, current_offset, SEEK_SET))\n> > + return -1;\n> \n> How likely are we ever to be in the right place here?\n\nYou mean something like\n\n\tif (current_offset != offset + count &&\n\t\t\tcurrent_offset != lseek(fd, current_offset, SEEK_SET))\n\t\treturn -1;\n\ninstead? Seems cheap enough.\n\nCiao,\nDscho\n"},{"id":"31320","messageId":"7v7ivvivej.fsf@assigned-by-dhcp.cox.net","threadId":"6316","inReplyTo":"20070109232540.GA30023@spearce.org","subject":"Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-10T00:21:24Z","receivedAt":"2007-01-10T00:21:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> With the exception of this style difference, the patch looked\n> pretty good. Nice work Stefan. Andy's right, we do tend to prefer\n> \"rc = read_in_full\" over \"rc=read_in_full\". Quite a bit actually,\n> though Junio is the final decider on all such matters as he gets\n> to choose to accept or reject the patch. ;-)\n\nI am a nice guy and do not reject a patch for missing two SP\ncharacters which means I have to --amend it which takes time\naway from me. Maybe I should stop being nice ;-).\n\n>> > +\n>> > + if (current_offset != lseek(fd, current_offset, SEEK_SET))\n>> > + return -1;\n>> \n>> How likely are we ever to be in the right place here? Seems vanishingly\n>> small putting us firmly in the four syscalls per call space. I wonder\n>> if git ever actually cares about the seek location. ie if we could stop\n>> reading and resetting it. Probabally not worth working it out I guess\n>> as any _sane_ system has one.\n>\n> Andy's right actually. If we are using pread() we aren't relying\n> on the current file pointer. Which means its unnecessary to get\n> the current pointer before seeking to the requested offset, and its\n> unnecessary to restore it before the git_pread() function returns.\n\nThe caller of pread() does not care the current position, but\nthat is not to mean it does not care the position after pread()\nreturns. The current callers do not care, though.\n"},{"id":"31324","messageId":"Pine.LNX.4.63.0701100128410.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6316","inReplyTo":"7v7ivvivej.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-10T00:30:47Z","receivedAt":"2007-01-10T00:30:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 9 Jan 2007, Junio C Hamano wrote:\n\n> The caller of pread() does not care the current position, but that is \n> not to mean it does not care the position after pread() returns. The \n> current callers do not care, though.\n\nNot completely true. We fixed in v1.4.4.1~23 a bug which was triggered by \nNO_MMAP. Since this recently became the default for cygwin, \n\"git-index-pack --fix-thin\" would fail in most cases.\n\nCiao,\nDscho\n"},{"id":"31335","messageId":"Pine.LNX.4.64.0701091958170.4964@xanadu.home","threadId":"6316","inReplyTo":"Pine.LNX.4.63.0701100041310.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-01-10T00:59:49Z","receivedAt":"2007-01-10T00:59:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 10 Jan 2007, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Tue, 9 Jan 2007, Andy Whitcroft wrote:\n> \n> > Stefan-W. Hahn wrote:\n> >\n> > > + if (current_offset != lseek(fd, current_offset, SEEK_SET))\n> > > + return -1;\n> > \n> > How likely are we ever to be in the right place here?\n> \n> You mean something like\n> \n> \tif (current_offset != offset + count &&\n> \t\t\tcurrent_offset != lseek(fd, current_offset, SEEK_SET))\n> \t\treturn -1;\n> \n> instead? Seems cheap enough.\n\nIn the index-pack case it simply will never happen.\n\n\nNicolas\n"},{"id":"31342","messageId":"Pine.LNX.4.64.0701092010090.4964@xanadu.home","threadId":"6316","inReplyTo":"20070109232540.GA30023@spearce.org","subject":"Re: [PATCH] Replacing the system call pread() with lseek()/xread()/lseek() sequence.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-01-10T01:12:34Z","receivedAt":"2007-01-10T01:12:34Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 9 Jan 2007, Shawn O. Pearce wrote:\n\n> Andy Whitcroft wrote:\n> > How likely are we ever to be in the right place here? Seems vanishingly\n> > small putting us firmly in the four syscalls per call space. I wonder\n> > if git ever actually cares about the seek location. ie if we could stop\n> > reading and resetting it. Probabally not worth working it out I guess\n> > as any _sane_ system has one.\n> \n> Andy's right actually. If we are using pread() we aren't relying\n> on the current file pointer. Which means its unnecessary to get\n> the current pointer before seeking to the requested offset, and its\n> unnecessary to restore it before the git_pread() function returns.\n\nNo this is wrong. The original offset _has_ to be preserved. \nindex-pack counts on it.\n\n\nNicolas\n"}]}