On Tue, May 3, 2016 at 1:23 AM, Andreas Dilger <adilger@xxxxxxxxx> wrote: > On May 2, 2016, at 4:45 PM, Andreas Gruenbacher <agruenba@xxxxxxxxxx> wrote: >> >> Hi all, >> >> what are your thoughts on this patch set? It applies on top of the >> work.xattr branch [*], converts the remaining filesystems over to xattr >> handlers, and replaces the getxattr, setxattr, and removexattr inode >> operations. The only way to implement getxattr, setxattr, and >> removexattr with this approach is through xattr handlers. > > Maybe I'm missing the point of this patch, but is there a long-term benefit > to this change? It seems to be replacing the ->getxattr() and related inode > methods with a generic ->xattr handler list that needs to be demultiplexed > on each access? Is there a net improvement in functionality that comes > out the other end? All filesystems except for things like overlayfs and fuse implement this kind of multiplexing already, so we might as well make that the default. I'm hoping that we can further clean things up on top to get rid of at least some of the name parsing in fs/xattr.c that has crept in over time; it's not pretty. > That isn't at all clear from your comments or the > patches themselves. It seems that all of LOC savings is from deleting > the .setxattr, .getxattr, and .removexattr lines from inode_operations > but there is added complexity in the rest of the code? Hmm, except for the rather trivial dummy dispatching for bad inodes, "empty" directories, fuse, and overlayfs, here do you see increased complexity? The indirection through the generic inode operations goes away, which isn't a bad thing. I agree that better documentation would help. Thanks, Andreas -- To unsubscribe from this list: send the line "unsubscribe ecryptfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
