[RFC 0/3] current read implementation and POSIX

[RFC 0/3] current read implementation and POSIX

[Alexander Aring]

Hi all,

first I want to say "Sascha, please don't apply this patch serie!". I suppose I
detected some incompatible POSIX read call behaviour.

History:

I am thinking about to add a procfs into barebox and I begun to hack this while
I sitting in the train. I don't want to talk about the procfs idea right now,
maybe we don't need something like this. But while I hacked it I detected some
misbehaviour of read(2).

Issue:

read returns the number of bytes which are readed if there are no bytes it should
return 0;

But if the file is zero read will return nothing, this is fixed in Patch 2/3 (please
review carefully, I did this fix fast and check only if it working)

Some commands/functions check if the file is zero and then these functions calls no read
but this is wrong. When a file is zero, read can read some bytes of course. A zero file
not indicate that the file has no content. Maybe I should reference the ugly POSIX opengroup
documentation, but you can see it yourself in linux:

ls -l /proc/version:
-r--r--r-- 1 root root 0 Feb 28 08:30 /proc/version

is a zero file and contains information with "cat /proc/version".

Patch 3/3 fix this issue for commands like edit which use the read_full function which
should read contents with the return value of read and not with size value.


To test the behaviour I advice you this steps:

1. Apply patch 1/3 which adds a simple ugly hacked foofs.

2. Compile a sandbox and start it.

3. in barebox run: "mkdir /bar;mount none foofs /bar"

4. run "cat /bar/foobar" and "edit /bar/foobar" -> nothing will be displayed

5. Apply patch 2/3 and 3/3 and rerun steps 2-4



So why I don't fix this issue? Because I can't test everything and I think some filesystem
and other commands can't deal with it and maybe we don't want to fix this issue and accept the
current behaviour. :-(

Another hint:

Sascha had some problems with zero files in tftpfs maybe there is some relationship between
this issue and the zero file tftpfs files... because the read of zero files is currently some
kind of broken. :-)

- Alex

Alexander Aring (3):
  ramfs: add foofs for testing
  fs: read: handle zero files
  libbb: read_full: use read return instead size

 fs/fs.c     |  3 ---
 fs/ramfs.c  | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/libbb.c |  2 +-
 3 files changed, 70 insertions(+), 4 deletions(-)

-- 
1.9.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[RFC 1/3] ramfs: add foofs for testing

[Alexander Aring]

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 fs/ramfs.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/fs/ramfs.c b/fs/ramfs.c
index f45a454..4b93c2e 100644
--- a/fs/ramfs.c
+++ b/fs/ramfs.c
@@ -608,6 +608,48 @@ static int ramfs_probe(struct device_d *dev)
 	return 0;
 }
 
+static int foofs_read(struct device_d *_dev, FILE *f, void *buf, size_t insize)
+{
+	if (f->pos == strlen("Hello World!\n"))
+		return 0;
+
+	return sprintf(buf, "%s", "Hello World!\n");
+}
+
+static int foofs_probe(struct device_d *dev)
+{
+	struct ramfs_inode *n;
+	struct ramfs_priv *priv = xzalloc(sizeof(struct ramfs_priv));
+
+	dev->priv = priv;
+
+	priv->root.name = "/";
+	priv->root.mode = S_IFDIR | S_IRWXU | S_IRWXG | S_IRWXO;
+	priv->root.parent = &priv->root;
+	n = ramfs_get_inode();
+	n->name = strdup(".");
+	n->mode = S_IFDIR;
+	n->parent = &priv->root;
+	n->child = n;
+	priv->root.child = n;
+	n = ramfs_get_inode();
+	n->name = strdup("..");
+	n->mode = S_IFDIR | S_IRWXU | S_IRWXG | S_IRWXO;
+	n->parent = &priv->root;
+	n->child = priv->root.child;
+	priv->root.child->next = n;
+
+	/* just for test zero file read */
+	n = ramfs_get_inode();
+	n->name = strdup("foobar");
+	n->mode = S_IFREG | S_IRWXU | S_IRWXG | S_IRWXO;
+	n->parent = &priv->root;
+	n->child = priv->root.child;
+	priv->root.child->next = n;
+
+	return 0;
+}
+
 static void ramfs_remove(struct device_d *dev)
 {
 	free(dev->priv);
@@ -638,8 +680,35 @@ static struct fs_driver_d ramfs_driver = {
 	}
 };
 
