[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L
From: |
Kamil Dudka |
Subject: |
Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L |
Date: |
Mon, 3 Oct 2011 19:29:20 +0200 |
User-agent: |
KMail/1.9.10 |
On Monday 03 October 2011 18:25:10 Bruno Haible wrote:
> Kamil Dudka wrote:
> > > 2) see a test case added to gnulib or coreutils.
> >
> > A while ago, Jim wrote a test-case for coreutils that catches exactly the
> > bug that the original patch introduced.
>
> I'm asking for a test case also for the bug that the original patch fixed.
Fair enough. I am adding this to my TODO list, although there are more urgent
issues preventing me from working on this atm.
> Thanks for that background. So we're looking at "ls -l /media" or
> "ls -ld mountpoint". Since you've mentioned symlinks, we also have to
> consider symlinks to mountpoints.
Well, symlinks to mount points were not mentioned in that bug. I would need
to ask kernel guys whether symlinks to mount points are worth to consider in
this fix at all.
> So in fact we have 9 cases:
> a) A non-symlink, non-directory. Here acl_extended_file_nofollow and
> acl_extended_file are equivalent.
If I understand this, you expect non-directories cannot be mount points, thus
the call cannot trigger the mount, right?
> b) A directory that is not a mount point.
> c) A mount point.
> d) A symlink to a file, with dereferencing (stat()).
> e) A symlink to a directory that is not a mount point, with dereferencing
> (stat()). f) A symlink to a mount point, with dereferencing (stat()).
> g) A symlink to a file, without dereferencing (lstat()).
> h) A symlink to a directory that is not a mount point, without
> dereferencing (lstat()). i) A symlink to a mount point, without
> dereferencing (lstat()).
>
> Remember the option '-L' of 'ls' is meant to mean dereference a symbolic
> link, but does not mean a different treatment of mount points. But in the
> kernel, the difference between getxattr and lgetxattr is a flags word of
> LOOKUP_FOLLOW vs. 0 and - apparently, like you say - makes a difference for
> mount points.
The behavior of ls wrt. tregerring mounts is implementation defined, isn't it?
Let's take it such and just choose the implementaion that makes sense.
> So how do these 9 cases need to be handled:
> a) Either acl_extended_file_nofollow or acl_extended_file will do.
> b) Either acl_extended_file_nofollow or acl_extended_file will do.
> c) Must call acl_extended_file_nofollow.
> d) Either use acl_extended_file, or find the target filename through
> canonicalize_filename_mode(CAN_EXISTING), then
> acl_extended_file_nofollow. e) Either detect the non-mount-point through
> statvfs() or similar then call acl_extended_file, or must find the target
> filename through
> canonicalize_filename_mode(CAN_EXISTING) then call
> acl_extended_file_nofollow. f) Must find the target filename through
> canonicalize_filename_mode(CAN_EXISTING), then
> acl_extended_file_nofollow. g) Must return 0.
> h) Must return 0.
> i) Must return 0.
>
> The original code mishandled the cases c), e), f).
> The patch from 2011-07-22 corrected case c) and mishandled the cases d),
> e), f). Your current patch corrects the cases d), e), but still mishandles
> case f). And it adds an extra, unnecessary system call in case a).
The Problem is that case a) is not detectable on its own.
> I'm trying to avoid useless system calls. If
> ! S_ISLNK (sb->st_mode) && ! S_ISDIR (sb->st_mode)
> then we are in case a) or d), and then no additional lstat() call is
> needed. Since case a) is the most frequent one, I would find it worth to do
> this check for S_ISDIR (sb->st_mode) since it can save an lstat() call.
>
> So I'm asking to change
>
> ret = acl_extended_file_wrap (name);
>
> to
>
> if (S_ISDIR (sb->st_mode))
> ret = acl_extended_file_wrap (name);
> else
> ret = acl_extended_file (name);
Is optimization the only purpose of this change? I would then prefer to get
it working first and optimize it later on.
> But still still does not fix the problem of case f). Here, with your
> current patch, "ls -lLd symlink-to-mountpoint" will call
> acl_extended_file(), which will call getxattr(), which will activate an
> autofs mount.
Case f) has never worked, nobody complained. Partial fix is better than no
fix. The last patch does not break anything that worked before, does it?
> IMO, there are two solutions to this:
> - Either convince the kernel people to introduce a different flag,
> keeping LOOKUP_FOLLOW for symlinks only,
This is non-starter. Kernel developers are not interested in optimizing out
stat() calls from user-space. There is a similar, more serious, bug open for
years. Nobody cares.
https://bugzilla.redhat.com/501848
> - Or use code equivalent to canonicalize_filename_mode(CAN_EXISTING).
I am personally not intersted in changing something that works unless there is
a user that needs the change actually.
Kamil
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, (continued)
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Bruno Haible, 2011/10/03
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Kamil Dudka, 2011/10/03
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Kamil Dudka, 2011/10/03
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Bruno Haible, 2011/10/03
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L,
Kamil Dudka <=
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Bruno Haible, 2011/10/03
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Kamil Dudka, 2011/10/03
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Jim Meyering, 2011/10/05
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Bruno Haible, 2011/10/05
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Jim Meyering, 2011/10/05
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Kamil Dudka, 2011/10/05
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Kamil Dudka, 2011/10/03
- Re: [PATCH] file-has-acl: revert unintended change in behavior of ls -L, Kamil Dudka, 2011/10/03