[ECOS] fseek on JFFS2

[ECOS] fseek on JFFS2

[Paluch Sebastian]

hi, i have strange problem when i'm using fwrite after fseek. eg.

this is samo file:

0a a0 10 03 00 49 00 00 00 0a a0 01 40 e2 01 00
01 a1 00 00 d8 fd 0c 8c 00 00 00 00 01 00 00 00
01 00 d8 00 40 01 db 00 01 00 d8 00 40 01 f0 00
01 00 01 00 40 01 f0 00 01 00 d8 00 40 01 db 00
01 00 d8 00 40 01 db 00 08 69 08 8c 01 00 d8 00
40 01 db 00 a2'0a'a0 01 80 24 11 00 02 a1 00 00 <-
d8 fd 0c 8c 00 00 00 00 01 00 00 00 01 00 d8 00
40 01 db 00 01 00 d8 00 40 01 f0 00 01 00 01 00
40 01 f0 00 01 00 d8 00 40 01 db 00 01 00 d8 00
40 01 db 00 08 69 08 8c 01 00 d8 00 40 01 db 00
a2 0a a0 01 02 6d ab 00 02 a1 00 00 d8 fd 0c 8c
00 00 00 00 01 00 00 00 01 00 d8 00 40 01 db 00
01 00 d8 00 40 01 f0 00 01 00 01 00 40 01 f0 00
01 00 d8 00 40 01 db 00 01 00 d8 00 40 01 db 00
08 69 08 8c 01 00 d8 00 40 01 db 00 a2 ff

and this is some code operating on this file:

    //seek to position marked in file
    if( fseek(file,85,SEEK_SET) )
    {
       return ferror(file);
    }
    //sizeof(header) = 3
    if( fread(&header,sizeof(header),1,file) != 1 )
    {
       return ferror(file);
    }
    //...
    //some checks on header
    //...

    if( fseek(table->file,85+sizeof(header),SEEK_SET) )
    {
       return ferror(file);
    }
    //sizeof(buf) = 73
    if( fwrite(buf,sizeof(buf),1,file) != 1 )
    {
       return ferror(file);
    }

buf starts with values:

40 e2 01 00 02 a1


so after this code i should have file:

0a a0 10 03 00 49 00 00 00 0a a0 01 40 e2 01 00
01 a1 00 00 d8 fd 0c 8c 00 00 00 00 01 00 00 00
01 00 d8 00 40 01 db 00 01 00 d8 00 40 01 f0 00
01 00 01 00 40 01 f0 00 01 00 d8 00 40 01 db 00
01 00 d8 00 40 01 db 00 08 69 08 8c 01 00 d8 00
40 01 db 00 a2 0a a0 01'40 e2 01 00 02 a1'00 00 <-
d8 fd 0c 8c 00 00 00 00 01 00 00 00 01 00 d8 00
40 01 db 00 01 00 d8 00 40 01 f0 00 01 00 01 00
40 01 f0 00 01 00 d8 00 40 01 db 00 01 00 d8 00
40 01 db 00 08 69 08 8c 01 00 d8 00 40 01 db 00
a2 0a a0 01 02 6d ab 00 02 a1 00 00 d8 fd 0c 8c
00 00 00 00 01 00 00 00 01 00 d8 00 40 01 db 00
01 00 d8 00 40 01 f0 00 01 00 01 00 40 01 f0 00
01 00 d8 00 40 01 db 00 01 00 d8 00 40 01 db 00
08 69 08 8c 01 00 d8 00 40 01 db 00 a2 ff


i should... but i have:

0a a0 10 03 00 49 00 00 00 0a a0 01 40 e2 01 00
01 a1 00 00 78 fd 0c 8c 00 00 00 00 01 00 00 00
01 00 d8 00 40 01 db 00 01 00 d8 00 40 01 f0 00
01 00 01 00 40 01 f0 00 01 00 d8 00 40 01 db 00
01 00 d8 00 40 01 db 00 a8 68 08 8c 01 00 d8 00
40 01 db 00 a2 0a a0 01 80 24 11 00 02 a1 00 00 <- should be here
78 fd 0c 8c 00 00 00 00 01 00 00 00 01 00 d8 00
40 01 db 00 01 00 d8 00 40 01 f0 00 01 00 01 00
40 01 f0 00 01 00 d8 00 40 01 db 00 01 00 d8 00
40 01 db 00 a8 68 08 8c 01 00 d8 00 40 01 db 00
a2 0a a0 01 02 6d ab 00 02 a1 00 00 78 fd 0c 8c
00 00 00 00 01 00 00 00 01 00 d8 00 40 01 db 00
01 00 d8 00 40 01 f0 00 01 00 01 00 40 01 f0 00
01 00 d8 00 40 01 db 00 01 00 d8 00 40 01 db 00
a8 68 08 8c 01 00 d8 00 40 01 db 00 a2'40 e2 01 <- is here
00 02 a1'00 00 78 fd 0c 8c 00 00 00 00 01 00 00
00 01 00 d8 00 40 01 db 00 01 00 d8 00 40 01 f0
00 01 00 01 00 40 01 f0 00 01 00 d8 00 40 01 db
00 01 00 d8 00 40 01 db 00 a8 68 08 8c 01 00 d8
00 40 01 db 00 a2 ff

