svn commit: r267977 - head/bin/mv

[Bruce Evans]

[My mail connection wasn't working back in June when I wrote this.  This
is the first of many replies to try to prevent breakage of mv.

I have now checked what happens for simple tests on ref11.  Details in
later replies.]

On Sat, 28 Jun 2014, Bruce Evans wrote:

> On Sat, 28 Jun 2014, Jilles Tjoelker wrote:
>
>> On Fri, Jun 27, 2014 at 07:57:54PM +0000, Xin LI wrote:
>>> Author: delphij
>> 
>>> Log:
>>>   Always set UF_ARCHIVE on target (because they are by definition new 
>>> files
>>>   and should be archived) and ignore error when we can't set it (e.g. 
>>> NFS).
>> 
>>>   Reviewed by:	ken
>>>   MFC after:	2 weeks
>> 
>>> Modified:
>>>   head/bin/mv/mv.c
>> 
>>> Modified: head/bin/mv/mv.c
>>> ==============================================================================
>>> --- head/bin/mv/mv.c	Fri Jun 27 19:50:30 2014	(r267976)
>>> +++ head/bin/mv/mv.c	Fri Jun 27 19:57:54 2014	(r267977)
>>> @@ -337,8 +337,8 @@ err:		if (unlink(to))
>>>  	 * on a file that we copied, i.e., that we didn't create.)
>>>  	 */
>>>  	errno = 0;
>>> -	if (fchflags(to_fd, sbp->st_flags))
>>> -		if (errno != EOPNOTSUPP || sbp->st_flags != 0)
>>> +	if (fchflags(to_fd, sbp->st_flags | UF_ARCHIVE))
>>> +		if (errno != EOPNOTSUPP || ((sbp->st_flags & ~UF_ARCHIVE) != 
>>> 0))
>>>  			warn("%s: set flags (was: 0%07o)", to, 
>>> sbp->st_flags);
>>> 
>>>  	tval[0].tv_sec = sbp->st_atime;
>> 
>> The part ignoring failures to set UF_ARCHIVE is OK.
>
> No, it is not OK.  The error was only a warning, and that is the best
> possible.

Now I'm not sure about this.  The same warning for the usual case where
cp -pR is used becomes fatal for mv:

@ pts/9:bde at freefall:~> cd /tmp
@ pts/9:bde at freefall:/tmp> mkdir bde
@ pts/9:bde at freefall:/tmp> mv bde ~/z
@ mv: rename bde to /home/bde/z: Not a directory
@ pts/9:bde at freefall:/tmp> mv bde ~/z1
@ mv: chflags: /home/bde/z1: Operation not supported
@ mv: /bin/cp bde /home/bde/z1: terminated with 1 (non-zero) status

After cp exits with nonzero status, mv cannot remove the source
directory and a mess results.

It is more reasonable for cp -p to ignore this error, but it is mv
that doesn't allow the error to affect its exit status.  POSIX has
detailed rules for mv, and IIRC not so detailed rules for cp -p.
POSIX doesn't allow implementing mv across file systems using cp -pR
like the FreeBSD implementation does, except possibly if cp -pR follows
exactly the same rules as mv and rules followed don't conflict with
the POSIX rules.

