[PATCH] libfs/jffs2: Force the inode size to a non-zero value if a directory.

Chris Johns chrisj at rtems.org
Wed Mar 14 23:07:15 UTC 2018


On 14/03/2018 17:53, Sebastian Huber wrote:
> 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.
> 

I was not planning on this fix for 5 just 4.11 and I do not think we will be
changing newlib on that release branch.

>>
>>> 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().
> 

Thank, that makes sense, I have not looked into it.

>>
>> 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:

Because for a directory it is not being used. If it was the 0 value would mean
the directory has no valid data to read.

> 
> 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.
> 

Sure, if it what you would like I will send a v2 patch.

Chris



More information about the devel mailing list