[PATCH] libfs/jffs2: Force the inode size to a non-zero value if a directory.
Sebastian Huber
sebastian.huber at embedded-brains.de
Wed Mar 14 06:53:27 UTC 2018
On 13/03/18 22:45, Chris Johns wrote:
> On 13/03/2018 17:52, Sebastian Huber wrote:
>> On 13/03/18 07:50, Sebastian Huber wrote:
>>> On 13/03/18 07:47, Chris Johns wrote:
>>>> On 13/03/2018 17:18, Sebastian Huber wrote:
>>>>> On 13/03/18 00:49, Chris Johns wrote:
>>>>>> Set the inode size to 256 to work around a newlib scandir check where a
>>>>>> directory has to have a non-zero size to work. Set the size to greater than
>>>>>> 24 bytes, a small dirent size so the allocator in scandir works.
>>>>>>
>>>>>> The newlib scandir code should be updated to a more recent scandir from
>>>>>> FreeBSD where these checks have been removed. This change is a work
>>>>>> around to avoid new tools on the release branch.
>>>>>>
>>>>>> With this change scandir works on IMFS, RFS and JFFS2.
>>>>> I cannot find a scandir() test in the fstests for all file systems.
>>>>>
>>>>>> Closes #3332
>>>>>> ---
>>>>>> cpukit/libfs/src/jffs2/src/fs-rtems.c | 3 +++
>>>>>> 1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/cpukit/libfs/src/jffs2/src/fs-rtems.c
>>>>>> b/cpukit/libfs/src/jffs2/src/fs-rtems.c
>>>>>> index ce84d44470..790929d856 100644
>>>>>> --- a/cpukit/libfs/src/jffs2/src/fs-rtems.c
>>>>>> +++ b/cpukit/libfs/src/jffs2/src/fs-rtems.c
>>>>>> @@ -1512,6 +1512,9 @@ static int jffs2_read_inode (struct _inode *inode)
>>>>>> inode->i_mtime = je32_to_cpu(latest_node.mtime);
>>>>>> inode->i_ctime = je32_to_cpu(latest_node.ctime);
>>>>>> + if (S_ISDIR(inode->i_mode))
>>>>>> + inode->i_size = 256;
>>>>>> +
>>>>>> inode->i_nlink = f->inocache->pino_nlink;
>>>>>> mutex_unlock(&f->sem);
>>>>> This code is from the original JFFS2 support for eCos. Which side-effects has
>>>>> this i_size change for directories? Why can't you place this hack in
>>>>> rtems_jffs2_fstat()? This would reduce the impact area of this change.
>>>>>
>>>> I did not know the history of the RTEMS wrapper. I just assumed all code in that
>>>> file is specific to RTEMS. The change I have makes things consistent. I still
>>>> think it is OK?
>>> What do you mean with consistent?
> The function being patched is internal to `fs-rtems.c` which is a wrapper for
> JFFS2. The consistent comment I made referred to any reference to the inode had
> the same value exported to RTEMS, ie consistent.
>
> The code may have come from ecos and if ecos is using newlib with scandir they
> have the same problem. I do not consider ecos as being upstream, rather this
> code has been taken 'as was' (?).
>
>> I thought this is a hack to make the Newlib scandir() happy?
> The issue is clearly in scandir. FreeBSD these days and glibc do not stat the
> directory for it's size, OpenSolaris still does. The proper fix is to scandir.
>
> Fixing scandir requires a newlib patch and that means an update to the RSB. If
> this is preferred please say, it however is a lot more work so I am reluctant to
> do this, I do not have the time.
>
> I have coded around the scandir issue with opendir and readdir and I suspect
> there are a number of cases of this happening. I have seen at least one other
> case and wondered at the time why opendir etc was being used instead of scandir.
Then we should open a new ticket for the scandir() problem in Newlib and
reference this ticket in the workaround. Once Newlib is fixed we can
remove the workaround.
>
>> This i_size is used in several places. How do you know that
>>> you didn't accidentally broke something?
> The only thing I can think of is reading a directory as a file and a 0 means do
> not read. That does not seem to be a problem with the patch, I can cat a
> directory and get the same sort of rubbish I get when doing this on any other
> host which implies the directory as a file does have a size. If I remove the
> patch the cat of the same directory gives the same result and that would imply
> something else is wrong or inconsistent so who knows if what we have there is
> correct.
If you read() from a directory, then you get a stream of struct dirent
items, see rtems_jffs2_dir_read().
>
> The size of a directory is file system specific so I would say any code that
> assumes anything is file system specific and fragile and the scandir issue would
> seem to support this. I have built a large application which uses the JFFS2
> heavily for application installing and management with many files and I see no
> issue. I have no other test results to say otherwise.
Ok, I have difficulties to understand from the source code, why the
i_size change has no impact on the internal file system operation:
grep -r '\<i_size\>' cpukit/libfs/
cpukit/libfs/src/jffs2/include/linux/kernel.h:#define
truncate_setsize(x, y) do { (x)->i_size = (y); } while (0)
cpukit/libfs/src/jffs2/src/nodelist.c:/* Doesn't set inode->i_size */
cpukit/libfs/src/jffs2/src/dir-rtems.c: inode->i_size = datalen;
cpukit/libfs/src/jffs2/src/dir-rtems.c: ri->isize = ri->dsize =
ri->csize = cpu_to_je32(inode->i_size);
cpukit/libfs/src/jffs2/src/dir-rtems.c: ri->totlen =
cpu_to_je32(sizeof(*ri) + inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c: (unsigned
int)inode->i_size, offset));
cpukit/libfs/src/jffs2/src/fs-rtems.c: ri->isize =
cpu_to_je32(max((uint32_t)inode->i_size, offset));
cpukit/libfs/src/jffs2/src/fs-rtems.c: ri->offset =
cpu_to_je32(inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c: ri->dsize = cpu_to_je32(offset -
inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c: inode->i_size = offset;
cpukit/libfs/src/jffs2/src/fs-rtems.c: ri->isize = cpu_to_je32((ivalid
& ATTR_SIZE)?iattr->ia_size:inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c: if (ivalid & ATTR_SIZE &&
inode->i_size < iattr->ia_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c: ri->dsize =
cpu_to_je32(iattr->ia_size - inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c: ri->offset =
cpu_to_je32(inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c: if (ivalid & ATTR_SIZE &&
inode->i_size > iattr->ia_size)
cpukit/libfs/src/jffs2/src/fs-rtems.c: if (ivalid & ATTR_SIZE &&
inode->i_size < iattr->ia_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c: inode->i_size =
iattr->ia_size;
cpukit/libfs/src/jffs2/src/fs-rtems.c: We are protected from a
simultaneous write() extending i_size
cpukit/libfs/src/jffs2/src/fs-rtems.c: if (ivalid & ATTR_SIZE &&
inode->i_size > iattr->ia_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c: buf->st_size = inode->i_size;
cpukit/libfs/src/jffs2/src/fs-rtems.c: if (pos >= inode->i_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c: uint32_t max_available =
inode->i_size - pos_32;
cpukit/libfs/src/jffs2/src/fs-rtems.c: pos = inode->i_size;
cpukit/libfs/src/jffs2/src/fs-rtems.c: if (pos > inode->i_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c: ri.isize =
cpu_to_je32(inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c: if (pos > inode->i_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c: inode->i_size = pos;
cpukit/libfs/src/jffs2/src/fs-rtems.c: inode->i_size = 0;
cpukit/libfs/src/jffs2/src/fs-rtems.c: inode->i_size = 0;
cpukit/libfs/src/jffs2/src/fs-rtems.c: inode->i_size =
je32_to_cpu(latest_node.isize);
cpukit/libfs/src/jffs2/src/os-rtems.h:#define JFFS2_F_I_SIZE(f)
(OFNI_EDONI_2SFFJ(f)->i_size)
cpukit/libfs/src/jffs2/src/os-rtems.h: off_t i_size; // For
files only
cpukit/libfs/src/jffs2/src/gc.c: * from i_size since
i_size may have not been updated yet */
cpukit/libfs/src/jffs2/src/gc.c: * from i_size since
i_size may have not been updated yet */
In case
diff --git a/cpukit/libfs/src/jffs2/src/fs-rtems.c
b/cpukit/libfs/src/jffs2/src/fs-rtems.c
index 652a8e7dbe..074c9640ab 100644
--- a/cpukit/libfs/src/jffs2/src/fs-rtems.c
+++ b/cpukit/libfs/src/jffs2/src/fs-rtems.c
@@ -411,7 +411,14 @@ static int rtems_jffs2_fstat(
buf->st_nlink = inode->i_nlink;
buf->st_uid = inode->i_uid;
buf->st_gid = inode->i_gid;
- buf->st_size = inode->i_size;
+
+ if (S_ISDIR(inode->i_mode)) {
+ /* Magic value to make Newlib scandir() happy */
+ buf->st_size = 256;
+ } else {
+ buf->st_size = inode->i_size;
+ }
+
buf->st_atime = inode->i_atime;
buf->st_mtime = inode->i_mtime;
buf->st_ctime = inode->i_ctime;
is sufficient to fix the scandir() problem I would rather fix it like this.
>
>> We have also this comment:
>>
>> struct _inode {
>> cyg_uint32 i_ino;
>>
>> int i_count;
>> mode_t i_mode;
>> nlink_t i_nlink; // Could we dispense with this?
>> uid_t i_uid;
>> gid_t i_gid;
>> time_t i_atime;
>> time_t i_mtime;
>> time_t i_ctime;
>> // union {
>> unsigned short i_rdev; // For devices only
>> struct _inode * i_parent; // For directories only
>> off_t i_size; // For files only
> Is that a requirement or an observation?
>
> It could be related to the implementation and there not being a need to set this
> field for a directory so it never was, ie non-data nodes ...
>
> https://git.rtems.org/rtems/tree/cpukit/libfs/src/jffs2/src/readinode.c#n1207
With your change the comment should be updated/removed.
>
>> // };
>> struct super_block * i_sb;
>> struct jffs2_full_dirent * i_fd;
>>
>> struct jffs2_inode_info jffs2_i;
>>
>> struct _inode * i_cache_prev; // We need doubly-linked?
>> struct _inode * i_cache_next;
>> };
>>
>> Which is no longer valid with this change.
>>
> I do not think so looking at the code and the comment provides no clue.
>
> Chris
--
Sebastian Huber, embedded brains GmbH
Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail : sebastian.huber at embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
More information about the devel
mailing list