it go at the end of the file, why?

when i write back readed header and then buf it's OK, but it should work  
in both way

    //seek to position marked in file
    if( fseek(file,85,SEEK_SET) )
    {
       return ferror(file);
    }
    //sizeof(header) = 3
    if( fread(&header,sizeof(header),1,file) != 1 )
    {
       return ferror(file);
    }
    //...
    //some checks on header
    //...

    if( fseek(table->file,85,SEEK_SET) )
    {
       return ferror(file);
    }
    //sizeof(header) = 3
    if( fwrite(&header,sizeof(header),1,file) != 1 )
    {
       return ferror(file);
    }
    //sizeof(buf) = 73
    if( fwrite(buf,sizeof(buf),1,file) != 1 )
    {
       return ferror(file);
    }

can somebody explain? thnx

-- 
Sebastian Paluch

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Andrew Lunn]

On Fri, Sep 22, 2006 at 12:50:09PM +0200, Paluch Sebastian wrote:
> hi, i have strange problem when i'm using fwrite after fseek. eg.

Could you please make a realy test case which demonstrates the problem
out of your code.  Maybe you can base it on the code in

/packages/fs/jffs2/current/tests/jffs2_2.c

which already does some seek tests because there was a bug once found
in this area. 

      Thanks
        Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Andrew Lunn]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

On Fri, Sep 22, 2006 at 02:26:30PM +0200, Paluch Sebastian wrote:
> hi,
> here you have some test file 'fseek_test.c', and my output.

I tried running the code on the ramfs. It also fails there as
well. Attached is the code. Please can you check it really does what
you want it to do. It could be that both jffs2 and ramfs is broken, or
it could be the test case is broken.  I've not looked at the test case
myself yet, so i cannot say either way.

       Andrew

