This is the mail archive of the ecos-patches@sourceware.org mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [ECOS] fseek on JFFS2


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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]