Re: [PATCH 01/18] fs: add readlink support

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

On 15:25 Mon 27 Aug     , Sascha Hauer wrote:
> On Fri, Aug 24, 2012 at 06:50:01AM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  fs/fs.c      |   59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/fs.h |    5 +++++
> >  2 files changed, 64 insertions(+)
> > 
> > diff --git a/fs/fs.c b/fs/fs.c
> > index 8ab1a3a..64a0d94 100644
> > --- a/fs/fs.c
> > +++ b/fs/fs.c
> > @@ -890,6 +890,65 @@ int close(int fd)
> >  }
> >  EXPORT_SYMBOL(close);
> >  
> > +ssize_t readlink(const char *pathname, char *buf, size_t bufsiz)
> 
> man 2 readlink says:
> 
> > On  success,  readlink()  returns  the number of bytes placed in buf.
> > On error, -1 is returned and errno is set to indicate the error.
> 
> This function instead returns 0 for success and a negative error value.
> This is ok, but if the meaning of the return value is different, then it
> does not make sense to try to match the function prototype with the libc
> function.
> Make the return type int.
> 
> > +{
> > +	struct fs_driver_d *fsdrv;
> > +	struct fs_device_d *fsdev;
> > +	char *p = normalise_path(pathname);
> > +	char *freep = p;
> > +	int ret;
> > +	size_t len = 0;
> > +	char tmp[PATH_MAX];
> > +
> > +	tmp[0] = 0;
> > +	buf[0] = 0;
> > +
> > +	ret = path_check_prereq(pathname, S_IFLNK);
> > +	if (ret)
> > +		goto out;
> > +
> > +	fsdev = get_fs_device_and_root_path(&p);
> > +	if (!fsdev) {
> > +		ret = -ENODEV;
> > +		goto out;
> > +	}
> > +	fsdrv = fsdev->driver;
> > +
> > +	len = min(bufsiz, (size_t)PATH_MAX);
> > +	if (fsdrv->readlink) {
> > +		ret = fsdrv->readlink(&fsdev->dev, p, tmp, len);
> > +	} else {
> > +		ret = -EROFS;
> 
> EROFS is a strange error code here.
ENOSYS maybe better
> 
> > +		goto out;
> 
> This is not needed
> 
> > +	}
> > +
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (tmp[0] == '/') {
> > +		int l = strlen(fsdev->path);
> > +
> > +		if (fsdev->path[l - 1] == '/')
> > +			l--;
> > +
> > +		if (l) {
> > +			len -= l;
> > +			strncat(buf, fsdev->path, l);
> > +		}
> > +	}
> 
> I don't understand why you manipulate the link target when the link target
> is an absolute path. This means that if mount a fatfs to fat and put a
> link there:
> 
> /fat/somelink -> /env/bla
this is not how linux work

an absolute path is related to the mounted path that why I did so
> 
> Then readlink will return /fat/env/bla, which is not what you want.
it's in fact

I try on nfs

Best Regards,
J.

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