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

2018-03-14 Thread Chris Johns
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(>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 

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

2018-03-14 Thread Sebastian Huber

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(>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 '\' 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));

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

2018-03-13 Thread Chris Johns
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(>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.

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

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.

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

> //    };
>     struct super_block *    i_sb;
>     struct jffs2_full_dirent * i_fd;
> 
>     struct jffs2_inode_info  

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

2018-03-13 Thread Sebastian Huber

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(>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? I thought this is a hack to make the 
Newlib scandir() happy? This i_size is used in several places. How do 
you know that you didn't accidentally broke something?




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
//    };
    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.

--
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.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2018-03-13 Thread Sebastian Huber

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(>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? I thought this is a hack to make the 
Newlib scandir() happy? This i_size is used in several places. How do 
you know that you didn't accidentally broke something?


--
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.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2018-03-13 Thread Sebastian Huber

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


--
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.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel