Date: Thu, 21 Aug 2014 15:02:57 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Xin LI <delphij@freebsd.org> Subject: Re: svn commit: r268129 - head/bin/mv Message-ID: <20140821144734.B1461@besplex.bde.org> In-Reply-To: <20140702130950.N637@etaplex.bde.org> References: <201407012246.s61MkdQc049307@svn.freebsd.org> <20140702130950.N637@etaplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[My mail connection wasn't working back in June when I wrote this. This
is the last of many old replies to try to prevent breakage of mv. The
second of the old replies was labeled as the first again. This reply
adds new details]
On Wed, 2 Jul 2014, Bruce Evans wrote:
> On Tue, 1 Jul 2014, Xin LI wrote:
>
>> Log:
>> Check if fchflags() is needed by fstat'ing before and check
>> the results.
>>
>> Reviewed by: jilles
>> X-MFC-With: r267977
>
> This keeps getting worse. It now does extra work (with style bugs)
> to check one of the broken cases and try harder to keep it broken.
>
>> Modified: head/bin/mv/mv.c
>> ==============================================================================
>> --- head/bin/mv/mv.c Tue Jul 1 22:42:53 2014 (r268128)
>> +++ head/bin/mv/mv.c Tue Jul 1 22:46:39 2014 (r268129)
>> @@ -278,6 +278,7 @@ fastcopy(const char *from, const char *t
>> static char *bp = NULL;
>> mode_t oldmode;
>> int nread, from_fd, to_fd;
>> + struct stat tsb;
>
> Style bug.
>
>>
>> if ((from_fd = open(from, O_RDONLY, 0)) < 0) {
>> warn("fastcopy: open() failed (from): %s", from);
>> @@ -336,10 +337,18 @@ 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.)
>> */
>
> The code keeps rotting further away from the comment. The last sentence
> in it is about being wrong when the "server" (really, any target file
> system) sets flags that aren't set in the source file. We now do extra
> work to try harder to break this for the UF_ARCHIVE flag.
>
>> - 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, &tsb) == 0) {
>> + if ((sbp->st_flags & ~UF_ARCHIVE) !=
>> + (tsb.st_flags & ~UF_ARCHIVE)) {
>
> This sometimes (when the other flags are identical) skips fchflags()
> to explicitly break clearing of UF_ARCHIVE on the target file when it
> is set in the target but not in the source. In this case, the
> fchflags() to clear it should work so there was no bug until recently.
> It also skips fchflags() in some cases where UF_ARCHIVE is set in the
> source but not in the target. This case was previously broken a little
> differently. The previous brokenness is retained in the fchlags()
> error handling for the case where fchflags() is called in an attempt
> to change other flags.
>
> The above is a verbose way of writing the flags test. Tests for
> equality of a subset of flags are best written using the xor operator:
>
> if ((sbp->st_flags ^ tsb.st_flags) & ~UF_ARCHIVE) {
>
> or in KNF verboseness:
>
> if (((sbp->st_flags ^ tsb.st_flags) & ~UF_ARCHIVE) != 0) {
>
>> + if (fchflags(to_fd,
>> + sbp->st_flags | (tsb.st_flags & UF_ARCHIVE)))
>
> This retains the brokenness of forcing the UF_ARCHIVE on in the target
> to ensure breaking it in the case that it is off in the source. This
> is is really silly now. We only reach here if the flags other than
> UF_ARCHIVE differ. The target UF_ARCHIVE is now not clobbered in the
> usual case where all the flags except possibly UF_ARCHIVE are clear
> in both the source and the target, but it is also not cleared when it
> should be in this case. It is now forcibly set in other cases, to
> retain the brokenness when it should be cleared.
>
>> + if (errno != EOPNOTSUPP ||
>> + ((sbp->st_flags & ~UF_ARCHIVE) != 0))
>> + warn("%s: set flags (was: 0%07o)",
>> + to, sbp->st_flags);
>
> This error handling to break the warning is much the same as before,
> but sillier. Now we only get here if there are flags other than
> UF_ARCHIVE to change. If errno == EOPNOTSUPP, then the whole syscall
> failed so it was impossible to change the other flags. So errno ==
> EOPNOTSUPP implies an error that it just as fatal as errno !=
> EOPNOTSUPP, and any flags test after it is redundant or broken. The
> correct redundant test is that the flags are the same, but we already
> know that they differ and fchflags() has the bug of changing some but
> failing. The current broken test is that all the source flags except
> UF_ARCHIVE are the same. These tests only give different results if
> the target has some flag other than UF_ARCHIVE set, since even an
> unsupported chflags() should result in mostly-zero flags. There are
> currently no such flags, but mv shouldn't depend on this.
>
>> + }
>> + } else
>> + warn("%s: cannot stat", to);
>
> The error handling for fstat() is laborious and silly. We just opened
> the file, so fstat() can't fail. If it somehow fails, then the following
> close() of the file should also fail and generate a fatal error.
>
>>
>> tval[0].tv_sec = sbp->st_atime;
>> tval[1].tv_sec = sbp->st_mtime;
>
> I just noticed that fastcopy() is even more broken than first appeared.
> Unlike cp -pR, it still hasn't caught up with the reason for existence
> of utimes() (4.2BSD, 1983) -- just after here, it uses utimes(), but
> defeats it by forcing tv_usec to 0 in the target.
I have now checked what happens for simple tests on ref11. The first
bug observed was that mv now fails to preserve UF_ARCHIVE even when
both the source and target file systems support it. mv still works
correctly and reports failure to preserve the gid (due to the source
file being on /tmp so the gids are unlikely to match (one was wheel
and the other was bde). I rarely see this warning on my local file
systems since I don't use gid bde and almost everything has gid wheel).
Also checked:
cp -pR wasn't changed, so it UF_ARCHIVE still causes zillions of warnings
for it, but it doesn't fail to preserve the flag if possible.
The usual case of mv, where the move is of a directory tree, still uses
cp -pR so it is unaffected by the changes.
Old versions of mv on old versions of FreeBSD that don't have UF_ARCHIVE
still work as correctly as possible for SF_ARCHIVE and all other flags.
SF_ARCHIVE is privileged and not supported by zfs IIRC, so it is harder
to test and rarely set.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140821144734.B1461>