+static struct fs_driver_d foofs_driver = {
+	.create    = ramfs_create,
+	.unlink    = ramfs_unlink,
+	.open      = ramfs_open,
+	.close     = ramfs_close,
+	.truncate  = ramfs_truncate,
+	.read      = foofs_read,
+	.write     = ramfs_write,
+	.lseek     = ramfs_lseek,
+	.mkdir     = ramfs_mkdir,
+	.rmdir     = ramfs_rmdir,
+	.opendir   = ramfs_opendir,
+	.readdir   = ramfs_readdir,
+	.closedir  = ramfs_closedir,
+	.stat      = ramfs_stat,
+	.symlink   = ramfs_symlink,
+	.readlink  = ramfs_readlink,
+	.flags     = FS_DRIVER_NO_DEV,
+	.drv = {
+		.probe  = foofs_probe,
+		.remove = ramfs_remove,
+		.name = "foofs",
+	}
+};
+
+
 static int ramfs_init(void)
 {
+	register_fs_driver(&foofs_driver);
 	return register_fs_driver(&ramfs_driver);
 }
 
-- 
1.9.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[RFC 2/3] fs: read: handle zero files

[Alexander Aring]

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 fs/fs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/fs.c b/fs/fs.c
index 1b43c61..9584495 100644
--- a/fs/fs.c
+++ b/fs/fs.c
@@ -804,9 +804,6 @@ static ssize_t __read(FILE *f, void *buf, size_t count)
 	if (f->size != FILE_SIZE_STREAM && f->pos + count > f->size)
 		count = f->size - f->pos;
 
-	if (!count)
-		return 0;
-
 	ret = fsdrv->read(dev, f, buf, count);
 
 	if (ret < 0)
-- 
1.9.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[RFC 3/3] libbb: read_full: use read return instead size

[Alexander Aring]

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 lib/libbb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/libbb.c b/lib/libbb.c
index 189a170..c8d0835 100644
--- a/lib/libbb.c
+++ b/lib/libbb.c
@@ -162,7 +162,7 @@ int read_full(int fd, void *buf, size_t size)
 	int now;
 	int total = 0;
 
-	while (size) {
+	while (now) {
 		now = read(fd, buf, size);
 		if (now == 0)
 			return total;
-- 
1.9.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 1/3] ramfs: add foofs for testing

[Alexander Aring]

On Fri, Feb 28, 2014 at 08:44:26AM +0100, Alexander Aring wrote:
> +
> +	/* just for test zero file read */
> +	n = ramfs_get_inode();
> +	n->name = strdup("foobar");
> +	n->mode = S_IFREG | S_IRWXU | S_IRWXG | S_IRWXO;
> +	n->parent = &priv->root;
> +	n->child = priv->root.child;
> +	priv->root.child->next = n;

oops, I overwrite the ".." here, it should be
priv->root.child->next->next = n

sry, but you can test it anyway with this. :-)

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 3/3] libbb: read_full: use read return instead size

[Alexander Aring]

