On 26/03/10 11:53, Pádraig Brady wrote:
> I was surprised that `cp --preserve=all file.xattr /dev/shm` produced no
> warnings
> In any case, this makes the docs match the behaviour.
Actually some warnings could be output in this case so my patch is incorrect.
The xattr/selinux diagnostics from cp/mv/install are done like:
1. cp -a outputs no warnings
2. cp --preserve=all outputs all but ENOTSUP
3. cp --preserve=xattr,context outputs all errors
4. mv outputs all but ENOTSUP
5. install --preserve-context outputs all but ENOTSUP
I'm not sure about treating xattr errors differently
to other attribute preservation errors, and in
addition ENOTSUP differently from other errors.
It just seems a bit inconsistent to me.
So how about simplifying the code to match the doc change.
I.E. don't output any warnings for 2,4 & 5 above.
A diff to do that is attached:
copy.c | 63 ++++++++++++++++----------------------------------------------
copy.h | 32 +++++++++++--------------------
cp.c | 2 -
install.c | 1
mv.c | 1
5 files changed, 29 insertions(+), 70 deletions(-)
Alternatively we could make 1 behave like 2,4,5?
cheers,
Pádraig.
diff --git a/src/copy.c b/src/copy.c
index d7c0d7b..f77b5d1 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -168,31 +168,10 @@ is_ancestor (const struct stat *sb, const struct dir_list *ancestors)
return false;
}
-static bool
-errno_unsupported (int err)
-{
- return err == ENOTSUP || err == ENODATA;
-}
-
#if USE_XATTR
-static void
-copy_attr_error (struct error_context *ctx ATTRIBUTE_UNUSED,
- char const *fmt, ...)
-{
- if (!errno_unsupported (errno))
- {
- int err = errno;
- va_list ap;
-
- /* use verror module to print error message */
- va_start (ap, fmt);
- verror (0, err, fmt, ap);
- va_end (ap);
- }
-}
static void
-copy_attr_allerror (struct error_context *ctx ATTRIBUTE_UNUSED,
+copy_attr_error (struct error_context *ctx ATTRIBUTE_UNUSED,
char const *fmt, ...)
{
int err = errno;
@@ -220,32 +199,30 @@ static bool
copy_attr_by_fd (char const *src_path, int src_fd,
char const *dst_path, int dst_fd, const struct cp_options *x)
{
+ bool show_errors = x->require_preserve_xattr;
struct error_context ctx =
{
- .error = x->require_preserve_xattr ? copy_attr_allerror : copy_attr_error,
+ .error = copy_attr_error,
.quote = copy_attr_quote,
.quote_free = copy_attr_free
};
return 0 == attr_copy_fd (src_path, src_fd, dst_path, dst_fd, 0,
- (x->reduce_diagnostics
- && !x->require_preserve_xattr
- && x->data_copy_required)? NULL : &ctx);
+ show_errors ? &ctx: NULL);
}
static bool
copy_attr_by_name (char const *src_path, char const *dst_path,
const struct cp_options *x)
{
+ bool show_errors = x->require_preserve_xattr;
struct error_context ctx =
{
- .error = x->require_preserve_xattr ? copy_attr_allerror : copy_attr_error,
+ .error = copy_attr_error,
.quote = copy_attr_quote,
.quote_free = copy_attr_free
};
return 0 == attr_copy_file (src_path, dst_path, 0,
- (x->reduce_diagnostics
- && !x->require_preserve_xattr
- && x->data_copy_required) ? NULL : &ctx);
+ show_errors ? &ctx: NULL);
}
#else /* USE_XATTR */
@@ -533,7 +510,7 @@ copy_reg (char const *src_name, char const *dst_name,
security_context_t con = NULL;
if (getfscreatecon (&con) < 0)
{
- if (!x->reduce_diagnostics || x->require_preserve_context)
+ if (x->require_preserve_context)
error (0, errno, _("failed to get file system create context"));
if (x->require_preserve_context)
{
@@ -546,7 +523,7 @@ copy_reg (char const *src_name, char const *dst_name,
{
if (fsetfilecon (dest_desc, con) < 0)
{
- if (!x->reduce_diagnostics || x->require_preserve_context)
+ if (x->require_preserve_context)
error (0, errno,
_("failed to set the security context of %s to %s"),
quote_n (0, dst_name), quote_n (1, con));
@@ -1833,29 +1810,23 @@ copy_internal (char const *src_name, char const *dst_name,
{
if (setfscreatecon (con) < 0)
{
- if (!x->reduce_diagnostics || x->require_preserve_context)
- error (0, errno,
- _("failed to set default file creation context to %s"),
- quote (con));
if (x->require_preserve_context)
{
+ error (0, errno,
+ _("failed to set default file creation context to %s"),
+ quote (con));
freecon (con);
return false;
}
}
freecon (con);
}
- else
+ else if (x->require_preserve_context)
{
- if (!errno_unsupported (errno) || x->require_preserve_context)
- {
- if (!x->reduce_diagnostics || x->require_preserve_context)
- error (0, errno,
- _("failed to get security context of %s"),
- quote (src_name));
- if (x->require_preserve_context)
- return false;
- }
+ error (0, errno,
+ _("failed to get security context of %s"),
+ quote (src_name));
+ return false;
}
}
diff --git a/src/copy.h b/src/copy.h
index 8c5cab6..e1fe33a 100644
--- a/src/copy.h
+++ b/src/copy.h
@@ -184,15 +184,13 @@ struct cp_options
Set this only if the kernel is SELinux enabled. */
bool preserve_security_context;
- /* Useful only when preserve_security_context is true.
- If true, a failed attempt to preserve a file's security context
- propagates failure "out" to the caller. If false, a failure to
- preserve a file's security context does not change the invoking
- application's exit status. Give diagnostics for failed syscalls
- regardless of this setting. For example, with "cp --preserve=context"
- this flag is "true", while with "cp -a", it is false. That means
- "cp -a" attempts to preserve any security context, but does not
- fail if it is unable to do so. */
+ /* Useful only when preserve_context is true.
+ If true, a failed attempt to preserve file's security context
+ propagates failure "out" to the caller, along with full diagnostics.
+ If false, a failure to preserve file's security context does not
+ change the invoking application's exit status, or output diagnostics.
+ For example, with `cp --preserve=context` this flag is "true",
+ while with `cp --preserve=all` or `cp -a`, it is "false". */
bool require_preserve_context;
/* If true, attempt to preserve extended attributes using libattr.
@@ -201,19 +199,13 @@ struct cp_options
/* Useful only when preserve_xattr is true.
If true, a failed attempt to preserve file's extended attributes
- propagates failure "out" to the caller. If false, a failure to
- preserve file's extended attributes does not change the invoking
- application's exit status. Give diagnostics for failed syscalls
- regardless of this setting. For example, with "cp --preserve=xattr"
- this flag is "true", while with "cp --preserve=all", it is false. */
+ propagates failure "out" to the caller, along with full diagnostics.
+ If false, a failure to preserve file's extended attributes does not
+ change the invoking application's exit status, or output diagnostics.
+ For example, with `cp --preserve=xattr` this flag is "true",
+ while with `cp --preserve=all` or `cp -a`, it is "false". */
bool require_preserve_xattr;
- /* Used as difference boolean between cp -a and cp -dR --preserve=all.
- If true, non-mandatory failure diagnostics are not displayed. This
- should prevent poluting cp -a output.
- */
- bool reduce_diagnostics;
-
/* If true, copy directories recursively and copy special files
as themselves rather than copying their contents. */
bool recursive;
diff --git a/src/cp.c b/src/cp.c
index 13ce9e0..968f2c1 100644
--- a/src/cp.c
+++ b/src/cp.c
@@ -781,7 +781,6 @@ cp_option_init (struct cp_options *x)
x->preserve_security_context = false;
x->require_preserve_context = false;
x->preserve_xattr = false;
- x->reduce_diagnostics = false;
x->require_preserve_xattr = false;
x->data_copy_required = true;
@@ -956,7 +955,6 @@ main (int argc, char **argv)
if (selinux_enabled)
x.preserve_security_context = true;
x.preserve_xattr = true;
- x.reduce_diagnostics = true;
x.recursive = true;
break;
diff --git a/src/install.c b/src/install.c
index bef7f8a..31dee86 100644
--- a/src/install.c
+++ b/src/install.c
@@ -281,7 +281,6 @@ cp_option_init (struct cp_options *x)
x->preserve_links = false;
x->preserve_mode = false;
x->preserve_timestamps = false;
- x->reduce_diagnostics=false;
x->data_copy_required = true;
x->require_preserve = false;
x->require_preserve_context = false;
diff --git a/src/mv.c b/src/mv.c
index 91aadfa..e02ed57 100644
--- a/src/mv.c
+++ b/src/mv.c
@@ -118,7 +118,6 @@ cp_option_init (struct cp_options *x)
x->preserve_mode = true;
x->preserve_timestamps = true;
x->preserve_security_context = selinux_enabled;
- x->reduce_diagnostics = false;
x->data_copy_required = true;
x->require_preserve = false; /* FIXME: maybe make this an option */
x->require_preserve_context = false;
