Re: svn commit: r267977 - head/bin/mv

On Fri, Jun 27, 2014 at 04:06:53PM -0700, Xin Li wrote:
> [moving discussion to freebsd-current_at_]

> On 06/27/14 15:23, Jilles Tjoelker wrote:
> > On Fri, Jun 27, 2014 at 07:57:54PM +0000, Xin LI wrote:
> >> Author: delphij
> >> Date: Fri Jun 27 19:57:54 2014
> >> New Revision: 267977
> >> URL: http://svnweb.freebsd.org/changeset/base/267977

> >> 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)
> >> _at__at_ -337,8 +337,8 _at__at_ 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. 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 or a file
> > newly created via shell output redirection.

> > If UF_ARCHIVE is supposed to be set automatically, I think this should
> > be done in the kernel, like msdosfs already does. However, I'm not sure
> > this is actually a useful feature: backup programs are smarter than an
> > archive attribute these days.

> The flag is supposed to be set automatically (as my understanding of the
> ZFS portion of implementation).

OK, I did not know that ZFS set UF_ARCHIVE automatically. If so, blindly
copying st_flags like the old code did is indeed suboptimal.

> However in order to implement that way, we will have to stat() the
> target file (attached).  Personally, I think this is a little bit
> wasteful, but it would probably something that we have to do if we
> implement a switch to turn off automatic UF_ARCHIVE behavior.

This seems better. For example, it avoids UF_ARCHIVE spreading around
randomly on UFS.

Nits: the errno = 0 assignment seems unnecessary, "can not" should be
"cannot".

> Index: bin/mv/mv.c
> ===================================================================
> --- bin/mv/mv.c	(revision 267984)
> +++ bin/mv/mv.c	(working copy)
> _at__at_ -278,6 +278,7 _at__at_ fastcopy(const char *from, const char *to, struct
>  	static char *bp = NULL;
>  	mode_t oldmode;
>  	int nread, from_fd, to_fd;
> +	struct stat to_sb;
>  
>  	if ((from_fd = open(from, O_RDONLY, 0)) < 0) {
>  		warn("fastcopy: open() failed (from): %s", from);
> _at__at_ -329,6 +330,7 _at__at_ err:		if (unlink(to))
>  	 */
>  	preserve_fd_acls(from_fd, to_fd, from, to);
>  	(void)close(from_fd);
> +
>  	/*
>  	 * XXX
>  	 * NFS doesn't support chflags; ignore errors unless there's reason
> _at__at_ -336,10 +338,19 _at__at_ err:		if (unlink(to))
>  	 * if the server supports flags and we were trying to *remove* flags
>  	 * on a file that we copied, i.e., that we didn't create.)
>  	 */
> -	errno = 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);
> +	if (fstat(to_fd, &to_sb) == 0) {
> +		if ((sbp->st_flags  & ~UF_ARCHIVE) !=
> +		    (to_sb.st_flags & ~UF_ARCHIVE)) {
> +			errno = 0;
> +			if (fchflags(to_fd,
> +			    sbp->st_flags | (to_sb.st_flags & UF_ARCHIVE)))
> +				if (errno != EOPNOTSUPP ||
> +				    ((sbp->st_flags & ~UF_ARCHIVE) != 0))
> +					warn("%s: set flags (was: 0%07o)",
> +					    to, sbp->st_flags);
> +		}
> +	} else
> +		warn("%s: can not stat", to);
>  
>  	tval[0].tv_sec = sbp->st_atime;
>  	tval[1].tv_sec = sbp->st_mtime;

-- 
Jilles Tjoelker