On Fri, Feb 28, 2014 at 08:44:28AM +0100, Alexander Aring wrote:
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>  lib/libbb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/libbb.c b/lib/libbb.c
> index 189a170..c8d0835 100644
> --- a/lib/libbb.c
> +++ b/lib/libbb.c
> @@ -162,7 +162,7 @@ int read_full(int fd, void *buf, size_t size)
>  	int now;
>  	int total = 0;
>  
> -	while (size) {
> +	while (now) {
>  		now = read(fd, buf, size);
>  		if (now == 0)
>  			return total;
and this should be a:

do {
...
} while (now);

sry, it's only to demonstrate the issue.

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 3/3] libbb: read_full: use read return instead size

[Sascha Hauer]

On Fri, Feb 28, 2014 at 09:03:34AM +0100, Alexander Aring wrote:
> On Fri, Feb 28, 2014 at 08:44:28AM +0100, Alexander Aring wrote:
> > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > ---
> >  lib/libbb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/libbb.c b/lib/libbb.c
> > index 189a170..c8d0835 100644
> > --- a/lib/libbb.c
> > +++ b/lib/libbb.c
> > @@ -162,7 +162,7 @@ int read_full(int fd, void *buf, size_t size)
> >  	int now;
> >  	int total = 0;
> >  
> > -	while (size) {
> > +	while (now) {
> >  		now = read(fd, buf, size);
> >  		if (now == 0)
> >  			return total;
> and this should be a:
> 
> do {
> ...
> } while (now);
> 
> sry, it's only to demonstrate the issue.

'now' will never be 0 at the end of the loop, so you could equally well
write while(1). With this change we try to read as long as we read
something last time, even if there's nothing left to read (size is 0).
What issue do you see with this function?

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 3/3] libbb: read_full: use read return instead size

[Alexander Aring]

Hi Sascha,

On Fri, Feb 28, 2014 at 03:21:18PM +0100, Sascha Hauer wrote:
> On Fri, Feb 28, 2014 at 09:03:34AM +0100, Alexander Aring wrote:
> > On Fri, Feb 28, 2014 at 08:44:28AM +0100, Alexander Aring wrote:
> > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > ---
> > >  lib/libbb.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/libbb.c b/lib/libbb.c
> > > index 189a170..c8d0835 100644
> > > --- a/lib/libbb.c
> > > +++ b/lib/libbb.c
> > > @@ -162,7 +162,7 @@ int read_full(int fd, void *buf, size_t size)
> > >  	int now;
> > >  	int total = 0;
> > >  
> > > -	while (size) {
> > > +	while (now) {
> > >  		now = read(fd, buf, size);
> > >  		if (now == 0)
> > >  			return total;
> > and this should be a:
> > 
> > do {
> > ...
> > } while (now);
> > 
> > sry, it's only to demonstrate the issue.
> 
> 'now' will never be 0 at the end of the loop, so you could equally well
> write while(1). With this change we try to read as long as we read
yes, I did it quickly to make something that command "edit" works in
some way. The whole patches is only to demonstrate the issue. Also the
foofs demo works if we read the whole thing at once, but that's okay for
the test.


> something last time, even if there's nothing left to read (size is 0).
> What issue do you see with this function?

The function read_full makes similar things like:

stat(fd, &statbuf);
...
read(fd, buf, statbuf.st_size);

this is wrong because a filesize can be zero and read can read something
from this file.

For example procfs in linux, if you run this under linux for /proc/version
it will do nothing.

a "while (size)" don't call read because at first we checked if the file
is zero, which don't indicate that you can read something from a file.


So we have many function which works like this (I suppose). But there is
something wrong with the whole read function because we can't read
something from a file, when the filesize is zero. Patch 2/3 changes this.

Sascha, do you understand what I mean... in some way? :-/

I know it sounds a little bit crazy that read can read more than zero
bytes if the filesize is zero.

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 3/3] libbb: read_full: use read return instead size

[Alexander Aring]

On Fri, Feb 28, 2014 at 06:12:07PM +0100, Alexander Aring wrote:
> Hi Sascha,
> 
> On Fri, Feb 28, 2014 at 03:21:18PM +0100, Sascha Hauer wrote:
> > On Fri, Feb 28, 2014 at 09:03:34AM +0100, Alexander Aring wrote:
> > > On Fri, Feb 28, 2014 at 08:44:28AM +0100, Alexander Aring wrote:
> > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > ---
> > > >  lib/libbb.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/libbb.c b/lib/libbb.c
> > > > index 189a170..c8d0835 100644
> > > > --- a/lib/libbb.c
> > > > +++ b/lib/libbb.c
> > > > @@ -162,7 +162,7 @@ int read_full(int fd, void *buf, size_t size)
> > > >  	int now;
> > > >  	int total = 0;
> > > >  
> > > > -	while (size) {
> > > > +	while (now) {
> > > >  		now = read(fd, buf, size);
> > > >  		if (now == 0)
> > > >  			return total;
> > > and this should be a:
> > > 
> > > do {
> > > ...
> > > } while (now);
> > > 
> > > sry, it's only to demonstrate the issue.
> > 
> > 'now' will never be 0 at the end of the loop, so you could equally well
> > write while(1). With this change we try to read as long as we read
> yes, I did it quickly to make something that command "edit" works in
> some way. The whole patches is only to demonstrate the issue. Also the
> foofs demo works if we read the whole thing at once, but that's okay for
> the test.
> 
> 
> > something last time, even if there's nothing left to read (size is 0).
> > What issue do you see with this function?
> 
> The function read_full makes similar things like:
> 
> stat(fd, &statbuf);
> ...
> read(fd, buf, statbuf.st_size);
> 
> this is wrong because a filesize can be zero and read can read something
> from this file.
> 
> For example procfs in linux, if you run this under linux for /proc/version
> it will do nothing.
> 
> a "while (size)" don't call read because at first we checked if the file
> is zero, which don't indicate that you can read something from a file.
> 
I meant s/can read/can't read/

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 3/3] libbb: read_full: use read return instead size

[Sascha Hauer]

On Fri, Feb 28, 2014 at 06:12:09PM +0100, Alexander Aring wrote:
> Hi Sascha,
> 
> On Fri, Feb 28, 2014 at 03:21:18PM +0100, Sascha Hauer wrote:
> > On Fri, Feb 28, 2014 at 09:03:34AM +0100, Alexander Aring wrote:
> > > On Fri, Feb 28, 2014 at 08:44:28AM +0100, Alexander Aring wrote:
> > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > ---
> > > >  lib/libbb.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/libbb.c b/lib/libbb.c
> > > > index 189a170..c8d0835 100644
> > > > --- a/lib/libbb.c
> > > > +++ b/lib/libbb.c
> > > > @@ -162,7 +162,7 @@ int read_full(int fd, void *buf, size_t size)
> > > >  	int now;
> > > >  	int total = 0;
> > > >  
> > > > -	while (size) {
> > > > +	while (now) {
> > > >  		now = read(fd, buf, size);
> > > >  		if (now == 0)
> > > >  			return total;
> > > and this should be a:
> > > 
> > > do {
> > > ...
> > > } while (now);
> > > 
> > > sry, it's only to demonstrate the issue.
> > 
> > 'now' will never be 0 at the end of the loop, so you could equally well
> > write while(1). With this change we try to read as long as we read
> yes, I did it quickly to make something that command "edit" works in
> some way. The whole patches is only to demonstrate the issue. Also the
> foofs demo works if we read the whole thing at once, but that's okay for
> the test.
> 
> 
> > something last time, even if there's nothing left to read (size is 0).
> > What issue do you see with this function?
> 
> The function read_full makes similar things like:

s/read_full/read_file/

> 
> stat(fd, &statbuf);
> ...
> read(fd, buf, statbuf.st_size);
> 
> this is wrong because a filesize can be zero and read can read something
> from this file.
> 
> For example procfs in linux, if you run this under linux for /proc/version
> it will do nothing.

So our read_file implementation doesn't work procfs like filesystems where
all sizes are 0. Your patch doesn't change this though. In 2/3 you
remove the if (!size) check and call the fs drivers read function with
size 0. In this case the read function may return values, but never
actually read something because the buffer size is 0.

Sascha

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 1/3] ramfs: add foofs for testing

[Sascha Hauer]

On Fri, Feb 28, 2014 at 08:44:26AM +0100, Alexander Aring wrote:
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>  fs/ramfs.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/fs/ramfs.c b/fs/ramfs.c
> index f45a454..4b93c2e 100644
> --- a/fs/ramfs.c
> +++ b/fs/ramfs.c
> @@ -608,6 +608,48 @@ static int ramfs_probe(struct device_d *dev)
>  	return 0;
>  }
>  
> +static int foofs_read(struct device_d *_dev, FILE *f, void *buf, size_t insize)
> +{
> +	if (f->pos == strlen("Hello World!\n"))
> +		return 0;
> +
> +	return sprintf(buf, "%s", "Hello World!\n");
> +}

You should never read more bytes than insize. This is also true for
insize == 0. Implement this correctly and you'll see that your patches
do not solve the problem.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 3/3] libbb: read_full: use read return instead size

[Alexander Aring]

Hi Sascha,

On Mon, Mar 03, 2014 at 09:30:50AM +0100, Sascha Hauer wrote:
> 
> So our read_file implementation doesn't work procfs like filesystems where
> all sizes are 0. Your patch doesn't change this though. In 2/3 you
> remove the if (!size) check and call the fs drivers read function with
> size 0. In this case the read function may return values, but never
> actually read something because the buffer size is 0.
> 
exactly. The imporant question for me is "do you want to accept patches
to changes this behaviour, so we can read zero file size files". I mean
this would be a huge change in internal api of filesystem layer and I
hope we doesn't break anything else.

But your are fine with me to say "there exists a problem with read of
zero sized files?".

I will create some "better" patches for this, this RFC should only answer
the question if you apply fixes or not.

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [RFC 1/3] ramfs: add foofs for testing

[Alexander Aring]

Hi Sascha,

On Mon, Mar 03, 2014 at 09:36:20AM +0100, Sascha Hauer wrote:
> On Fri, Feb 28, 2014 at 08:44:26AM +0100, Alexander Aring wrote:
> > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > ---
> >  fs/ramfs.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> > 
> > diff --git a/fs/ramfs.c b/fs/ramfs.c
> > index f45a454..4b93c2e 100644
> > --- a/fs/ramfs.c
> > +++ b/fs/ramfs.c
> > @@ -608,6 +608,48 @@ static int ramfs_probe(struct device_d *dev)
> >  	return 0;
> >  }
> >  
> > +static int foofs_read(struct device_d *_dev, FILE *f, void *buf, size_t insize)
> > +{
> > +	if (f->pos == strlen("Hello World!\n"))
> > +		return 0;
> > +
> > +	return sprintf(buf, "%s", "Hello World!\n");
> > +}
> 
> You should never read more bytes than insize. This is also true for
> insize == 0. Implement this correctly and you'll see that your patches
> do not solve the problem.
> 
yes it's not correctly implemented. Maybe I implement a "testfs"
filesystem for barebox with unit-tests which can be used to test the
filesystem layer to see if something broken or not. This fs should be
based on ramfs.

The fs tests be placed into commands -> testing and it's only interesting for
some developers.

- Alex

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox