[ECOS] [eCos] a question about ROMFS

[ECOS] [eCos] a question about ROMFS

[jjtsai]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

Hi, 
    I have a question about ROM files 
system.
 
[Question Description]
    A fseek(,,SEEK_CUR) after fread() will 
cause inconsistency between "(CYG_StdioStream) 
real_stream.position"
    and "fp->f_offset". See also 
stream.inl and rom_fs.c.
    
    Here is my test program. See 
attchment: fseek_test.c  .
 
[My modfication]
 
See attachment: stream_inl_patch.pat.
Instruction about how to apply it: cd 
$(ECOS_REPOSITORY)/packages/language/c/libc/stdio/current/include patch -p0 
< $(WHERE_THE_PATCH_IS)/stream_inl_patch.pat
regards,
Jang-Jer
fseek_test.c
stream_inl_patch.pat


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

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <cyg/fileio/fileio.h>
#include <pkgconf/fs_rom.h>	// Address of ROMFS

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

MTAB_ENTRY( romfs_mte1,
                   "/",
                   "romfs",
                   "",
                   (CYG_ADDRWORD) CYGNUM_FS_ROM_BASE_ADDRESS );

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

int main( int argc, char **argv )
{
    FILE *fd;
    int err;
    int i;
    int length;
    char s[1000];
    struct stat sbuf;
    
    stat("/HelloWorld.jar", &sbuf);

    printf("leng=%d\n",sbuf.st_size);  

    if( (fd=fopen("/HelloWorld.jar","rw")) == NULL)
	printf("fopen error\n");
    else {
	length=0;

	do{
	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 0;

	if( (err=fseek(fd, 195, SEEK_SET))<0)
		printf("fseek=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 195

	if( (err=fread((s+length),sizeof(char), 30, fd))!=30) 
		printf("fread=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 225

	if( (err=fseek(fd, 0, SEEK_CUR))<0)
		printf("fseek=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 225

	if( (err=fseek(fd, 1, SEEK_CUR))<0)
		printf("fseek=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 226

	}
	while(0);

	fclose(fd);
	}

    return 0;

}


[-- Attachment #3: stream_inl_patch.pat --]
[-- Type: text/x-diff, Size: 566 bytes --]

--- stream.inl	Sat Apr  7 01:20:40 2001
+++ stream.inl.my	Mon Jul 16 14:37:34 2001
@@ -406,7 +406,16 @@
     if (!lock_me())
         return EBADF; // assume file is now invalid
 
-    err = cyg_stdio_lseek( my_device, &newpos, whence );
+    //add jjt
+    if( whence == SEEK_CUR ){ 
+	newpos = position + pos;
+	err = cyg_stdio_lseek( my_device, &newpos, SEEK_SET);
+	}
+    else {
+    	err = cyg_stdio_lseek( my_device, &newpos, whence );
+	}
+    //end jjt
+    //err = cyg_stdio_lseek( my_device, &newpos, whence ); //del jjt
 
     if( err == ENOERR )
     {

Re: [ECOS] [eCos] a question about ROMFS

[Jonathan Larmour]

> jjtsai wrote:
> 
> Hi,
>     I have a question about ROM files system.
> 
> [Question Description]
>     A fseek(,,SEEK_CUR) after fread() will cause inconsistency between
> "(CYG_StdioStream) real_stream.position"
>     and "fp->f_offset". See also stream.inl and rom_fs.c.

I believe I understand the problem: the position required by lseek differs
from the stream position, potentially by the number of bytes in the buffer.
In that case your patch is not quite complete to address this problem: what
if you are moving forward 1 byte, ie. fseek(,1, SEEK_CUR), *but* the data
is already in the buffer. For a true stream rather than a file, we should
not flush the input buffer.

I'll see if I can work on a better patch.

Jifl
-- 
Red Hat, Rustat House, Clifton Road, Cambridge, UK. Tel: +44 (1223) 271062
Maybe this world is another planet's Hell -Aldous Huxley || Opinions==mine
Come to the Red Hat TechWorld open source conference in Brussels!
Keynotes, techie talks and exhibitions    http://www.redhat-techworld.com/

Re: [ECOS] [eCos] a question about ROMFS

[jjtsai]

----- Original Message -----
From: Jonathan Larmour <jlarmour@redhat.com>
To: jjtsai <jjtsai@itri.org.tw>
Cc: <ecos-discuss@sources.redhat.com>
Sent: Tuesday, July 17, 2001 5:01 AM
Subject: Re: [ECOS] [eCos] a question about ROMFS


> > [Question Description]
> >     A fseek(,,SEEK_CUR) after fread() will cause inconsistency between
> > "(CYG_StdioStream) real_stream.position"
> >     and "fp->f_offset". See also stream.inl and rom_fs.c.
>
> I believe I understand the problem: the position required by lseek differs
> from the stream position, potentially by the number of bytes in the
buffer.
> In that case your patch is not quite complete to address this problem:
what
> if you are moving forward 1 byte, ie. fseek(,1, SEEK_CUR), *but* the data
> is already in the buffer. For a true stream rather than a file, we should
> not flush the input buffer.
Yes, you do understand my question and my solution. :p

> I'll see if I can work on a better patch.
That would be great!

best regards,
JJ

Re: [ECOS] [eCos] a question about ROMFS

[Jonathan Larmour]

jjtsai wrote:
> > I'll see if I can work on a better patch.
> That would be great!

Can you try the attached patch for me please? I haven't even tried
compiling it yet nevermind testing it, but I was hoping you could do that
instead :-). Let me know how it goes. Then I'll check it in.

Jifl
-- 
Red Hat, Rustat House, Clifton Road, Cambridge, UK. Tel: +44 (1223) 271062
Maybe this world is another planet's Hell -Aldous Huxley || Opinions==mine
Come to the Red Hat TechWorld open source conference in Brussels!
Keynotes, techie talks and exhibitions    http://www.redhat-techworld.com/
Index: include/stream.inl
===================================================================
RCS file: /home/cvs/ecc/ecc/language/c/libc/stdio/current/include/stream.inl,v
retrieving revision 1.3
diff -u -5 -p -r1.3 stream.inl
--- include/stream.inl	2001/03/15 20:30:22	1.3
+++ include/stream.inl	2001/07/17 20:05:43
@@ -361,19 +361,19 @@ Cyg_StdioStream::get_position( fpos_t *p
 
 // set absolute position
 inline Cyg_ErrNo
 Cyg_StdioStream::set_position( fpos_t pos, int whence )
 {
+    CYG_ASSERTCLASS( this, "Stream object is not a valid stream!" );
+    
 #ifndef CYGPKG_LIBC_STDIO_FILEIO    
     // this is currently a workaround until we have real files
     // this will be corrected when we decide the true filesystem interface
 
     Cyg_ErrNo err;
     cyg_uint8 c;
 
-    CYG_ASSERTCLASS( this, "Stream object is not a valid stream!" );
-    
     if ((whence != SEEK_CUR) || pos < 0)
         return ENOSYS;
 
     if (!lock_me())
         return EBADF; // assume file is now invalid
@@ -396,18 +396,46 @@ Cyg_StdioStream::set_position( fpos_t po
 
     return ENOERR;
     
 #else
 
-    Cyg_ErrNo err;
-    off_t newpos = pos;
-
-    CYG_ASSERTCLASS( this, "Stream object is not a valid stream!" );
-    
     if (!lock_me())
         return EBADF; // assume file is now invalid
 
+    if ( whence != SEEK_END ) {
+        cyg_ucount32 bytesavail = bytes_available_to_read();
+        off_t abspos = (whence == SEEK_CUR) ? position + pos : pos;
+        off_t posdiff = abspos - position;
+    
+        if ( bytesavail > posdiff ) {
+            // can just "seek" within the existing buffer
+#ifdef CYGFUN_LIBC_STDIO_ungetc
+            if (flags.unread_char_buf_in_use) {
+                flags.unread_char_buf_in_use = false;
+                posdiff--;
+            }
+#endif
+#ifdef CYGSEM_LIBC_STDIO_WANT_BUFFERED_IO
+            if (posdiff>0 && flags.buffering) {
+                io_buf.set_bytes_read(posdiff);
+                posdiff=0;
+            } else 
+#endif
+            if (posdiff>0 && flags.readbuf_char_in_use) {
+                flags.readbuf_char_in_use = false;
+                posdiff--;
+            }
+            CYG_ASSERT(posdiff==0, "Failed to seek within buffer correctly");
+
+            position = abspos;
+            unlock_me();
+            return ENOERR;
+        }
+    }
+
+    Cyg_ErrNo err;
+    off_t newpos = pos;
     err = cyg_stdio_lseek( my_device, &newpos, whence );
 
     if( err == ENOERR )
     {
         // Clean out the buffer. Flush output if any present,
@@ -420,14 +448,14 @@ Cyg_StdioStream::set_position( fpos_t po
         flags.unread_char_buf_in_use = false;
 #endif
 
         // Clear EOF indicator.
         flags.at_eof = false;
-    }
-    
-    if( err == ENOERR )
+        
+        // update stream pos
         position = newpos;
+    }
     
     unlock_me();
 
     return err;
 #endif    

Re: [ECOS] [eCos] a question about ROMFS

[jjtsai]

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

----- Original Message -----
From: Jonathan Larmour <jlarmour@redhat.com>
To: jjtsai <jjtsai@itri.org.tw>
Cc: <ecos-discuss@sources.redhat.com>
Sent: Wednesday, July 18, 2001 4:09 AM
Subject: Re: [ECOS] [eCos] a question about ROMFS

> jjtsai wrote:
> > > I'll see if I can work on a better patch.
> > That would be great!
>
> Can you try the attached patch for me please? I haven't even tried
> compiling it yet nevermind testing it, but I was hoping you could do that
> instead :-). Let me know how it goes. Then I'll check it in.
 Your attached patch still has problems. Test program (fseek.c) is attached.

 I do a little modification on the stream.inl (based on your patch) and
romfs.c.
 It works well in my test program. Patch files are attached.

Have a try please. I am afraid if I make any mistakes.

Instruction about how to apply the patch files:

 cd $(ECOS_REPOSITORY)/packages/language/c/libc/stdio/current/include
patch -p0 < $(WHERE_THE_PATCH_IS)/stream_inl.pat
(This is the patch to the original file.)

 cd $(ECOS_REPOSITORY)/packages/fs/rom/current/src
patch -p0 < $(WHERE_THE_PATCH_IS)/romfs_c.pat

best regards,
JJ



[-- Attachment #2: fseek.c --]
[-- Type: text/x-c, Size: 1773 bytes --]

#include <unistd.h>
#include <fcntl.h>
#include <stdio.h>
#include <cyg/fileio/fileio.h>
#include <pkgconf/fs_rom.h>	// Address of ROMFS

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

MTAB_ENTRY( romfs_mte1,
                   "/",
                   "romfs",
                   "",
                   (CYG_ADDRWORD) CYGNUM_FS_ROM_BASE_ADDRESS );

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

int main( int argc, char **argv )
{
    FILE *fd;
    int err;
    int i;
    int length;
    char s[1000];
    struct stat sbuf;
    
    stat("/HelloWorld.jar", &sbuf);

    printf("leng=%d\n",sbuf.st_size);  

    if( (fd=fopen("/HelloWorld.jar","rw")) == NULL)
	printf("fopen error\n");
    else {
	length=0;

	do{
	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 0;

	if( (err=fseek(fd, 195, SEEK_SET))<0)
		printf("fseek=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 195

	if( (err=fread((s+length),sizeof(char), 30, fd))!=30) 
		printf("fread=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 225

	if( (err=fseek(fd, 1, SEEK_CUR))<0)
		printf("fseek=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 226

	if( (err=fread((s+length),sizeof(char), 30, fd))!=30) 
		printf("fread=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 256

	if( (err=fseek(fd, 400, SEEK_CUR))<0)
		printf("fseek=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 656

	if( (err=fread((s+length),sizeof(char), 30, fd))!=30) 
		printf("fread=%d\n",err);

	err=ftell(fd); printf("ftell=%d\n",err); //ftell should return 686
	}
	while(0);

	fclose(fd);
	}

    return 0;

}


[-- Attachment #3: romfs_c.pat --]
[-- Type: text/x-diff, Size: 518 bytes --]

--- romfs.c	Wed Jul 18 14:50:23 2001
+++ romfs.c.new	Wed Jul 18 14:28:15 2001
@@ -912,10 +912,16 @@
     {
     case SEEK_SET:
         // Pos is already where we want to be.
+	fp->f_offset= *apos; //add jjt
         break;
 
     case SEEK_CUR:
         // Add pos to current offset.
+	// comment jjt
+	// the following line should NEVER be executed
+	// because it will cause the inconsistency between
+	// fp->f_offset and real_stream.position. 
+	// end comment jjt
         pos += fp->f_offset;
         break;
 

[-- Attachment #4: stream_inl.pat --]
[-- Type: text/x-diff, Size: 2666 bytes --]

--- stream.inl	Wed Jul 18 14:45:00 2001
+++ stream.inl.new	Wed Jul 18 14:44:09 2001
@@ -363,6 +363,8 @@
 inline Cyg_ErrNo
 Cyg_StdioStream::set_position( fpos_t pos, int whence )
 {
+    CYG_ASSERTCLASS( this, "Stream object is not a valid stream!" );
+    
 #ifndef CYGPKG_LIBC_STDIO_FILEIO    
     // this is currently a workaround until we have real files
     // this will be corrected when we decide the true filesystem interface
@@ -370,8 +372,6 @@
     Cyg_ErrNo err;
     cyg_uint8 c;
 
-    CYG_ASSERTCLASS( this, "Stream object is not a valid stream!" );
-    
     if ((whence != SEEK_CUR) || pos < 0)
         return ENOSYS;
 
@@ -398,15 +398,54 @@
     
 #else
 
-    Cyg_ErrNo err;
-    off_t newpos = pos;
-
-    CYG_ASSERTCLASS( this, "Stream object is not a valid stream!" );
-    
     if (!lock_me())
         return EBADF; // assume file is now invalid
 
-    err = cyg_stdio_lseek( my_device, &newpos, whence );
+    if ( whence != SEEK_END ) {
+        cyg_ucount32 bytesavail = bytes_available_to_read();
+        off_t abspos = (whence == SEEK_CUR) ? position + pos : pos;
+        off_t posdiff = abspos - position;
+    
+        if ( bytesavail > posdiff ) {
+            // can just "seek" within the existing buffer
+#ifdef CYGFUN_LIBC_STDIO_ungetc
+            if (flags.unread_char_buf_in_use) {
+                flags.unread_char_buf_in_use = false;
+                posdiff--;
+            }
+#endif
+#ifdef CYGSEM_LIBC_STDIO_WANT_BUFFERED_IO
+            if (posdiff>0 && flags.buffering) {
+                io_buf.set_bytes_read(posdiff);
+                posdiff=0;
+            } else 
+#endif
+            if (posdiff>0 && flags.readbuf_char_in_use) {
+                flags.readbuf_char_in_use = false;
+                posdiff--;
+            }
+            CYG_ASSERT(posdiff==0, "Failed to seek within buffer correctly");
+
+            position = abspos;
+            unlock_me();
+            return ENOERR;
+        } // endif (bytesavail > posdiff)
+    } //endif (whence != SEEK_END)
+
+    Cyg_ErrNo err;
+    off_t newpos=pos;
+
+    //add jjt
+    if( whence == SEEK_CUR ){
+        newpos = position + pos;
+        err = cyg_stdio_lseek( my_device, &newpos, SEEK_SET);
+        }
+    else {
+        err = cyg_stdio_lseek( my_device, &newpos, whence );
+        }
+    //end jjt
+    //err = cyg_stdio_lseek( my_device, &newpos, whence ); //del jjt
+
 
     if( err == ENOERR )
     {
@@ -422,10 +461,10 @@
 
         // Clear EOF indicator.
         flags.at_eof = false;
-    }
-    
-    if( err == ENOERR )
+        
+        // update stream pos
         position = newpos;
+    }
     
     unlock_me();
 

Re: [ECOS] [eCos] a question about ROMFS

[Jonathan Larmour]

jjtsai wrote:
> 
> ----- Original Message -----
> From: Jonathan Larmour <jlarmour@redhat.com>
> To: jjtsai <jjtsai@itri.org.tw>
> Cc: <ecos-discuss@sources.redhat.com>
> Sent: Wednesday, July 18, 2001 4:09 AM
> Subject: Re: [ECOS] [eCos] a question about ROMFS
> 
> > jjtsai wrote:
> > > > I'll see if I can work on a better patch.
> > > That would be great!
> >
> > Can you try the attached patch for me please? I haven't even tried
> > compiling it yet nevermind testing it, but I was hoping you could do that
> > instead :-). Let me know how it goes. Then I'll check it in.
>  Your attached patch still has problems. Test program (fseek.c) is attached.
> 
>  I do a little modification on the stream.inl (based on your patch) and
> romfs.c.
>  It works well in my test program. Patch files are attached.
> 
> Have a try please. I am afraid if I make any mistakes.
> 
> Instruction about how to apply the patch files:
> 
>  cd $(ECOS_REPOSITORY)/packages/language/c/libc/stdio/current/include
> patch -p0 < $(WHERE_THE_PATCH_IS)/stream_inl.pat
> (This is the patch to the original file.)

Oops, I was so caught up in trying to seek within the buffer, I forgot
about the original problem :). I'm going to update anon CVS with this later
today. You can try it from there to make sure it's okay. I made a slight
variation on your change, just for efficiency and code size.
 
>  cd $(ECOS_REPOSITORY)/packages/fs/rom/current/src
> patch -p0 < $(WHERE_THE_PATCH_IS)/romfs_c.pat

I don't get this one. Your change doesn't appear to do anything because
further in dev_fo_lseek there is:

    // All OK, set fp offset and return new position.
    *apos = fp->f_offset = pos;

And your comment for the SEEK_CUR case: are you saying that pos shouldn't
be changed? If so, can you explain in more detail why?

Jifl
-- 
Red Hat, Rustat House, Clifton Road, Cambridge, UK. Tel: +44 (1223) 271062
Maybe this world is another planet's Hell -Aldous Huxley || Opinions==mine
Come to the Red Hat TechWorld open source conference in Brussels!
Keynotes, techie talks and exhibitions    http://www.redhat-techworld.com/