a user pointed out that `cp -p` failed to preserve times on a nfs mount:
http://bugs.gentoo.org/132673

this is because changing ownership on a nfs mount updates the ctimes:
touch foo
# stat foo
  File: `foo'
  Size: 0               Blocks: 0          IO Block: 16384  regular empty file
Device: eh/14d  Inode: 584188      Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (  250/ portage)
Access: 2006-05-08 22:34:13.621879904 -0400
Modify: 2006-05-08 22:34:13.621879904 -0400
Change: 2006-05-08 22:34:13.621879904 -0400
# chown 1 foo
# stat foo
  File: `foo'
  Size: 0               Blocks: 0          IO Block: 16384  regular empty file
Device: eh/14d  Inode: 584188      Links: 1
Access: (0644/-rw-r--r--)  Uid: (    1/     bin)   Gid: (  250/ portage)
Access: 2006-05-08 22:34:13.621879904 -0400
Modify: 2006-05-08 22:34:13.621879904 -0400
Change: 2006-05-08 22:34:21.628662688 -0400

i dont know if POSIX has an opinion on the order of operations, but SUSv3 
doesnt care:
http://www.opengroup.org/onlinepubs/009695399/utilities/cp.html
...
-p Duplicate the following characteristics of each source file in the 
corresponding destination file:
...
The order in which the preceding characteristics are duplicated is 
unspecified.

so would there be any real detrimental effects with the attached patch ? 
clearing the setid bits wouldnt be affected by changing of timestamps, so 
doing it after the chown() should be fine ...
-mike
2006-05-08  Mike Frysinger  <[EMAIL PROTECTED]>

	src/copy.c (copy_reg): Move preserve_timestamps logic last.
	(copy_internal): Likewise.

--- src/copy.c
+++ src/copy.c
@@ -483,23 +483,6 @@ copy_reg (char const *src_name, char con
 	}
     }
 
-  if (x->preserve_timestamps)
-    {
-      struct timespec timespec[2];
-      timespec[0] = get_stat_atime (src_sb);
-      timespec[1] = get_stat_mtime (src_sb);
-
-      if (futimens (dest_desc, dst_name, timespec) != 0)
-	{
-	  error (0, errno, _("preserving times for %s"), quote (dst_name));
-	  if (x->require_preserve)
-	    {
-	      return_val = false;
-	      goto close_src_and_dst_desc;
-	    }
-	}
-    }
-
   if (x->preserve_ownership && ! SAME_OWNER_AND_GROUP (*src_sb, sb))
     {
       if (! set_owner (x, dst_name, dest_desc, src_sb->st_uid, src_sb->st_gid))
@@ -523,6 +506,23 @@ copy_reg (char const *src_name, char con
 	return_val = false;
     }
 
+  if (x->preserve_timestamps)
+    {
+      struct timespec timespec[2];
+      timespec[0] = get_stat_atime (src_sb);
+      timespec[1] = get_stat_mtime (src_sb);
+
+      if (futimens (dest_desc, dst_name, timespec) != 0)
+	{
+	  error (0, errno, _("preserving times for %s"), quote (dst_name));
+	  if (x->require_preserve)
+	    {
+	      return_val = false;
+	      goto close_src_and_dst_desc;
+	    }
+	}
+    }
+
 close_src_and_dst_desc:
   if (close (dest_desc) < 0)
     {
@@ -1727,24 +1727,9 @@ copy_internal (char const *src_name, cha
      the destination must not be removed.
      FIXME: implement the above. */
 
-  /* Adjust the times (and if possible, ownership) for the copy.
-     chown turns off set[ug]id bits for non-root,
+  /* chown turns off set[ug]id bits for non-root,
      so do the chmod last.  */
 
-  if (x->preserve_timestamps)
-    {
-      struct timespec timespec[2];
-      timespec[0] = get_stat_atime (&src_sb);
-      timespec[1] = get_stat_mtime (&src_sb);
-
-      if (utimens (dst_name, timespec) != 0)
-	{
-	  error (0, errno, _("preserving times for %s"), quote (dst_name));
-	  if (x->require_preserve)
-	    return false;
-	}
-    }
-
   /* Avoid calling chown if we know it's not necessary.  */
   if (x->preserve_ownership
       && (new_dst || !SAME_OWNER_AND_GROUP (src_sb, dst_sb)))
@@ -1777,6 +1762,21 @@ copy_internal (char const *src_name, cha
 	}
     }
 
+  /* Adjust the times (and if possible, ownership) for the copy.  */
+  if (x->preserve_timestamps)
+    {
+      struct timespec timespec[2];
+      timespec[0] = get_stat_atime (&src_sb);
+      timespec[1] = get_stat_mtime (&src_sb);
+
+      if (utimens (dst_name, timespec) != 0)
+	{
+	  error (0, errno, _("preserving times for %s"), quote (dst_name));
+	  if (x->require_preserve)
+	    return false;
+	}
+    }
+
   return delayed_ok;
 
 un_backup:
_______________________________________________
Bug-coreutils mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/bug-coreutils

Reply via email to