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