[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.cTue Jul 1 22:42:53 2014(r268128)
+++ head/bin/mv/mv.cTue 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 =
