Re: [ECOS] fseek on JFFS2

[Ivan Djelic <ivan dot djelic at parrot dot fr>]

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