[-- Attachment #2: fseek_test.c --]
[-- Type: text/x-csrc, Size: 4325 bytes --]

#include <stdio.h>
#include <unistd.h>
#include <fcntl.h>
#include <errno.h>
#include <string.h>

#include <cyg/fileio/fileio.h>
#include <cyg/io/flash.h>

#include <cyg/infra/testcase.h>
#include <cyg/infra/diag.h>            // HAL polled output
//==========================================================================

#define SHOW_RESULT( _fn, _res ) \
diag_printf("FAIL: " #_fn "() returned %ld %s\n", \
           (unsigned long)_res, _res<0?strerror(errno):"");

//==========================================================================

cyg_uint8 buf[256];
cyg_uint8 buf1[256];

//==========================================================================
// main

int main( int argc, char **argv )
{
    int err;
    FILE *stream;
    long pos;
    unsigned int i;
    cyg_interrupt_disable();
    CYG_TEST_INIT();

    // --------------------------------------------------------------

    CYG_TEST_INFO("mount /");    
    err = mount( "", "/", "ramfs" );

    if( err < 0 ) SHOW_RESULT( mount, err );    
    
    CYG_TEST_INFO("creating /fseek");
    stream = fopen("/fseek","w+");
    if (!stream) {
      diag_printf("FAIL: fopen() returned NULL, %s\n", strerror(errno));
      CYG_TEST_FINISH("done");          \
    }


    for (i = 0; i < sizeof(buf); i++) {
      buf[i] = i % 256;
    }
    
    CYG_TEST_INFO("writing test pattern");    
    err=fwrite(buf,sizeof(buf), 1, stream);
    if ( err < 0 ) SHOW_RESULT( fwrite, err );
    

    pos = ftell(stream);

    if (pos < 0) SHOW_RESULT( ftell, pos );
    if (pos != sizeof(buf))
      diag_printf("<FAIL>: ftell is not telling the truth.");
    
    CYG_TEST_INFO("fseek()ing to 85");


    err = fseek(stream, 85, SEEK_SET);
    if ( err < 0 ) SHOW_RESULT( fseek, err );

    pos = ftell(stream);
    
    if (pos < 0) SHOW_RESULT( ftell, pos );
    if (pos != 85) CYG_TEST_FAIL("ftell is not telling the truth");


    char header[3];
    err = fread(header,3,1,stream);
    if ( err < 0 ) SHOW_RESULT( fwrite, err );

    pos = ftell(stream);
    
    if (pos < 0) SHOW_RESULT( ftell, pos );
    if (pos != 88)  CYG_TEST_FAIL("ftell is not telling the truth");

    for (i = 88; i < 161; i++) {
      buf[i] = 0;
    }
    CYG_TEST_INFO("writing");
    err = fwrite(buf+88, 73, 1, stream);
    if ( err < 0 ) SHOW_RESULT( fwrite, err );

    pos = ftell(stream);
    
    if (pos < 0) SHOW_RESULT( ftell, pos );
    if (pos != 161)  CYG_TEST_FAIL("ftell is not telling the truth");

    CYG_TEST_INFO("closing file");

    err = fclose(stream);
    if (err != 0) SHOW_RESULT( fclose, err );

    CYG_TEST_INFO("open file /fseek");
    stream = fopen("/fseek", "r+");
    if (!stream) {
      diag_printf("<FAIL>: fopen() returned NULL, %s\n", strerror(errno));
    }

    err = fseek(stream, 0, SEEK_SET);
    if ( err < 0 ) SHOW_RESULT( fseek, err );

    err = fread(buf1,sizeof(buf1),1, stream);
    if (err != 1) SHOW_RESULT( fread, err );

    CYG_TEST_INFO("Comparing contents");
    if (memcmp(buf, buf1, sizeof(buf1))) {
      CYG_TEST_FAIL("File contents inconsistent");
      
       int t = 0;
       cyg_uint8 c;
   
       err = fseek(stream, 0, SEEK_SET);
       if ( err < 0 ) SHOW_RESULT( fseek, err );
   
       CYG_TEST_INFO("file dump");
       while( !feof(stream) )
       {
         
         c = (cyg_uint8)getc(stream);
         if( !feof(stream) )
         {
            diag_printf("%02x ",c);
            t++;
            if( t == 16 )
            {
               diag_printf("\n");
               t = 0;
            }
         }
       }
       diag_printf("\n");
       t=0;
       CYG_TEST_INFO("buf dump");
       for (i = 0; i < sizeof(buf); i++) {
         diag_printf("%02x ",buf[i]);
         t++;
         if( t == 16 )
         {
            diag_printf("\n");
            t = 0;
         }
       }
       diag_printf("\n");      


    }
    
    CYG_TEST_INFO("closing file");

    err = fclose(stream);
    if (err != 0) SHOW_RESULT( fclose, err );


    CYG_TEST_INFO("umount /");    
    err = umount( "/" );
    if( err < 0 ) SHOW_RESULT( umount, err );    
    cyg_interrupt_enable();
    CYG_TEST_PASS_FINISH("jffs2_2");
}


[-- Attachment #3: Type: text/plain, Size: 148 bytes --]

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Andrew Lunn]

On Fri, Sep 22, 2006 at 03:25:09PM +0200, Andrew Lunn wrote:
> On Fri, Sep 22, 2006 at 02:26:30PM +0200, Paluch Sebastian wrote:
> > hi,
> > here you have some test file 'fseek_test.c', and my output.
> 
> I tried running the code on the ramfs. It also fails there as
> well. Attached is the code.

Hi Sebastian

I spent some time looking at this yesterday. There is definitely a
problem with the stdio stream implementation of buffered IO. The file
position as kept by the stream layer and the file system get out of
sync and so writes are made to the wrong position. 

What i don't understand is how it is supposed to work. The stream
layer does not make any lseek()s when reading/writing blocks to the
file system. It seems to assume that synchronisation will just be
kept.  When just doing simple linear fread()s or linear fwrite()s
maybe this is true, but as soon as you start fseek()ing around this
does not hold. Either i'm missing something in how the code is
supposed to work, or maybe the basic design did not implement fseek()
and it was incorrectly added later?

Jifl, this seems to be your code. I know you wrote it a long time ago,
but do you remember how this is supposed to work?

    Thanks
        Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Jonathan Larmour]

Andrew Lunn wrote:
> On Fri, Sep 22, 2006 at 03:25:09PM +0200, Andrew Lunn wrote:
>> On Fri, Sep 22, 2006 at 02:26:30PM +0200, Paluch Sebastian wrote:
>>> hi,
>>> here you have some test file 'fseek_test.c', and my output.
>> I tried running the code on the ramfs. It also fails there as
>> well. Attached is the code.
> 
> Hi Sebastian
> 
> I spent some time looking at this yesterday. There is definitely a
> problem with the stdio stream implementation of buffered IO. The file
> position as kept by the stream layer and the file system get out of
> sync and so writes are made to the wrong position. 
> 
> What i don't understand is how it is supposed to work. The stream
> layer does not make any lseek()s when reading/writing blocks to the
> file system. It seems to assume that synchronisation will just be
> kept.  When just doing simple linear fread()s or linear fwrite()s
> maybe this is true, but as soon as you start fseek()ing around this
> does not hold. Either i'm missing something in how the code is
> supposed to work, or maybe the basic design did not implement fseek()
> and it was incorrectly added later?

