From owner-svn-src-all@FreeBSD.ORG Sat Jun 28 11:32:03 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 3F36C566; Sat, 28 Jun 2014 11:32:03 +0000 (UTC) Received: from extdelivery1.dodo.com.au (extdelivery1.dodo.com.au [123.2.6.232]) by mx1.freebsd.org (Postfix) with ESMTP id BF6642F3B; Sat, 28 Jun 2014 11:32:02 +0000 (UTC) Received: from extspam2.internal (extspam2.internal [10.20.4.19]) by extdelivery1.dodo.com.au (Postfix) with ESMTP id 8B78320A9F; Sat, 28 Jun 2014 21:31:59 +1000 (EST) Received: from extrelay0.dodo.com.au (extrelay0.internal [10.20.4.23]) by extspam2.internal (Postfix) with ESMTP id 178F22312D; Sat, 28 Jun 2014 21:31:31 +1000 (EST) Received: from etaplex.bde.org (unknown [139.218.225.48]) by extrelay0.dodo.com.au (Postfix) with ESMTPS id C3734203B3; Sat, 28 Jun 2014 21:31:29 +1000 (EST) Date: Sat, 28 Jun 2014 21:31:19 +1000 (EST) From: Bruce Evans X-X-Sender: bde@etaplex.bde.org To: Jilles Tjoelker Subject: Re: svn commit: r267977 - head/bin/mv In-Reply-To: <20140627222323.GA43131@stack.nl> Message-ID: <20140628172421.U1406@etaplex.bde.org> References: <201406271957.s5RJvs6j074326@svn.freebsd.org> <20140627222323.GA43131@stack.nl> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Dodo-MailScanner-ID: 178F22312D.35A35 X-Dodo-MailScanner: Found to be clean X-Dodo-MailScanner-From: brde@optusnet.com.au X-Spam-Status: No Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Xin LI X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Jun 2014 11:32:03 -0000 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 mapped to . But backing up under WinDOS normally clears the archive flag, so if it is done then the usual case was mapped to .) @ 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