[PATCH] e2p: fix getflags for link file

[PATCH] e2p: fix getflags for link file

[Wang Shilong]

From: Wang Shilong <wshilong@ddn.com>

Steps to reproduce:

$ touch a
$ lsattr -dp a
 2000 ----------------P a
$ ln -s a b
$ lsattr -dp b
lsattr: Operation not supported While reading flags on b

Link files should be supported, fix it.

Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
 lib/e2p/fgetflags.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/e2p/fgetflags.c b/lib/e2p/fgetflags.c
index 7b93cba..a39cdbd 100644
--- a/lib/e2p/fgetflags.c
+++ b/lib/e2p/fgetflags.c
@@ -73,7 +73,8 @@ int fgetflags (const char * name, unsigned long * flags)
 	int fd, r, f, save_errno = 0;
 
 	if (!lstat(name, &buf) &&
-	    !S_ISREG(buf.st_mode) && !S_ISDIR(buf.st_mode)) {
+	    !S_ISREG(buf.st_mode) && !S_ISDIR(buf.st_mode) &&
+	    !S_ISLNK(buf.st_mode)) {
 		goto notsupp;
 	}
 #if !APPLE_DARWIN
-- 
1.8.3.1

Re: [PATCH] e2p: fix getflags for link file

[Theodore Ts'o]

On Mon, Dec 04, 2017 at 10:56:35PM +0900, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
> 
> Steps to reproduce:
> 
> $ touch a
> $ lsattr -dp a
>  2000 ----------------P a
> $ ln -s a b
> $ lsattr -dp b
> lsattr: Operation not supported While reading flags on b
> 
> Link files should be supported, fix it.
> 
> Signed-off-by: Wang Shilong <wshilong@ddn.com>

You can't actually set or get the extended attributes for a symlink;
the *kernel* doesn't support what you seem to be hoping to achieve.
This patch result in fgetflags() following the symlink and opening
whatever file it points at.

	      	      	  	  	   	- Ted

Re: [PATCH] e2p: fix getflags for link file

[Wang Shilong]

On Wed, Dec 6, 2017 at 12:10 AM, Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Dec 04, 2017 at 10:56:35PM +0900, Wang Shilong wrote:
> > From: Wang Shilong <wshilong@ddn.com>
> >
> > Steps to reproduce:
> >
> > $ touch a
> > $ lsattr -dp a
> >  2000 ----------------P a
> > $ ln -s a b
> > $ lsattr -dp b
> > lsattr: Operation not supported While reading flags on b
> >
> > Link files should be supported, fix it.
> >
> > Signed-off-by: Wang Shilong <wshilong@ddn.com>
>
> You can't actually set or get the extended attributes for a symlink;
> the *kernel* doesn't support what you seem to be hoping to achieve.
> This patch result in fgetflags() following the symlink and opening
> whatever file it points at.

Yup, you are right.

But this is one of our customers feedback, this is not good that
lsattr/chattr did not support symlink, we might need make it
clear, for example, we support symlink, but it always follow
original files, that is even better than output errors.

In this way, we need fix chattr too.

What do you think?


Thanks,
Shilong


>
>                                                 - Ted

Re: [PATCH] e2p: fix getflags for link file

[Theodore Ts'o]

On Wed, Dec 06, 2017 at 09:19:26AM +0800, Wang Shilong wrote:
> > But this is one of our customers feedback, this is not good that
> > lsattr/chattr did not support symlink, we might need make it
> > clear, for example, we support symlink, but it always follow
> > original files, that is even better than output errors.

What is the basis of the customer's complaint?  I could imagine
printing a more explanatory message.  So instead of:

lsattr: Operation not supported While reading flags on /foo/bar/baz

maybe:

/foo/bar/baz: file attributes not supported for symlinks

It's not clear to me at all that following symlinks is the right
thing.

> >
> > In this way, we need fix chattr too.
> 
> Just to think more, there is a problem to follow symlink for chattr:
> consider following case that users want to use directory quota:
> 
> dir1/dir1.1         --->project ID is 1
> dir2/dir2.link.1.1 ----->dir2's Project ID is 2, link file to dir1.1
> 
> Considering if users do something like:
> #chattr -p 1 -R dir1
> #chattr -p 2 -R dir2/
> 
> This will break some users expected behavior, since dir1.1
> will be set to project ID 2 which expected as 1.
> 
> So i supposed we should disallow follow symlink for chattr?

... and this is why I don't think we should change the behavior for
lsattr *or* chattr.

I could imagine adding an option which causes lsattr and chattr to
follow symlinks (but in the absense of the option, lsattr and chattr
will do what it does today, which is not follow symlinks, possibly
with a friendly error message), or maybe which causes lsattr to print
something like this:

--------------e---- /usr/bin/emacsclient.emacs24
   <symlink>        /usr/bin/emacs -> /etc/alternatives/emacs

... but honestly, is it really worth it?

					- Ted