It was subsequently added (the stdio package predates filesystem support by 
many years - before then it only used to work on streams), but this is all 
meant to be handled in Cyg_StdioStream::set_position and I thought was 
being used in various places so I didn't think was completely broken.

> Jifl, this seems to be your code.

Some of, less so this bit in fact. Although scanning through the code, 
nothing seems obviously wrong either.

> I know you wrote it a long time ago,
> but do you remember how this is supposed to work?

set_position() calculates the new absolute position and calls 
cyg_stdio_lseek() which (with CYGPKG_FILEIO) is meant to be just a thin 
veneer over lseek().

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Andrew Lunn]

> set_position() calculates the new absolute position and calls 
> cyg_stdio_lseek() which (with CYGPKG_FILEIO) is meant to be just a thin 
> veneer over lseek().

fseek() works fine. However flush_output_unlocked() does not call
set_position() before flushing out the buffer. It seems to assume the
file position is already in the correct position. The test case shows
its not. What has happened is a read is made which fills the buffer
and obviously leaves the file pointer after the end of the data just
read. There is when a write within this buffer, which is naturally
buffered. The test case then does a fseek() back to the beginning of
the file. This causes the buffer to be flushed, which just causes it
to be written out. There is no set_position() called to seek back to
the start of the buffer location in the file, so the data gets written
in the wrong place. 

   Andrew

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Jonathan Larmour]

[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]

Andrew Lunn wrote:
>> set_position() calculates the new absolute position and calls 
>> cyg_stdio_lseek() which (with CYGPKG_FILEIO) is meant to be just a thin 
>> veneer over lseek().
> 
> fseek() works fine. However flush_output_unlocked() does not call
> set_position() before flushing out the buffer. It seems to assume the
> file position is already in the correct position. The test case shows
> its not. What has happened is a read is made which fills the buffer
> and obviously leaves the file pointer after the end of the data just
> read. There is when a write within this buffer, which is naturally
> buffered.  The test case then does a fseek() back to the beginning of
> the file. This causes the buffer to be flushed, which just causes it
> to be written out. There is no set_position() called to seek back to
> the start of the buffer location in the file, so the data gets written
> in the wrong place. 

That doesn't seem like the right explanation. First of all, let's clarify 
nomenclature: there are two "file positions" around, the stdio file 
position which is where stdio will be reading from/writing to next, and 
thus takes into account the buffer; and the underlying file pointer, which 
is what is (or could be) passed to lseek, and is updated by read()s and 
write()s as well. I'll call these the sfp and the ufp.

What you describe above is correct as written. Here's what should be happening:
After the first read, the sfp and the ufp will be updated. The sfp should 
reflect what point was actually read to and likely corresponds to the start 
of the buffer. The ufp corresponds to the end of the buffer. During the 
write, the old read data in the buffer is emptied. After the write, there 
may be stuff pending to be flushed in the buffer or not, but regardless sfp 
should only be greater than ufp by the number of bytes in the buffer. The 
fseek should cause the buffer to be flushed. flush_output_unlocked() will 
write the buffer at the current ufp. After which sfp will equal ufp and 
then the file is seeked.

So from looking at the code, and comparing to what should happen, I think 
flush_output_unlocked is ok. Instead I think the problem is in 
Cyg_StdioStream::write(). I think the problem is that we are nuking the 
buffer but not accounting for the possibility of sfp != ufp. We need to 
reset the ufp accordingly.

I think there is a problem in this area (but for a different reason) in 
Cyg_StdioStream::read() as well.

The testcase as well as all the stdio and ramfs tests, pass with the 
attached patch which I'll check in (easiest way for you to try it :-)). Let 
me know whether you think it's ok.

It would be nice if in exchange, one of you could tidy up that testcase to 
be a proper testcase suitable to be checked in.

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine

