Prev: [PATCH 0/2] cfq-iosched: fixing RQ_NOIDLE handling.
Next: Add async PF initialization to PV guest.
From: Nick Piggin on 7 Jul 2010 11:30
On Tue, Jun 15, 2010 at 10:42:55PM +0530, Aneesh Kumar K.V wrote:
> From: NeilBrown <neilb(a)suse.de>
>
> This enables to use readlink to get the link target name
> from a file descriptor point to the link. This can be used
> with open_by_handle syscall that returns a file descriptor for a link.
> We can then use this file descriptor to get the target name.
>
> Signed-off-by: NeilBrown <neilb(a)suse.de>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar(a)linux.vnet.ibm.com>
> ---
> fs/stat.c | 30 ++++++++++++++++++++++--------
> 1 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/fs/stat.c b/fs/stat.c
> index c4ecd52..49b95a7 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -284,26 +284,40 @@ SYSCALL_DEFINE2(newfstat, unsigned int, fd, struct stat __user *, statbuf)
> SYSCALL_DEFINE4(readlinkat, int, dfd, const char __user *, pathname,
> char __user *, buf, int, bufsiz)
> {
> - struct path path;
> - int error;
> + int error = 0;
> + struct path path, *pp;
> + struct file *file = NULL;
>
> if (bufsiz <= 0)
> return -EINVAL;
>
> - error = user_path_at(dfd, pathname, 0, &path);
> + if (pathname == NULL && dfd != AT_FDCWD) {
> + file = fget(dfd);
> +
> + if (file)
> + pp = &file->f_path;
> + else
> + error = -EBADF;
> + } else {
> + error = user_path_at(dfd, pathname, 0, &path);
> + pp = &path;
> + }

This (and all the others) is really ugly overloading of syscall
arguments IMO, and the changelog is seriously lacking for such changes.

This also changes the the syscall API of existing calls; from reading
the path at NULL, to switching to a completely different syscall.
Perhaps you're assuming nobody relies on SIGSEGV / mmapped NULL address
there, but even then you surely need to document the changed semantics
somewhere (and document the new syscall semantics properly).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Aneesh Kumar K. V on 7 Jul 2010 12:40
On Thu, 8 Jul 2010 01:27:06 +1000, Nick Piggin <npiggin(a)suse.de> wrote:
> On Tue, Jun 15, 2010 at 10:42:55PM +0530, Aneesh Kumar K.V wrote:
> > From: NeilBrown <neilb(a)suse.de>
> >
> > This enables to use readlink to get the link target name
> > from a file descriptor point to the link. This can be used
> > with open_by_handle syscall that returns a file descriptor for a link.
> > We can then use this file descriptor to get the target name.
> >
> > Signed-off-by: NeilBrown <neilb(a)suse.de>
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar(a)linux.vnet.ibm.com>
> > ---
> > fs/stat.c | 30 ++++++++++++++++++++++--------
> > 1 files changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/stat.c b/fs/stat.c
> > index c4ecd52..49b95a7 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -284,26 +284,40 @@ SYSCALL_DEFINE2(newfstat, unsigned int, fd, struct stat __user *, statbuf)
> > SYSCALL_DEFINE4(readlinkat, int, dfd, const char __user *, pathname,
> > char __user *, buf, int, bufsiz)
> > {
> > - struct path path;
> > - int error;
> > + int error = 0;
> > + struct path path, *pp;
> > + struct file *file = NULL;
> >
> > if (bufsiz <= 0)
> > return -EINVAL;
> >
> > - error = user_path_at(dfd, pathname, 0, &path);
> > + if (pathname == NULL && dfd != AT_FDCWD) {
> > + file = fget(dfd);
> > +
> > + if (file)
> > + pp = &file->f_path;
> > + else
> > + error = -EBADF;
> > + } else {
> > + error = user_path_at(dfd, pathname, 0, &path);
> > + pp = &path;
> > + }
>
> This (and all the others) is really ugly overloading of syscall
> arguments IMO, and the changelog is seriously lacking for such
> changes.

Initially we had freadlink

http://lkml.org/lkml/2010/5/12/222

We updated the patches to use the existing readlinkat interface because
utimensat(2) already exposed a similar interface. So it should be ok to
expect that other *at call behaved in a similar way ?

>
> This also changes the the syscall API of existing calls; from reading
> the path at NULL, to switching to a completely different syscall.
> Perhaps you're assuming nobody relies on SIGSEGV / mmapped NULL address
> there, but even then you surely need to document the changed semantics
> somewhere (and document the new syscall semantics properly).


Yes this would need a documentation update. But i guess since we already
have utimensat(2) behaving similarly we are ok to extent readlinkat,
linkat and faccessat on similar lines ?

-aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Nick Piggin on 7 Jul 2010 13:10
On Wed, Jul 07, 2010 at 10:02:12PM +0530, Aneesh Kumar K. V wrote:
> On Thu, 8 Jul 2010 01:27:06 +1000, Nick Piggin <npiggin(a)suse.de> wrote:
> > This (and all the others) is really ugly overloading of syscall
> > arguments IMO, and the changelog is seriously lacking for such
> > changes.
>
> Initially we had freadlink
>
> http://lkml.org/lkml/2010/5/12/222
>
> We updated the patches to use the existing readlinkat interface because
> utimensat(2) already exposed a similar interface. So it should be ok to
> expect that other *at call behaved in a similar way ?

I'm not sure whether it's OK or not. Probably is, it is a slight API
change though, that should at least be noted in the changelog.


> > This also changes the the syscall API of existing calls; from reading
> > the path at NULL, to switching to a completely different syscall.
> > Perhaps you're assuming nobody relies on SIGSEGV / mmapped NULL address
> > there, but even then you surely need to document the changed semantics
> > somewhere (and document the new syscall semantics properly).
>
>
> Yes this would need a documentation update. But i guess since we already
> have utimensat(2) behaving similarly we are ok to extent readlinkat,
> linkat and faccessat on similar lines ?

At least there is precedent. Pretty ugly though :( Well if others
(Christoph and Al, primarily) think it's OK then fine by me. But
please put comments or changelog for API changes such that a man page
writer could easily update it.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Miklos Szeredi on 12 Jul 2010 04:10
On Mon, 12 Jul 2010, Aneesh Kumar K.V wrote:
> From: NeilBrown <neilb(a)suse.de>
>
> This enables to use readlink to get the link target name
> from a file descriptor point to the link. This can be used
> with open_by_handle syscall that returns a file descriptor for a link.
> We can then use this file descriptor to get the target name.
>
> This is similar to utimensat(2) interface

We could introduce pair of new helper functions to extract the common
code from do_utimes() and this:

err = lookup_path_at(dfd, filename, atflags, &path, &file);
/* do something with path */
put_path_at(&path, file);

Thanks,
Miklos

> Signed-off-by: NeilBrown <neilb(a)suse.de>
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar(a)linux.vnet.ibm.com>
> ---
> fs/stat.c | 30 ++++++++++++++++++++++--------
> 1 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/fs/stat.c b/fs/stat.c
> index c4ecd52..a66a0ef 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -284,26 +284,40 @@ SYSCALL_DEFINE2(newfstat, unsigned int, fd, struct stat __user *, statbuf)
> SYSCALL_DEFINE4(readlinkat, int, dfd, const char __user *, pathname,
> char __user *, buf, int, bufsiz)
> {
> - struct path path;
> - int error;
> + int error = 0, fput_needed;
> + struct path path, *pp;
> + struct file *file = NULL;
>
> if (bufsiz <= 0)
> return -EINVAL;
>
> - error = user_path_at(dfd, pathname, 0, &path);
> + if (pathname == NULL && dfd != AT_FDCWD) {
> + file = fget_light(dfd, &fput_needed);
> +
> + if (file)
> + pp = &file->f_path;
> + else
> + error = -EBADF;
> + } else {
> + error = user_path_at(dfd, pathname, 0, &path);
> + pp = &path;
> + }
> if (!error) {
> - struct inode *inode = path.dentry->d_inode;
> + struct inode *inode = pp->dentry->d_inode;
>
> error = -EINVAL;
> if (inode->i_op->readlink) {
> - error = security_inode_readlink(path.dentry);
> + error = security_inode_readlink(pp->dentry);
> if (!error) {
> - touch_atime(path.mnt, path.dentry);
> - error = inode->i_op->readlink(path.dentry,
> + touch_atime(pp->mnt, pp->dentry);
> + error = inode->i_op->readlink(pp->dentry,
> buf, bufsiz);
> }
> }
> - path_put(&path);
> + if (file)
> + fput_light(file, fput_needed);
> + else
> + path_put(&path);
> }
> return error;
> }
> --
> 1.7.2.rc1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [PATCH 0/2] cfq-iosched: fixing RQ_NOIDLE handling.
Next: Add async PF initialization to PV guest.