From owner-freebsd-current@FreeBSD.ORG Tue Jul 1 21:35:45 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 943ECB8A; Tue, 1 Jul 2014 21:35:45 +0000 (UTC) Received: from mx1.stack.nl (relay04.stack.nl [IPv6:2001:610:1108:5010::107]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client CN "mailhost.stack.nl", Issuer "CA Cert Signing Authority" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 25D28215B; Tue, 1 Jul 2014 21:35:45 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id D6DC5B801F; Tue, 1 Jul 2014 23:35:42 +0200 (CEST) Received: by snail.stack.nl (Postfix, from userid 1677) id BF4EE28497; Tue, 1 Jul 2014 23:35:42 +0200 (CEST) Date: Tue, 1 Jul 2014 23:35:42 +0200 From: Jilles Tjoelker To: d@delphij.net Subject: Re: svn commit: r267977 - head/bin/mv Message-ID: <20140701213542.GC18628@stack.nl> References: <201406271957.s5RJvs6j074326@svn.freebsd.org> <20140627222323.GA43131@stack.nl> <53ADF90D.2030707@delphij.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53ADF90D.2030707@delphij.net> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: "Kenneth D. Merry" , FreeBSD Current , Xin LI X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Jul 2014 21:35:45 -0000 On Fri, Jun 27, 2014 at 04:06:53PM -0700, Xin Li wrote: > [moving discussion to freebsd-current@] > 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) > >> @@ -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. 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) > @@ -278,6 +278,7 @@ 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); > @@ -329,6 +330,7 @@ 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 > @@ -336,10 +338,19 @@ 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