[-- Attachment #2: stdio.fseek.bug.anoncvs.patch --]
[-- Type: text/x-patch, Size: 2536 bytes --]

Index: src/common/stream.cxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/stream.cxx,v
retrieving revision 1.1.1.1
diff -u -5 -p -r1.1.1.1 stream.cxx
--- src/common/stream.cxx	4 Sep 2006 15:56:17 -0000	1.1.1.1
+++ src/common/stream.cxx	26 Sep 2006 18:44:33 -0000
@@ -7,10 +7,11 @@
 //========================================================================
 //####ECOSGPLCOPYRIGHTBEGIN####
 // -------------------------------------------
 // This file is part of eCos, the Embedded Configurable Operating System.
 // Copyright (C) 1998, 1999, 2000, 2001, 2002 Red Hat, Inc.
+// Copyright (C) 2006 eCosCentric Limited
 //
 // eCos is free software; you can redistribute it and/or modify it under
 // the terms of the GNU General Public License as published by the Free
 // Software Foundation; either version 2 or (at your option) any later version.
 //
@@ -384,12 +385,10 @@ Cyg_StdioStream::read( cyg_uint8 *user_b
         *user_buffer = readbuf_char;
         *bytes_read = 1;
         flags.readbuf_char_in_use = false;
     }
 
-    position += *bytes_read;
-    
 
     // if we are unbuffered, we read as much as we can directly from the 
     // file system at this point.
     //
     // unless we do this, we could end up reading byte-by-byte from the filing system
@@ -403,10 +402,12 @@ Cyg_StdioStream::read( cyg_uint8 *user_b
         len=buffer_length-*bytes_read;
         read_err = cyg_stdio_read(my_device, user_buffer + *bytes_read, &len);      
         *bytes_read+=len;
     }
     
+    position += *bytes_read;
+    
     unlock_me();
 
     return read_err;
 } // read()
 
@@ -613,12 +614,26 @@ Cyg_StdioStream::write( const cyg_uint8 
         unlock_me();
         return EINVAL;
     }
 
 #ifdef CYGSEM_LIBC_STDIO_WANT_BUFFERED_IO
-    if (flags.last_buffer_op_was_read == true)
+    if (flags.last_buffer_op_was_read == true) {
+#ifdef CYGPKG_LIBC_STDIO_FILEIO
+        if ( 0 != io_buf.get_buffer_space_used() )
+        {
+            off_t newpos = position;
+            io_buf.drain_buffer();  // nuke input bytes to prevent confusion
+            Cyg_ErrNo err = cyg_stdio_lseek( my_device, &newpos, SEEK_SET );
+            if (err) {
+                unlock_me();
+                return err;
+            }
+        }
+#else
         io_buf.drain_buffer();  // nuke input bytes to prevent confusion
+#endif
+    }
 
     flags.last_buffer_op_was_read = false;
 
     if (!flags.buffering) {
 #endif


[-- Attachment #3: Type: text/plain, Size: 148 bytes --]

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Ivan Djelic]

Hello all,

I believe the problems you are discussing may be related to several bugs I
found in Cyg_StdioStream::set_position():

1) Type 'fpos_t' of parameter 'pos' is defined as cyg_ucount32, whereas
'pos' is generally assumed to be signed; which triggers a few annoying bugs.
For instance, if you do something like:

 fgetc(fp);
 fseek( fp, -1, SEEK_CUR );

 then set_position() gets called with pos = 0xffffffff (unsigned).

 'fpos_t' could probably be defined as cyg_count32, at least that's
 how I fixed the problem.

2) When CYGPKG_LIBC_STDIO_FILEIO is used, there is a piece of code in
set_position() which (as was said in some earlier post):
"calculates the new absolute position and calls cyg_stdio_lseek()".
Unfortunately it does not work with the following example:

  fgetc(fp);
  fseek( fp, -1, SEEK_CUR );

Assuming the stream uses buffers of 256 bytes, and using the 'sfp'/'ufp'
notation introduced in earlier posts, we have:

before fgetc() (assume we just opened the file):
  sfp = 0
  ufp = 0

after fgetc():
  sfp = 1
  ufp = 0x100 (because we read a full buffer)

At this point, set_position() is called with:
  whence = SEEK_CUR
  pos = -1 (or 0xffffffff is fpos_t is cyg_ucount32, it does not really change
            anything here)

The following variables are computed:
  abspos = position + pos;
  posdiff = abspos - position; ( = pos )

-> posdiff is equal to -1

then we have:

  position += bytesavail;

so that 'position' is now equal to ufp (0x100).

Then,

  cyg_stdio_lseek( my_device, &newpos, whence );

with newpos == -1, whence == SEEK_CUR. Remember that ufp = 0x100.

and finally 'position' is updated:

  position = newpos; (= 0xff)

Finally, we have:

sfp = 0xff
ufp = 0xff

But this is *not* what we want ! We want to have sfp = 0, so that for instance
ftell(fp) returns 0.
The problem is that we forced sfp to be equal to ufp before seeking, whereas
it should have been the other way around.
Here is my fix:


@@ -441,7 +441,9 @@ Cyg_StdioStream::set_position( fpos_t po
         } // endif (bytesavail > posdiff)
 
         if (whence == SEEK_CUR) {
-            position += bytesavail;
+          pos += position;
+          whence = SEEK_SET;
         }
     } //endif (whence != SEEK_END)
 

That way, we convert 'pos' to an offset from the beginning of the stream
(SEEK_SET), and we do not depend on the value of ufp when seeking... 

Ivan



