Subject: bug#5775: [PATCH] doc: fix info to say cp -a does _not_ diagnose xattr failures

From: Pádraig Brady <[email-masked]>
Date: Sat, 10 Apr 2010 00:52:48 +0100

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;