Date: Sat, 28 Jun 2014 21:31:19 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jilles Tjoelker <jilles@stack.nl> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Xin LI <delphij@freebsd.org> Subject: Re: svn commit: r267977 - head/bin/mv Message-ID: <20140628172421.U1406@etaplex.bde.org> In-Reply-To: <20140627222323.GA43131@stack.nl> References: <201406271957.s5RJvs6j074326@svn.freebsd.org> <20140627222323.GA43131@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140628172421.U1406>