On Tue, Sep 26, 2006 at 07:45:21PM +0100, Jonathan Larmour wrote:
> Andrew Lunn wrote:
> >>set_position() calculates the new absolute position and calls 
> >>cyg_stdio_lseek() which (with CYGPKG_FILEIO) is meant to be just a thin 
> >>veneer over lseek().
> >
> >fseek() works fine. However flush_output_unlocked() does not call
> >set_position() before flushing out the buffer. It seems to assume the
> >file position is already in the correct position. The test case shows
> >its not. What has happened is a read is made which fills the buffer
> >and obviously leaves the file pointer after the end of the data just
> >read. There is when a write within this buffer, which is naturally
> >buffered.  The test case then does a fseek() back to the beginning of
> >the file. This causes the buffer to be flushed, which just causes it
> >to be written out. There is no set_position() called to seek back to
> >the start of the buffer location in the file, so the data gets written
> >in the wrong place. 
> 
> That doesn't seem like the right explanation. First of all, let's clarify 
> nomenclature: there are two "file positions" around, the stdio file 
> position which is where stdio will be reading from/writing to next, and 
> thus takes into account the buffer; and the underlying file pointer, which 
> is what is (or could be) passed to lseek, and is updated by read()s and 
> write()s as well. I'll call these the sfp and the ufp.
> 
> What you describe above is correct as written. Here's what should be 
> happening:
> After the first read, the sfp and the ufp will be updated. The sfp should 
> reflect what point was actually read to and likely corresponds to the start 
> of the buffer. The ufp corresponds to the end of the buffer. During the 
> write, the old read data in the buffer is emptied. After the write, there 
> may be stuff pending to be flushed in the buffer or not, but regardless sfp 
> should only be greater than ufp by the number of bytes in the buffer. The 
> fseek should cause the buffer to be flushed. flush_output_unlocked() will 
> write the buffer at the current ufp. After which sfp will equal ufp and 
> then the file is seeked.
> 
> So from looking at the code, and comparing to what should happen, I think 
> flush_output_unlocked is ok. Instead I think the problem is in 
> Cyg_StdioStream::write(). I think the problem is that we are nuking the 
> buffer but not accounting for the possibility of sfp != ufp. We need to 
> reset the ufp accordingly.
> 
> I think there is a problem in this area (but for a different reason) in 
> Cyg_StdioStream::read() as well.
> 
> The testcase as well as all the stdio and ramfs tests, pass with the 
> attached patch which I'll check in (easiest way for you to try it :-)). Let 
> me know whether you think it's ok.
> 
> It would be nice if in exchange, one of you could tidy up that testcase to 
> be a proper testcase suitable to be checked in.
> 
> Jifl
> -- 
> eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
> ------["The best things in life aren't things."]------      Opinions==mine

> Index: src/common/stream.cxx
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/stream.cxx,v
> retrieving revision 1.1.1.1
> diff -u -5 -p -r1.1.1.1 stream.cxx
> --- src/common/stream.cxx	4 Sep 2006 15:56:17 -0000	1.1.1.1
> +++ src/common/stream.cxx	26 Sep 2006 18:44:33 -0000
> @@ -7,10 +7,11 @@
>  //========================================================================
>  //####ECOSGPLCOPYRIGHTBEGIN####
>  // -------------------------------------------
>  // This file is part of eCos, the Embedded Configurable Operating System.
>  // Copyright (C) 1998, 1999, 2000, 2001, 2002 Red Hat, Inc.
> +// Copyright (C) 2006 eCosCentric Limited
>  //
>  // eCos is free software; you can redistribute it and/or modify it under
>  // the terms of the GNU General Public License as published by the Free
>  // Software Foundation; either version 2 or (at your option) any later version.
>  //
> @@ -384,12 +385,10 @@ Cyg_StdioStream::read( cyg_uint8 *user_b
>          *user_buffer = readbuf_char;
>          *bytes_read = 1;
>          flags.readbuf_char_in_use = false;
>      }
>  
> -    position += *bytes_read;
> -    
>  
>      // if we are unbuffered, we read as much as we can directly from the 
>      // file system at this point.
>      //
>      // unless we do this, we could end up reading byte-by-byte from the filing system
> @@ -403,10 +402,12 @@ Cyg_StdioStream::read( cyg_uint8 *user_b
>          len=buffer_length-*bytes_read;
>          read_err = cyg_stdio_read(my_device, user_buffer + *bytes_read, &len);      
>          *bytes_read+=len;
>      }
>      
> +    position += *bytes_read;
> +    
>      unlock_me();
>  
>      return read_err;
>  } // read()
>  
> @@ -613,12 +614,26 @@ Cyg_StdioStream::write( const cyg_uint8 
>          unlock_me();
>          return EINVAL;
>      }
>  
>  #ifdef CYGSEM_LIBC_STDIO_WANT_BUFFERED_IO
> -    if (flags.last_buffer_op_was_read == true)
> +    if (flags.last_buffer_op_was_read == true) {
> +#ifdef CYGPKG_LIBC_STDIO_FILEIO
> +        if ( 0 != io_buf.get_buffer_space_used() )
> +        {
> +            off_t newpos = position;
> +            io_buf.drain_buffer();  // nuke input bytes to prevent confusion
> +            Cyg_ErrNo err = cyg_stdio_lseek( my_device, &newpos, SEEK_SET );
> +            if (err) {
> +                unlock_me();
> +                return err;
> +            }
> +        }
> +#else
>          io_buf.drain_buffer();  // nuke input bytes to prevent confusion
> +#endif
> +    }
>  
>      flags.last_buffer_op_was_read = false;
>  
>      if (!flags.buffering) {
>  #endif


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Jonathan Larmour]

