sys->pread advances file position

[GoogleCodeExporter]

To reproduce:

(1) Open a file
(2) Read using sys->pread and specify an offset 'off'
(3) check read position of file descriptor (by printing sys->seek(fd, 0, 
SEEKRELA)

The position is advanced to  off + numbytes

The man pages say that the file position should be unchanged.

The implementation of pwrite in sysfile.c looks to be correct.  It calls 
rwrite.  rwrite only advances the file position if the offset isn't specified.

However pread calls rread which advances the file position regardless of the 
read type.

Forgive me if I'm wrong about this, but I was debugging a test suite and it 
popped up.

Original issue reported on code.google.com by jvbur...@gmail.com on 30 Jul 2014 at 8:51

[GoogleCodeExporter]

Hmm. You are right that it should not, and I suspect Plan 9 has the same bug.
Thanks.

Original comment by Charles....@gmail.com on 30 Jul 2014 at 10:48

• Changed state: Accepted

[GoogleCodeExporter]

It's interesting that it might need to do that for directories, but since you 
can't seek (except to the start) in directories, they won't be safely shared 
anyway and it doesn't matter.

Original comment by Charles....@gmail.com on 30 Jul 2014 at 10:53

[GoogleCodeExporter]

The fix looks pretty straightforward.  Just do what the developer did in 
'rwrite' -- preferentially advance the offset only for iops where the offset 
isn't specified.

I'll go ahead and patch my copy.  Should we submit upstream to the plan9 team?

Original comment by jvbur...@gmail.com on 30 Jul 2014 at 10:55

[GoogleCodeExporter]

yes, that's fine. It turned out to be fixed in a 64-bit variant of Plan 9, 
along with some other changes to speed up the usual path, originally for Blue 
Gene, but there are other differences that make it less easy than just a 
cut-and-paste at the moment.

Original comment by Charles....@gmail.com on 30 Jul 2014 at 11:08

[GoogleCodeExporter]

ok.  i've made some extensive mods to the inferno kernel.  one of the hacks
was a limitation in alloc.c where the allocation quanta limited it to 32
bit space (among other things).

Original comment by jvbur...@gmail.com on 30 Jul 2014 at 11:28

[GoogleCodeExporter]

Well I tried the quick hack of bumping the offset increment only for
user-supplied 'nil' offset values and that blew up directory reads
somehow.  I'll look at it more later.

Original comment by jvbur...@gmail.com on 31 Jul 2014 at 2:35

[GoogleCodeExporter]

I tried bumping the offset only for relative reads.   That didn't work because 
it somehow broke directory reads.  I tried to determine how the directory read 
logic was working, but fell asleep somewhere in the union read code.  I'll have 
another look at it today.

Original comment by jvbur...@gmail.com on 31 Jul 2014 at 5:50

[GoogleCodeExporter]

I've attached the 64-bit version I mentioned. Note that the system call calling 
conventions are a little different.

Original comment by Charles....@gmail.com on 31 Jul 2014 at 6:04

Attachments:

• sysfile.c

[GoogleCodeExporter]

Charles, I made a slight modification to my previous attempted patch and I 
think this should work.  It tests properly in my code.  Directory reads seem to 
work and 'pread' leaves the file position alone.

Just surround the current position bump in rread with a check for relative 
reads:

        if (offp == nil) {
            lock(cl);
            c.c->offset += n;
            unlock(cl);
        }

Does that work for you?

Original comment by jvbur...@gmail.com on 31 Jul 2014 at 6:12

[GoogleCodeExporter]

For subtle reasons, pread applied to a directory needs to advance the offset as 
if it were read although a pread at 0 resets the pointer (similar to "seeks in 
a directory are ignored, except to 0")

Original comment by Charles....@gmail.com on 31 Jul 2014 at 8:10

[GoogleCodeExporter]

That makes sense especially wrt my first patch not working.  It was bypassing 
the directory case.  The second patch seems to take that into account because 
it's applied to any relative rread request.

Original comment by jvbur...@gmail.com on 31 Jul 2014 at 8:15