Hi Dawid, thanks for reporting this! On Thursday, October 6, 2016 12:47:26 PM CEST Dawid wrote: > I've tried to use `tar + ssh + tar` pipeline to copy files between two > hosts. The only twist over many examples you can find with 2 minutes of > online searching is that I need to copy `xattrs` and not overwrite the > existing files. > > To my surprise it worked the first time, but on subsequent attempts it > just hangs. After some investigation, it turned out that if one uses > `--xattrs --skip-old-files` together, and `tar x ...` encouter existing > file, it just hangs.
That's bug. The set_xattr() function ignores RECOVER_SKIP. > More investigation (when looking for workaround) revealed that > `--keep-old-files` does not seem to preserve `xattrs` of the destination. The > original destination file will be kept, but the xattrs will be overwritten > anyway. This is related bug too, can you please try the attached patch? Pavel
>From f201b360e67f53fb0a0e367dc69e7e59f3a38d3f Mon Sep 17 00:00:00 2001 From: Pavel Raiskup <[email protected]> Date: Fri, 7 Oct 2016 11:45:17 +0200 Subject: [PATCH] xattrs: don't set xattrs when --skip-old-files is used * src/extract.c (set_xattr): Properly handle maybe_recoverable() output. Throw warnings to not complicate caller. (extract_file): Don't handle set_xattr's error. * tests/xattr06.at: New testcase. * tests/Makefile.am: Mention new testcase. * tests/testsuite.at: Likewise. * THANKS: Dawid. --- THANKS | 1 + src/extract.c | 41 +++++++++++++++++++----------- tests/Makefile.am | 1 + tests/testsuite.at | 1 + tests/xattr06.at | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 102 insertions(+), 15 deletions(-) create mode 100644 tests/xattr06.at diff --git a/THANKS b/THANKS index f1def93..5e8e8c9 100644 --- a/THANKS +++ b/THANKS @@ -138,6 +138,7 @@ David Nugent [email protected] David Shaw [email protected] David Steiner [email protected] David Taylor [email protected] +Dawid [email protected] Dean Gaudet [email protected] Demizu Noritoshi [email protected] Denis Excoffier [email protected] diff --git a/src/extract.c b/src/extract.c index f982433..67885d7 100644 --- a/src/extract.c +++ b/src/extract.c @@ -795,13 +795,13 @@ maybe_recoverable (char *file_name, bool regular, bool *interdir_made) in advance dramatically improves the following performance of reading and writing a file). If not restoring permissions, invert the INVERT_PERMISSIONS bits from the file's current permissions. TYPEFLAG specifies the type of the - file. FILE_CREATED indicates set_xattr has created the file */ + file. Returns non-zero when error occurs (while un-available xattrs is not + an error, rather no-op). Non-zero FILE_CREATED indicates set_xattr has + created the file. */ static int set_xattr (char const *file_name, struct tar_stat_info const *st, mode_t invert_permissions, char typeflag, int *file_created) { - int status = 0; - #ifdef HAVE_XATTRS bool interdir_made = false; @@ -809,17 +809,32 @@ set_xattr (char const *file_name, struct tar_stat_info const *st, { mode_t mode = current_stat_info.stat.st_mode & MODE_RWX & ~ current_umask; - do - status = mknodat (chdir_fd, file_name, mode ^ invert_permissions, 0); - while (status && maybe_recoverable ((char *)file_name, false, - &interdir_made)); + for (;;) + { + if (!mknodat (chdir_fd, file_name, mode ^ invert_permissions, 0)) + { + /* Successfully created file */ + xattrs_xattrs_set (st, file_name, typeflag, 0); + *file_created = 1; + return 0; + } - xattrs_xattrs_set (st, file_name, typeflag, 0); - *file_created = 1; + switch (maybe_recoverable ((char *)file_name, false, &interdir_made)) + { + case RECOVER_OK: + continue; + case RECOVER_NO: + skip_member (); + open_error (file_name); + return 1; + case RECOVER_SKIP: + return 0; + } + } } #endif - return(status); + return 0; } /* Fix the statuses of all directories whose statuses need fixing, and @@ -1136,11 +1151,7 @@ extract_file (char *file_name, int typeflag) int file_created = 0; if (set_xattr (file_name, ¤t_stat_info, invert_permissions, typeflag, &file_created)) - { - skip_member (); - open_error (file_name); - return 1; - } + return 1; while ((fd = open_output_file (file_name, typeflag, mode, file_created, ¤t_mode, diff --git a/tests/Makefile.am b/tests/Makefile.am index 5376180..06f2325 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -246,6 +246,7 @@ TESTSUITE_AT = \ xattr03.at\ xattr04.at\ xattr05.at\ + xattr06.at\ acls01.at\ acls02.at\ acls03.at\ diff --git a/tests/testsuite.at b/tests/testsuite.at index 1f1897b..e0525a1 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -445,6 +445,7 @@ m4_include([xattr02.at]) m4_include([xattr03.at]) m4_include([xattr04.at]) m4_include([xattr05.at]) +m4_include([xattr06.at]) m4_include([acls01.at]) m4_include([acls02.at]) diff --git a/tests/xattr06.at b/tests/xattr06.at new file mode 100644 index 0000000..85d3bc7 --- /dev/null +++ b/tests/xattr06.at @@ -0,0 +1,73 @@ +# Process this file with autom4te to create testsuite. -*- Autotest -*- +# +# Test suite for GNU tar. +# Copyright 2011, 2013-2014, 2016 Free Software Foundation, Inc. + +# This file is part of GNU tar. + +# GNU tar is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. + +# GNU tar is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# +# Test description: +# Test that --keep-old-files doesn't change xattrs of already existing file. +# Per report: +# https://lists.gnu.org/archive/html/bug-tar/2016-10/msg00001.html + +AT_SETUP([xattrs: xattrs and --skip-old-files]) +AT_KEYWORDS([xattrs xattr06]) + +AT_TAR_CHECK([ +AT_XATTRS_PREREQ +mkdir dir +genfile --file dir/file +genfile --file dir/file2 + +setfattr -n user.test -v OurDirValue dir +setfattr -n user.test -v OurFileValue dir/file +setfattr -n user.test -v OurFileValue dir/file2 + +tar --xattrs -cf archive.tar dir + +setfattr -n user.test -v OurDirValue2 dir +setfattr -n user.test -v OurFileValue2 dir/file +setfattr -n user.test -v OurFileValue2 dir/file2 + +# Check that tar continues to file2 too! +tar --xattrs -xvf archive.tar --skip-old-files +tar --xattrs -xvf archive.tar --keep-old-files + +getfattr -h -d dir | grep -v -e '^#' -e ^$ +getfattr -h -d dir/file | grep -v -e '^#' -e ^$ +getfattr -h -d dir/file2 | grep -v -e '^#' -e ^$ +], +[0], +[dir/ +dir/file +dir/file2 +dir/ +dir/file +dir/file2 +user.test="OurDirValue2" +user.test="OurFileValue2" +user.test="OurFileValue2" +], [tar: dir: skipping existing file +tar: dir/file: skipping existing file +tar: dir/file: skipping existing file +tar: dir/file2: skipping existing file +tar: dir/file2: skipping existing file +tar: dir/file: Cannot open: File exists +tar: dir/file2: Cannot open: File exists +tar: Exiting with failure status due to previous errors +]) + +AT_CLEANUP -- 2.7.4
