Description of problem: This bug is opened to review a patch written by Andreas Dilger. The patch adds support for save/restore of "lustre.*" extended attributes. Additionally it ensures the attributes are set before a file is extracted. Actual results: The patch is not applied in Fedora, nor in upstream. Expected results: The patch is reviewed and applied. Additional info: I've updated the patch against the current sources of Fedora tar and split it into two pieces.
Created attachment 388800 [details] [PATCH 1/2] xattrs: add support for lustre attributes
Created attachment 388801 [details] [PATCH 2/2] xattrs: set attributes before extracting a file
Andreas, could you please have a look at the patches? Do they still work for you? Do we have any test case for them? Thanks in advance!
Created attachment 389010 [details] extract xattrs before writing file data Attached is the current version of the extract xattrs first patch, with a few cleanups over the one you had.
Created attachment 389012 [details] extract xattrs before writing file data
Created attachment 389013 [details] allow lustre.* extended attributes to be backed up and restored Updated version of the xattr-lustre patch. The one attached to this bug failed to handle the lustre attributes correctly.
Comment on attachment 389013 [details] allow lustre.* extended attributes to be backed up and restored >--- tar-1.22.sun1.orig/src/xheader.c >+++ tar-1.22.sun1/src/xheader.c >@@ -1677,9 +1677,10 @@ struct xhdr_tab const xhdr_tab[] = { > { "SCHILY.xattr.system.posix_acl_default", > xattr_acls_d_coder, xattr_acls_d_decoder, false, false }, > >- /* xattr's, use the star format note we only save the user/trusted varients... */ >+ /* xattrs use the star format. note we only save some variants... */ > { "SCHILY.xattr.user", xattr_coder, xattr_decoder, false, true }, > { "SCHILY.xattr.trusted", xattr_coder, xattr_decoder, false, true }, >+ { "SCHILY.xattr.lustre", xattr_coder, xattr_decoder, false, true }, > > /* ignore everything else in the xattr namespaces... */ > { "SCHILY.xattr", dummy_coder, dummy_decoder, false, true }, The above is the only change on top of the attachment #388800 [details]. Over all the patch looks good to me, granting r+.
Comment on attachment 389012 [details] extract xattrs before writing file data >--- tar-1.22.sun1.orig/src/extract.c >+++ tar-1.22.sun1/src/extract.c >@@ -297,7 +297,6 @@ set_stat (char const *file_name, > > xattrs_acls_set(st, file_name, typeflag); > xattrs_selinux_set(st, file_name, typeflag); >- xattrs_xattrs_set(st, file_name, typeflag); I'll need to test the above whether it's still safe. There have been several changes in handling xattrs/selinux for symlinks etc. in the meantime. >- | (old_files_option == OVERWRITE_OLD_FILES >- ? O_TRUNC >- : O_EXCL)); >+ | (old_files_option == OVERWRITE_OLD_FILES >+ ? O_TRUNC >+ : O_EXCL)); The above ^^^ changes only indentation for no reason. >@@ -817,8 +845,18 @@ extract_file (char *file_name, int typef > } > else > { >+ int file_created = 0; >+ if (set_xattr (file_name, ¤t_stat_info, invert_permissions, >+ typeflag, &file_created)) A tab is embedded in the line above ^^^.
Comment on attachment 389012 [details] extract xattrs before writing file data >--- tar-1.22.sun1.orig/src/extract.c >+++ tar-1.22.sun1/src/extract.c >@@ -297,7 +297,6 @@ set_stat (char const *file_name, > > xattrs_acls_set(st, file_name, typeflag); > xattrs_selinux_set(st, file_name, typeflag); >- xattrs_xattrs_set(st, file_name, typeflag); r-, the above breaks restoring of xattrs for directories. Could we just keep the line there?
(In reply to comment #9) > (From update of attachment 389012 [details]) > >--- tar-1.22.sun1.orig/src/extract.c > >+++ tar-1.22.sun1/src/extract.c > >@@ -297,7 +297,6 @@ set_stat (char const *file_name, > > > > xattrs_acls_set(st, file_name, typeflag); > > xattrs_selinux_set(st, file_name, typeflag); > >- xattrs_xattrs_set(st, file_name, typeflag); > > r-, the above breaks restoring of xattrs for directories. > > Could we just keep the line there? That would probably work - in some cases the xattrs will be set twice, but at least for Lustre I don't think this will cause a problem (it will silently ignore the xattrs that cannot be changed after file creation).
Created attachment 394722 [details] [v2] extract xattrs before writing file data Andreas, could you please have a look at the new version of the patch?
Created attachment 395254 [details] up2date version of the xattrs patch in Fedora (including both of the patches above)
built as tar-1.22-16.fc14
Comment on attachment 394722 [details] [v2] extract xattrs before writing file data This version of the patch is obsolete, yet I'm being asked every week to review the patch and I'm not able to mark the patch '+' or remove my review request.
Comment on attachment 394722 [details] [v2] extract xattrs before writing file data I am canceling the review request. Sorry about the inconveniences.