>> However, it seems
>> inconsistent to set UF_ARCHIVE on a cross-filesystem mv of a single
>> file, but not on a cross-filesystem mv of a directory tree
>
> It is also inconsistent with within-filesystem mv's in both cases.
>
>> or a file
>> newly created via shell output redirection.
>
> The file system should set it in that case, if the file system actually
> supports UF_ARCHIVE.
>
>> If UF_ARCHIVE is supposed to be set automatically, I think this should
>> be done in the kernel, like msdosfs already does.
>
> zfs sets it too.  That's where the problematic UF_ARCHIVE settings
> come from.  The problem was just less visible for msdsofs since it
> is less used for critical file systems.
>
> It used to be an even larger problem for msdosfs.  msdosfs's archive
> flag was mapped to SF_ARCHIVE instead of to UF_ARCHIVE.  So for mv
> or cp -p from msdsofs to msdosfs (or a similar file system), you
> could get an EPERM error.  The above hack only checks for
> EOPNOTSUPP, so it made no difference.
>
> When the target file system actually supports UF_ARCHIVE, the target
> file already has UF_ARCHIVE set because the file is new.  Then the
> chflags() is needed mainly to unset UF_ARCHIVE when the source file
> doesn't have it set.  The change does the opposite.
>
> The cp -pR case (for mv across file systems and cp itself) never even
> had the EOPNOTSUPP hack (except possibly in recent versions which I
> can't check now), so it tends to spew errors.  Old versions of cp -p
> did the following:
>
> @ 	/*
> @ 	 * Changing the ownership probably won't succeed, unless we're root
> @ 	 * or POSIX_CHOWN_RESTRICTED is not set.  Set uid/gid before setting
> @ 	 * the mode; current BSD behavior is to remove all setuid bits on
> @ 	 * chown.  If chown fails, lose setuid/setgid bits.
> @ 	 */
> @ 	if (!gotstat || fs->st_uid != ts.st_uid || fs->st_gid != ts.st_gid)
>
> This avoids most chown()s by not attempting any.  Good.
>
> @ 		if (fdval ? fchown(fd, fs->st_uid, fs->st_gid) :
> @ 		    (islink ? lchown(to.p_path, fs->st_uid, fs->st_gid) :
> @ 		    chown(to.p_path, fs->st_uid, fs->st_gid))) {
>
> Here it would be better to stat() the file again and mostly not use the
> syscall result.  syscalls that can set multiple attributes should allow
> setting subsets and require checking to see which ones were set.
> tcsettattr() is such a syscall.  It has the very bad error handling of
> returning success if at least 1 attribute was set.  That is bad because
> it is fail-unsafe for sloppy callers, and its success is guaranteed
> since there are always attributes which can be set to an unchanged
> value.  chown() is not such a syscall, but it is safer to check.
>
> See the XXX comment before the above code in mv.  It is about
> (mis)handling settable subsets of flags.  Currently there is no
> reasonable way, since chflags() is like chown() and has to accept all
> of the settings or not change any.
>
> @ 			if (errno != EPERM) {
> @ 				warn("chown: %s", to.p_path);
> @ 				rval = 1;
> @ 			}
> @ 			fs->st_mode &= ~(S_ISUID | S_ISGID);
> @ 		}
> @ @ 	if (!gotstat || fs->st_mode != ts.st_mode)
> @ 		if (fdval ? fchmod(fd, fs->st_mode) :
> @ 		    (islink ? lchmod(to.p_path, fs->st_mode) :
> @ 		    chmod(to.p_path, fs->st_mode))) {
> @ 			warn("chmod: %s", to.p_path);
> @ 			rval = 1;
> @ 		}
>
> Similarly for the mode, except not making null changes is closer to
> being just an optimization.
>
> Hmm, This order seems to be backwards.  Shouldn't we change the mode
> before the ownerships go keep more permission for changing the mode?
> We already change file flags last in case an immutable flag will be set.
>
> @ @ 	if (!gotstat || fs->st_flags != ts.st_flags)
>
> Avoiding null changes for file flags avoids permissions and support
> problems in the usual case where the file flags are 0.  However, for
> zfs and now msdosfs, this is now the unusual case -- the source file
> flags are usually UF_ARCHIVE.  (For msdosfs, the archive flag used to
> be SF_ARCHIVE, but this was inverted so the usual case for a new file
> was <msdosfs archive flag set> mapped to <SF_ARCHIVE clear>.  But
> backing up under WinDOS normally clears the archive flag, so if it
> is done then the usual case was <msdosfs archive flag clear> mapped
> to <SF_ARCHIVE set>.)
>
> @ 		if (fdval ?
> @ 		    fchflags(fd, fs->st_flags) :
> @ 		    (islink ? (errno = ENOSYS) :
> @ 		    chflags(to.p_path, fs->st_flags))) {
> @ 			warn("chflags: %s", to.p_path);
> @ 			rval = 1;
> @ 		}
>
> Note that any error here causes the exit status to be 1, while for
> fastcopy() in mv, no fchflags() error causes the exit status to be 1,
> and warnings are also suppressed for some EOPNOTSUPP errors.  Very
> inconsistent.  fastcopy() also never sets the exit status to 1 for
> errors in fchown(), fchmod() or utimes().  The utimes() in it is
> also broken by being placed after the fchflags(), so it is certain
> to fail if fchflags() set any immutable flag.  cp is careful to
> avoid this bug.
>
> POSIX specifies most of this: mv: "If the duplication of the file
> characteristics fails for any reason, then mv shall write a diagnostic
> message to stderr, but this failure shall not cause mv to modify its
> exit status".  cp -p: only duplicating the times, uid, gid and certain
> mode bits is required; a diagnostic is required for failure to duplicate
> the times or certain mode bits is required; a diagnostic is optional
> for failure to duplicate uid or gid; the exit status is not mentioned.
> FreeBSD cp -p always modifies the exit status and prints a diagnostic
> except for EPERM errors from chown().
>
> For all attributes, modifying the exit status in cp -pR makes FreeBSD
> cp -pR unusable for the mv across file systems that it is used for.
> cp -pR has other bugs like snapping hard links and not preserving
> directory times, but I still trust it more than fastcopy().
>
>> However, I'm not sure
>> this is actually a useful feature: backup programs are smarter than an
>> archive attribute these days.
>
> I mostly use the ctime to decide which files need backing up, but the
> archive bit would have some advantage if backup programs and system
> utilities actually supported it.  It can be controlled by utilities,
> unlike ctimes.  msdosfs utilities had good support for it 25-30 years
> ago (things like an option in all copying utilities to clear the archive
> bit in the source after copying, so that all copying utilities can be
> used to back up), but I never actually used this until recently.  I
> don't see how a backup utility could get equivalent functionality
> without maintaining large databases of files backed up and having bits
> like the archive bit in the database, and sub-utilities to manage the
> bits.
>
> Bruce

Bruce