Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement File Attribute Support #1693

Closed
wants to merge 2 commits into from
Closed

Implement File Attribute Support #1693

wants to merge 2 commits into from

Conversation

ryao
Copy link
Contributor

@ryao ryao commented Sep 4, 2013

We add support for lsattr and chattr. Only attributes common to both
Solaris and Linux are supported. These are 'a', 'd' and 'i'. File
attributes exclusive to Solaris are present in the ZFS code, but cannot
be accessed or modified through this method. That was the case prior to
this patch.

This commit removes the ZFS_IOC_GETFLAGS and ZFS_IOC_SETFLAGS macros in
recognition that this is not equivalent to the Solaris operation. The
resolution of issue #229 should implement something
equivalent that will permit access and modification of Solaris-specific
attributes.

This resolves a regression caused by
88c2839 that broke python's
xattr.list(). This broke Gentoo Portage's FEATURES=xattr, which depended
on this.

https://bugs.gentoo.org/show_bug.cgi?id=483516
Issue #1691

Original-patch-by: Brian Behlendorf [email protected]
Signed-off-by: Richard Yao [email protected]

@ryao
Copy link
Contributor Author

ryao commented Sep 4, 2013

I consider this to be a high priority fix because it affects Gentoo Portage.

https://bugs.gentoo.org/show_bug.cgi?id=483516

I have asked the Gentoo developer who encountered this problem to verify that it fixes it. I intend to fast track its inclusion into Gentoo's ZFS packaging. I plan to commit a revision to Gentoo's ZFS kernel package tomorrow that includes this unless an issue is found before then.

@behlendorf
Copy link
Contributor

@ryao Is this still something you're working on?

@ryao
Copy link
Contributor Author

ryao commented Sep 12, 2013

@behlendorf I am trying to focus on the ZIO buffers right now. I plan to revisit this after I have my initial patches ready.

@aarcane
Copy link

aarcane commented Dec 29, 2013

I'm affected by this bug as well. Is it possible to move this to milestone 0.6.3 as is, and the enhanced version you're discussing to 0.6.4 ?

@behlendorf
Copy link
Contributor

@aarcane Maybe, I'd love to get this in 0.6.3 as well but it really just comes down to how much developer time is available. We'll try.

@ryao
Copy link
Contributor Author

ryao commented Apr 25, 2014

@behlendorf @aarcane I have refreshed this pull request. The new patch addresses previous criticisms and passes XFS Test generic/079 on my system, which tests the immutable and append bits.

@ryao ryao mentioned this pull request Apr 25, 2014
@behlendorf behlendorf modified the milestones: 0.6.3, 0.6.4 Apr 25, 2014
@behlendorf
Copy link
Contributor

@ryao Aside from the inline comments the general approach here looks good and it passed my local testing. If you can just address the comments and refresh the patch we should be able to get it in.

@ryao
Copy link
Contributor Author

ryao commented Apr 30, 2014

@behlendorf The pull request has been refreshed. The changes since your review a 6 hours ago should only be to make corrections for the points you made. My system is preoccupied running ztest, so I am not at liberty to do my usual regression tests. While I do not expect there to be any regressions, I am relying on your systems to catch any mistakes with this refresh.

@behlendorf
Copy link
Contributor

@ryao The updated autoconf check fails incorrectly for the older interface. Including linux/sched.h for current which is used in the is_owner_or_cap macro resolves the issue. After fixing this there was a second issue which is that the result of the macro isn't used so I've resolved that we with cast. Here's a proposed fix which works as expected under CentOS 6.5.

configure:25853: checking whether is_owner_or_cap() exists
configure:25886: cp conftest.c build && make modules -C /usr/src/kernels/2.6.32-431.11.2.1chaos.ch5.2.x86_64 EXTRA_CFLAGS=-Werror   M=/home/behlendo/src/git/zfs/build
/home/behlendo/src/git/zfs/build/conftest.c: In function ���main���:
/home/behlendo/src/git/zfs/build/conftest.c:124: error: implicit declaration of function ���current_fsuid���
cc1: warnings being treated as errors
/home/behlendo/src/git/zfs/build/conftest.c:124: error: value computed is not used
make[1]: *** [/home/behlendo/src/git/zfs/build/conftest.o] Error 1
make: *** [_module_/home/behlendo/src/git/zfs/build] Error 2
diff --git a/config/kernel-is_owner_or_cap.m4 b/config/kernel-is_owner_or_cap.m4
index 7e34c66..d7e81c0 100644
--- a/config/kernel-is_owner_or_cap.m4
+++ b/config/kernel-is_owner_or_cap.m4
@@ -18,10 +18,11 @@ AC_DEFUN([ZFS_AC_KERNEL_INODE_OWNER_OR_CAPABLE], [
                AC_MSG_RESULT(no)
                AC_MSG_CHECKING([whether is_owner_or_cap() exists])
                ZFS_LINUX_TRY_COMPILE([
+                       #include <linux/sched.h>
                        #include <linux/fs.h>
                ],[
                        struct inode *ip = NULL;
-                       is_owner_or_cap(ip);
+                       (void) is_owner_or_cap(ip);
                ],[
                        AC_MSG_RESULT(yes)
                        AC_DEFINE(HAVE_IS_OWNER_OR_CAP, 1, [is_owner_or_cap() ex

@behlendorf
Copy link
Contributor

@ryao Just a few more markups, can you refresh it again.

ryao added 2 commits April 30, 2014 22:58
We need inode_owner_or_capable() for ZFS file attributes in addition to
xattrs, so it should go into its own file. This moves it into its own
file and changes it to be more comprehensive. It will now fail if no
known good API is detected.

Signed-off-by: Richard Yao <[email protected]>
We add support for lsattr and chattr to resolve a regression caused by
88c2839 that broke Python's
xattr.list(). That changet broke Gentoo Portage's FEATURES=xattr, which
depended on Python's xattr.list().

Only attributes common to both Solaris and Linux are supported. These
are 'a', 'd' and 'i' in Linux's lsattr and chattr commands. File
attributes exclusive to Solaris are present in the ZFS code, but cannot
be accessed or modified through this method.  That was the case prior to
this patch. The resolution of issue openzfs#229 should implement
some method to permit access and modification of Solaris-specific
attributes.

https://bugs.gentoo.org/show_bug.cgi?id=483516
Issue openzfs#1691

Original-patch-by: Brian Behlendorf <[email protected]>
Signed-off-by: Richard Yao <[email protected]>
@behlendorf
Copy link
Contributor

Merged as:

9d31779 Implement File Attribute Support
3b4f425 Refactor inode_owner_or_capable() autotools check

Thanks @ryao for getting this pushed over the finish line. I'm thrilled to see this support finally merged.

@behlendorf behlendorf closed this May 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Feature request or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants