[Issue 1000538] add a d_type field to struct dirent in fileio

[Issue 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugzilla.ecoscentric.com/show_bug.cgi?id=1000538


Jonathan Larmour <jifl@ecoscentric.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nickg@ecoscentric.com




--- Comment #5 from Jonathan Larmour <jifl@ecoscentric.com>  2008-04-03 13:52:53 ---
Nick should probably be brought in on this one too.


-- 
Configure issuemail: http://bugzilla.ecoscentric.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the issue.

[Issue 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugzilla.ecoscentric.com/show_bug.cgi?id=1000538





--- Comment #6 from Nick Garnett <nickg@ecoscentric.com>  2008-04-03 14:29:00 ---
My only caveats about this are that this is not POSIX compliant. However,
making it a config option probably solves that.

A more important worry is that many filesystems do not have this information to
hand in directory entries. Most UNIX filesystem, for example, only have name
and inode number. This also applies to the eCos RAM and ROM filesystems. So to
gather this information they would effectively have to do an internal stat()
operation. While it may save effort for some filesystems, it will involve
others in more.

Whether a filesystem can provide the d_type field is a property of the
filesystem, and shouldn't be imposed from the outside. So I think that in
addition to S_IFDIR or S_IFREG, any filesystem should be allowed to set d_type
to zero, meaning "don't know". This needs to be made clear in the CDL option
description, in the definition of struct dirent and in the documentation. 


-- 
Configure issuemail: http://bugzilla.ecoscentric.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the issue.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugzilla.ecoscentric.com/show_bug.cgi?id=1000538


Andrew Lunn <andrew.lunn@ascom.ch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #532 is|0                           |1
           obsolete|                            |
 Attachment #543 is|0                           |1
           obsolete|                            |




--- Comment #12 from Andrew Lunn <andrew.lunn@ascom.ch>  2008-04-21 20:48:36 ---
Created an attachment (id=546)
 --> (http://bugzilla.ecoscentric.com/attachment.cgi?id=546)
Next version of the patch. Might work now?

Testing the last patch i found a couple of problems. The ROMFS test was broken,
missing an include file. Was it tested? 

Once the test case ran, it showed up another problem with mount points. At
least i think it is a problem. The dirent returned by readdir() on a directory
which is a mount point returns the mode of the directory underneath the mount
point. stat() returns the mode of the mounted root directory. Hence you get
inconsistent return values. Both return directory types, but the permission
bits are different.

The Linux man pages for readdir() documents that d_type only returns a few bits
of useful information, basically the type of file. So i restricted the return
value in eCos to also return the file type and nothing more. This results in
consistent  values for readdir() and stat().  

Comments please.


-- 
Configure bugmail: http://bugzilla.ecoscentric.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugzilla.ecoscentric.com/show_bug.cgi?id=1000538





--- Comment #13 from yxinghua <eCos@sunnorth.com.cn>  2008-04-23 09:32:15 ---
(In reply to comment #12)
Sorry, we have tested d_type patch using our application patterns. Our
consideration is not comprehensive.
Appreciate your modification.


-- 
Configure bugmail: http://bugzilla.ecoscentric.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugzilla.ecoscentric.com/show_bug.cgi?id=1000538


Andrew Lunn <andrew.lunn@ascom.ch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |CURRENTRELEASE




--- Comment #14 from Andrew Lunn <andrew.lunn@ascom.ch>  2008-05-01 10:41:44 ---
I just committed the patch. Closing bug.


-- 
Configure bugmail: http://bugzilla.ecoscentric.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538





--- Comment #11 from yxinghua <eCos@sunnorth.com.cn>  2008-04-14 09:27:32 ---
Created an attachment (id=543)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=543)
complemented d_entry patch

This new version patch adds support to fatfs, ramfs, romfs and jffs2fs.
Also testcases of each one is extended to test the new member when it is
present.

One of my college and me discussed the problem and finally comes this patch, I
have corrected 

the errors of jffs2 and applied Andrew's opinion, thanks very much.


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538





--- Comment #10 from Andrew Lunn <andrew.lunn@ascom.ch>  2008-04-09 13:42:38 ---
I don't think your usage of ilookup is correct. That will just look in the
inode cache. The inode may not be in the cache. 

I think you should be using jffs2_iget() and don't forget to call jffs2_iput()
once you are finished with the inode.


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538





--- Comment #9 from yxinghua <eCos@sunnorth.com.cn>  2008-04-09 04:10:41 ---
(In reply to comment #6)
(In reply to comment #8)
Thanks for the new patch.
What I want to discuss here is mainly concerned with the ideas of Nick. One of
my colleague and me discuss for a long time, we both think that maybe the best
method is to add another cdl option, and this cdl option is active if
CYGPKG_FILEIO_DIRENT_DTYPE is enabled. The purpose of using this new cdl option
is to control whether a filesystem's xxx_fo_dirread function should set d_type
to an entry's mode(i.e. a file or a directory) or should set d_type to
zero(means "unknow"), so that the users of the certain filesystem could be able
to balance the cost between return a d_type and calling stat. 
If this thought was correct, then there are two ways to implement this method.
One is to set this cdl option in fileio(suppose to be
CYGPKG_FILEIO_RET_DIRENT_DTYPE), however, it affects all the filesystem. As a
result of fileio's global option CYGPKG_FILEIO_RET_DIRENT_DTYPE, all the
filesystem will need to set d_type to proper mode or set d_type to zero at the
same time. It seems not so good.
The other method is to set the cdl option for each filesystem(i.e. fatfs,
jffs2, ram, rom, suppose to be CYGPKG_FS_XXX_RET_DIRENT_DTYPE), then each
single filesystem could be able to select. If this cdl option
CYGPKG_FS_XXX_RET_DIRENT_DTYPE is enabled, then the filesystem itself tries to
set d_type, otherwise, it sets the d_type to zero and stat function should be
called to get the entry's mode. We both agree that this is a better method but
we are not sure whether we are totally right? 

By the way, Andrew is right, former patch for jffs does have erros, and I will
correct the errors. As follow
#ifdef CYGPKG_FILEIO_DIRENT_DTYPE
        mode_t *ntype = &ent->d_type;
#endif
        ...
#ifdef CYGPKG_FILEIO_DIRENT_DTYPE

        struct _inode *c_ino;
        c_ino = ilookup(inode->i_sb, fd->ino);  // here use ilookup to find the
entry's node structure, is it ok?
        if(NULL != c_ino)
            *ntype = c_ino->i_mode;
        else
            *ntype = 0;
#endif
        filldir(nbuf, nlen, fd->name, strlen((char *)fd->name));
        goto out_sem;
and this has been tested for smdk2410. However, ilookup need to search each
inode, and it costs. But it may argue that cost of comparing inode is less than
comparing string which is used by stat, right? We want to do the same thing to
ram & rom filesystem but we havent't started yet.

Then add the cdl option described as above.
        ...
#ifdef CYGPKG_FILEIO_DIRENT_DTYPE
#ifdef CYGPKG_FS_XXX_RET_DIRENT_DTYPE
        //*ntype = (mode_t)fd->type;
        struct _inode *c_ino;
        c_ino = ilookup(inode->i_sb, fd->ino);  // here use ilookup to find the
entry's node structure
        if(NULL != c_ino)
            *ntype = c_ino->i_mode;
        else
            *ntype = 0;
#else
        *ntype = 0;
#endif
#endif
        filldir(nbuf, nlen, fd->name, strlen((char *)fd->name));
        goto out_sem;

Should we submit another patch?


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538


Andrew Lunn <andrew.lunn@ascom.ch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #526 is|0                           |1
           obsolete|                            |




--- Comment #8 from Andrew Lunn <andrew.lunn@ascom.ch>  2008-04-07 10:36:51 ---
Created an attachment (id=532)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=532)
d_entry patch

Here is a new version of the patch. 

It adds documentation and extra comments about this entry being none portable
and not being part of POSIX.

It also extends the fatfs1 testcase to test the new member when it is prescent.

The patch also removes the jffs2 part which the more i look at it, the more i
think it is wrong. Before that part of the original patch is restored the
changes made in the fatfs1 testcase need to be ported to the jffs2 test case to
show it really does work.


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538





--- Comment #7 from yxinghua <eCos@sunnorth.com.cn>  2008-04-05 17:03:47 ---
Still some work to do. I will see into this. Thanks for all your opinions.


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538


Andrew Lunn <andrew.lunn@ascom.ch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1




--- Comment #4 from Andrew Lunn <andrew.lunn@ascom.ch>  2008-04-03 07:59:57 ---
Did you test the jffs2 part of the patch. On examination it looks wrong. Isn't
inode the directory file you are reading from, not the inode of the file in the
directory you are about to return?

I could be wrong here, i've not tested it yet....


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538





--- Comment #3 from yxinghua <eCos@sunnorth.com.cn>  2008-04-03 02:27:05 ---
(In reply to comment #1)
> Is there supposed to be a patch attached to this bug? 

yes, I have attached the patch when I create this bug, however, it is not
successful because of the special network of mine. Sorry for that.

Now I attach it again.


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538





--- Comment #2 from yxinghua <eCos@sunnorth.com.cn>  2008-04-03 02:20:34 ---
Created an attachment (id=526)
 --> (http://bugs.ecos.sourceware.org/attachment.cgi?id=526)
add a d_type field to struct dirent in fileio


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.

[Bug 1000538] add a d_type field to struct dirent in fileio

[bugzilla-daemon]

http://bugs.ecos.sourceware.org/show_bug.cgi?id=1000538


Andrew Lunn <andrew.lunn@ascom.ch> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrew.lunn@ascom.ch




--- Comment #1 from Andrew Lunn <andrew.lunn@ascom.ch>  2008-04-02 13:56:27 ---
Is there supposed to be a patch attached to this bug? 


-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.