Ivan Djelic wrote:
> Hello all,
> 
> I believe the problems you are discussing may be related to several bugs I
> found in Cyg_StdioStream::set_position():
> 
> 1) Type 'fpos_t' of parameter 'pos' is defined as cyg_ucount32, whereas
> 'pos' is generally assumed to be signed; which triggers a few annoying bugs.
> For instance, if you do something like:
> 
>  fgetc(fp);
>  fseek( fp, -1, SEEK_CUR );
> 
>  then set_position() gets called with pos = 0xffffffff (unsigned).
> 
>  'fpos_t' could probably be defined as cyg_count32, at least that's
>  how I fixed the problem.

Ok.

> 2) When CYGPKG_LIBC_STDIO_FILEIO is used, there is a piece of code in
> set_position() which (as was said in some earlier post):
> "calculates the new absolute position and calls cyg_stdio_lseek()".
> Unfortunately it does not work with the following example:
> 
>   fgetc(fp);
>   fseek( fp, -1, SEEK_CUR );
> 
> Assuming the stream uses buffers of 256 bytes, and using the 'sfp'/'ufp'
> notation introduced in earlier posts, we have:
> 
> before fgetc() (assume we just opened the file):
>   sfp = 0
>   ufp = 0
> 
> after fgetc():
>   sfp = 1
>   ufp = 0x100 (because we read a full buffer)
> 
> At this point, set_position() is called with:
>   whence = SEEK_CUR
>   pos = -1 (or 0xffffffff is fpos_t is cyg_ucount32, it does not really change
>             anything here)
> 
> The following variables are computed:
>   abspos = position + pos;
>   posdiff = abspos - position; ( = pos )
> 
> -> posdiff is equal to -1
> 
> then we have:
> 
>   position += bytesavail;
> 
> so that 'position' is now equal to ufp (0x100).
> 
> Then,
> 
>   cyg_stdio_lseek( my_device, &newpos, whence );
> 
> with newpos == -1, whence == SEEK_CUR. Remember that ufp = 0x100.
> 
> and finally 'position' is updated:
> 
>   position = newpos; (= 0xff)
> 
> Finally, we have:
> 
> sfp = 0xff
> ufp = 0xff
> 
> But this is *not* what we want ! We want to have sfp = 0, so that for instance
> ftell(fp) returns 0.
> The problem is that we forced sfp to be equal to ufp before seeking, whereas
> it should have been the other way around.
> Here is my fix:
> 
> 
> @@ -441,7 +441,9 @@ Cyg_StdioStream::set_position( fpos_t po
>          } // endif (bytesavail > posdiff)
>  
>          if (whence == SEEK_CUR) {
> -            position += bytesavail;
> +          pos += position;
> +          whence = SEEK_SET;
>          }
>      } //endif (whence != SEEK_END)
>  
> 
> That way, we convert 'pos' to an offset from the beginning of the stream
> (SEEK_SET), and we do not depend on the value of ufp when seeking... 

I agree with your analysis, but I think I would prefer a fix like:

Index: include/stream.inl
===================================================================
RCS file: 
/cvs/ecos/ecos/packages/language/c/libc/stdio/current/include/stream.inl,v
retrieving revision 1.7
diff -u -5 -p -r1.7 stream.inl
--- include/stream.inl  29 Mar 2004 11:24:38 -0000      1.7
+++ include/stream.inl  27 Sep 2006 15:14:52 -0000
@@ -440,10 +440,11 @@ Cyg_StdioStream::set_position( fpos_t po
              return ENOERR;
          } // endif (bytesavail > posdiff)

          if (whence == SEEK_CUR) {
              position += bytesavail;
+            pos -= bytesavail;
          }
      } //endif (whence != SEEK_END)

      Cyg_ErrNo err;

What do you think?

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Jonathan Larmour]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

