On 09/22/2012 11:17 PM, Jim Meyering wrote:
Ondrej Oprala wrote:
On 08/07/2012 05:37 PM, Bernhard Voelker wrote:
On 08/07/2012 05:01 PM, Ondrej Oprala wrote:
Hi, I've renamed the variable to be more hinting of it's purpose
and --no-preserve=mode should now work properly with directories
as well.
Cheers,
Ondrej
Thanks, that looks good ... maybe the directory case deserves to
be added to the test. Minor nit: the change in copy_internal() is
missing in the commit log.
I'm sure that Padraig/Jim will add both when committing.
BTW: Interestingly, the TODO entry mentioned "--no-preserve=X"
to be buggy:
-cp --no-preserve=X should not attempt to preserve attribute X
- reported by Andreas Schwab
but in fact, all the modes (t,o,l,c, and x) but "mode"
have already been working. ;-)
Have a nice day,
Berny
Hi, I've amended the commit log and added a test for directories.
Thanks again for your work on this.
Sorry it took so long for me to get back to it.
I've looked over the patch and noticed a problem: What happens
when we use --no-preserve=mode and --preserve=all together?
I would not expect --no-preserve=mode to silently override
a following --preserve=all.
Rather, the latter should supersede the former.
Hmm... the right thing appears to be what happens,
in spite of conflicting settings.
It looks like while the two members .preserve_mode and
.explicit_no_preserve_mode can both be set (a contradiction),
that doesn't cause an actual problem because whenever it happens,
.preserve_mode is both correct and the member that is tested first
in an if-else-if... cascade.
So maybe all you need to do it to turn off .explicit_no_preserve_mode
in the PRESERVE_ALL case?
A test case addition to cover that case would be most welcome,
but since I've waited so long to give feedback, it seems unfair
to require that.
From d52f8990353dac04bff141ff31b6601ba5112a18 Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <oopr...@redhat.com>
Date: Tue, 7 Aug 2012 16:56:52 +0200
Subject: [PATCH] cp: Fix the --no-preserve=mode option
The .explicit_no_preserve_mode flag should now be turned off when
--preserve=all
is run into.
I've also added a test for the mixed options next to other tests in
preserve-mode.sh.
Cheers,
Ondrej
>From f0c2c21ee919a81be8009905b0523759d98b446b Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <oopr...@redhat.com>
Date: Tue, 7 Aug 2012 16:56:52 +0200
Subject: [PATCH] cp: Fix the --no-preserve=mode option
* NEWS: Mention the fix.
* TODO: Remove an entry.
* src/copy.c (copy_reg): Add a condition to properly
handle the --no-preserve=mode option for files
(copy_internal): Add a condition to properly handle the
--no-preserve=mode option for directories.
* src/copy.h (struct cp_options): Add a new boolean.
* src/cp.c (cp_option_init,decode_preserve_arg): Set the
new boolean value according to specified options.
* src/install.c (struct cp_options): Initialize the new boolean.
* src/mv.c (struct cp_options): Initialize the new boolean.
* tests/cp/preserve-mode.sh: Add a new test.
* tests/local.mk: Add the new test to the list.
---
NEWS | 3 +++
TODO | 3 ---
src/copy.c | 10 +++++++++
src/copy.h | 1 +
src/cp.c | 3 +++
src/install.c | 1 +
src/mv.c | 1 +
tests/cp/preserve-mode.sh | 54 +++++++++++++++++++++++++++++++++++++++++++++++
tests/local.mk | 1 +
9 files changed, 74 insertions(+), 3 deletions(-)
create mode 100755 tests/cp/preserve-mode.sh
diff --git a/NEWS b/NEWS
index edc436c..25f1a27 100644
--- a/NEWS
+++ b/NEWS
@@ -12,6 +12,9 @@ GNU coreutils NEWS -*-
outline -*-
** Bug fixes
+ cp --no-preserve=mode now no longer preserves the original file's
+ permissions but correctly sets mode specified by 0666 & ~umask
+
du no longer emits a "disk-corrupted"-style diagnostic when it detects
a directory cycle that is due to a bind-mounted directory. Instead,
it detects this precise type of cycle, diagnoses it as such and
diff --git a/TODO b/TODO
index cd434e5..2dcaf08 100644
--- a/TODO
+++ b/TODO
@@ -47,9 +47,6 @@ doc/coreutils.texi:
ls: add --format=FORMAT option that controls how each line is printed.
-cp --no-preserve=X should not attempt to preserve attribute X
- reported by Andreas Schwab
-
copy.c: Address the FIXME-maybe comment in copy_internal.
And once that's done, add an exclusion so that 'cp --link'
no longer incurs the overhead of saving src. dev/ino and dest. filename
diff --git a/src/copy.c b/src/copy.c
index 2558fea..16aed03 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1151,6 +1151,11 @@ preserve_metadata:
if (set_acl (dst_name, dest_desc, x->mode) != 0)
return_val = false;
}
+ else if (x->explicit_no_preserve_mode)
+ {
+ set_acl (dst_name, dest_desc, 0666 & ~cached_umask ());
+ return_val = false;
+ }
else if (omitted_permissions)
{
omitted_permissions &= ~ cached_umask ();
@@ -2570,6 +2575,11 @@ copy_internal (char const *src_name, char const
*dst_name,
if (set_acl (dst_name, -1, x->mode) != 0)
return false;
}
+ else if (x->explicit_no_preserve_mode)
+ {
+ if (set_acl (dst_name, -1, 0777 & ~cached_umask ()) != 0)
+ return false;
+ }
else
{
if (omitted_permissions)
diff --git a/src/copy.h b/src/copy.h
index d70c09e..440d3bb 100644
--- a/src/copy.h
+++ b/src/copy.h
@@ -157,6 +157,7 @@ struct cp_options
bool preserve_ownership;
bool preserve_mode;
bool preserve_timestamps;
+ bool explicit_no_preserve_mode;
/* Enabled for mv, and for cp by the --preserve=links option.
If true, attempt to preserve in the destination files any
diff --git a/src/cp.c b/src/cp.c
index 6649af2..61b31af 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -783,6 +783,7 @@ cp_option_init (struct cp_options *x)
x->preserve_links = false;
x->preserve_mode = false;
x->preserve_timestamps = false;
+ x->explicit_no_preserve_mode = false;
x->preserve_security_context = false;
x->require_preserve_context = false;
x->preserve_xattr = false;
@@ -860,6 +861,7 @@ decode_preserve_arg (char const *arg, struct cp_options *x,
bool on_off)
{
case PRESERVE_MODE:
x->preserve_mode = on_off;
+ x->explicit_no_preserve_mode = !on_off;
break;
case PRESERVE_TIMESTAMPS:
@@ -889,6 +891,7 @@ decode_preserve_arg (char const *arg, struct cp_options *x,
bool on_off)
x->preserve_timestamps = on_off;
x->preserve_ownership = on_off;
x->preserve_links = on_off;
+ x->explicit_no_preserve_mode = !on_off;
if (selinux_enabled)
x->preserve_security_context = on_off;
x->preserve_xattr = on_off;
diff --git a/src/install.c b/src/install.c
index 854436a..8ea5491 100644
--- a/src/install.c
+++ b/src/install.c
@@ -275,6 +275,7 @@ cp_option_init (struct cp_options *x)
x->preserve_links = false;
x->preserve_mode = false;
x->preserve_timestamps = false;
+ x->explicit_no_preserve_mode = false;
x->reduce_diagnostics=false;
x->data_copy_required = true;
x->require_preserve = false;
diff --git a/src/mv.c b/src/mv.c
index 4f5708e..5b08fdd 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -118,6 +118,7 @@ cp_option_init (struct cp_options *x)
x->preserve_links = true;
x->preserve_mode = true;
x->preserve_timestamps = true;
+ x->explicit_no_preserve_mode= false;
x->preserve_security_context = selinux_enabled;
x->reduce_diagnostics = false;
x->data_copy_required = true;
diff --git a/tests/cp/preserve-mode.sh b/tests/cp/preserve-mode.sh
new file mode 100755
index 0000000..dc97cba
--- /dev/null
+++ b/tests/cp/preserve-mode.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+# ensure that cp's --no-preserve=mode works correctly
+
+# Copyright (C) 2002-2012 Free Software Foundation, Inc.
+
+# This program 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.
+
+# This program 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/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ cp
+
+rm -f a b c
+umask 0022
+touch a
+touch b
+chmod 600 b
+
+#regular file test
+cp --no-preserve=mode b c
+mode_a=$(ls -l a | gawk '{print $1}')
+mode_c=$(ls -l c | gawk '{print $1}')
+test "$mode_a" = "$mode_c" || fail=1
+
+rm -rf d1 d2 d3
+mkdir d1 d2
+chmod 705 d2
+
+#directory test
+cp --no-preserve=mode -r d2 d3
+mode_d1=$(ls -l d1 | gawk '{print $1}')
+mode_d3=$(ls -l d3 | gawk '{print $1}')
+test "$mode_d1" = "$mode_d3" || fail=1
+
+rm -f a b c
+touch a
+chmod 600 a
+
+#contradicting options test
+cp --no-preserve=mode --preserve=all a b
+mode_a=$(ls -l a | gawk '{print $1}')
+mode_b=$(ls -l b | gawk '{print $1}')
+test "$mode_a" = "$mode_b" || fail=1
+
+Exit $fail
diff --git a/tests/local.mk b/tests/local.mk
index 55700b5..0b6d576 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -434,6 +434,7 @@ all_tests = \
tests/cp/perm.sh \
tests/cp/preserve-2.sh \
tests/cp/preserve-link.sh \
+ tests/cp/preserve-mode.sh \
tests/cp/preserve-slink-time.sh \
tests/cp/proc-short-read.sh \
tests/cp/proc-zero-len.sh \
--
1.7.11.4