Jonathan Larmour wrote:
> 
> I agree with your analysis, but I think I would prefer a fix like:
> 
> Index: include/stream.inl
> ===================================================================
> RCS file: 
> /cvs/ecos/ecos/packages/language/c/libc/stdio/current/include/stream.inl,v
> retrieving revision 1.7
> diff -u -5 -p -r1.7 stream.inl
> --- include/stream.inl  29 Mar 2004 11:24:38 -0000      1.7
> +++ include/stream.inl  27 Sep 2006 15:14:52 -0000
> @@ -440,10 +440,11 @@ Cyg_StdioStream::set_position( fpos_t po
>              return ENOERR;
>          } // endif (bytesavail > posdiff)
> 
>          if (whence == SEEK_CUR) {
>              position += bytesavail;
> +            pos -= bytesavail;
>          }
>      } //endif (whence != SEEK_END)
> 
>      Cyg_ErrNo err;
> 
> What do you think?

Actually since at worst it doesn't cause any test regressions, I'll check 
it in, because I'm an optimist. Full patch attached.

Jifl
-- 
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
------["The best things in life aren't things."]------      Opinions==mine

[-- Attachment #2: stdio.seek.tweaks.patch.txt --]
[-- Type: text/plain, Size: 2358 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/ChangeLog,v
retrieving revision 1.32.10004.9
diff -u -5 -p -r1.32.10004.9 ChangeLog
--- ChangeLog	26 Sep 2006 18:47:19 -0000	1.32.10004.9
+++ ChangeLog	27 Sep 2006 16:16:05 -0000
@@ -1,5 +1,15 @@
+2006-09-27  Jonathan Larmour  <jifl@eCosCentric.com>
+
+	* include/stdio.h: Make fpos_t be signed to allow negative
+	SEEK_CUR offsets to fseek().
+	* include/stream.inl (set_position): If SEEK_CUR, then if
+	having to reconcile difference between position and underlying
+	file position, then requested seek position needs adjusting
+	for buffer size.
+	Both above reported and analysed by Ivan Djelic.
+
 2006-09-26  Jonathan Larmour  <jifl@eCosCentric.com>
 
 	* src/common/stream.cxx (read): Only update position after direct
 	reads from I/O system so it's updated by the correct number of
 	bytes.
Index: include/stdio.h
===================================================================
RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/include/stdio.h,v
retrieving revision 1.6.10012.1
diff -u -5 -p -r1.6.10012.1 stdio.h
--- include/stdio.h	14 Sep 2006 12:21:43 -0000	1.6.10012.1
+++ include/stdio.h	27 Sep 2006 16:16:06 -0000
@@ -87,11 +87,11 @@
 
 // TYPE DEFINITIONS
 
 // A type capable of specifying uniquely every file position - ISO C
 // standard chap 7.9.1
-typedef cyg_ucount32 fpos_t;
+typedef cyg_count32 fpos_t;
 
 
 // FILE is just cast to an address here. It is uncast internally to the
 // C library in stream.hxx  as the C++ Cyg_StdioStream class.
 // Optional run-time checking can be enabled to ensure that the cast is
Index: include/stream.inl
===================================================================
RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/include/stream.inl,v
retrieving revision 1.7
diff -u -5 -p -r1.7 stream.inl
--- include/stream.inl	29 Mar 2004 11:24:38 -0000	1.7
+++ include/stream.inl	27 Sep 2006 16:16:06 -0000
@@ -440,10 +440,11 @@ Cyg_StdioStream::set_position( fpos_t po
             return ENOERR;
         } // endif (bytesavail > posdiff)
 
         if (whence == SEEK_CUR) {
             position += bytesavail;
+            pos -= bytesavail;
         }
     } //endif (whence != SEEK_END)
 
     Cyg_ErrNo err;
 


[-- Attachment #3: Type: text/plain, Size: 148 bytes --]

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss

Re: [ECOS] fseek on JFFS2

[Ivan Djelic]

On Wed, Sep 27, 2006 at 04:15:39PM +0100, Jonathan Larmour wrote:
> I agree with your analysis, but I think I would prefer a fix like:
> 
> Index: include/stream.inl
> ===================================================================
> RCS file: 
> /cvs/ecos/ecos/packages/language/c/libc/stdio/current/include/stream.inl,v
> retrieving revision 1.7
> diff -u -5 -p -r1.7 stream.inl
> --- include/stream.inl  29 Mar 2004 11:24:38 -0000      1.7
> +++ include/stream.inl  27 Sep 2006 15:14:52 -0000
> @@ -440,10 +440,11 @@ Cyg_StdioStream::set_position( fpos_t po
>              return ENOERR;
>          } // endif (bytesavail > posdiff)
> 
>          if (whence == SEEK_CUR) {
>              position += bytesavail;
> +            pos -= bytesavail;
>          }
>      } //endif (whence != SEEK_END)
> 
>      Cyg_ErrNo err;
> 
> What do you think?

My fix sure was a hasty one. This one looks better, I'll take it :-)

Ivan

-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss