kubkon opened PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon requested sunfishcode for a review on PR #1725.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
It's confusing, because there's both
AT_SYMLINK_FOLLOW
andAT_SYMLINK_NOFOLLOW
, and they're both non-zero flags, because some system calls default to following by default, while others don't. It looks likefstatat
defaults to following symbolic links, so it looks like you should need to passAtFlag::SYMLINK_NOFOLLOW
here.
sunfishcode submitted PR Review.
sunfishcode created PR Review Comment:
Same as
fstatat
above.
sunfishcode created PR Review Comment:
Same as above wrt
atim
.
sunfishcode created PR Review Comment:
I realize this is copied from other tests in this file, but the
atim
field is effectively volatile; it could change depending on factors outside the control of this test, which could cause this check to fail, so unfortunately I think we should omit these checks.
sunfishcode created PR Review Comment:
I'm unclear on what this is testing.
new_mtim
isstat.mtim - 100
from above.sym_stat.mtim
is from the stat we just got after setting the time tosym_new_mtim
which issym_stat.mtim - 200
. It's not clear why those should be equal. Also, the message here "mtim should change" doesn't seem to fit usingassert_eq
to check that two things are the same.
kubkon submitted PR Review.
kubkon created PR Review Comment:
Oh, sorry, I meant
AT_SYMLINK_NOFOLLOW
in the comment above. I don't think we should specify any flags in the call tofstatat
here as otherwise we'll end up withEINVAL
sincepath
will always be resolved to an actual path by now. Symlink dereferencing happens inpath::get
call level above. Does that make sense?
kubkon submitted PR Review.
kubkon created PR Review Comment:
Oh you're right! It was meant to be
sys_new_mtim
rather thannew_mtim
. Now it makes we wonder why it passed the tests at all.
kubkon submitted PR Review.
kubkon created PR Review Comment:
Hmm, on second thought, we should pass flags here indeed. I think there some other subtlety going on here that dereferences the symlink too far, or something like that. I'll need to look closer and see what's happening here under the hood.
kubkon submitted PR Review.
kubkon created PR Review Comment:
OK, you're absolutely right we should pass appropriate flags to
fstatat
. I misread what you wrote (I guess tiredness is winning today), and missed the fact thatfstatat
always dereferences unless we specifyAT_SYMLINK_NOFOLLOW
.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon submitted PR Review.
kubkon created PR Review Comment:
Makes sense! I've addressed that in the latest commit.
kubkon submitted PR Review.
kubkon created PR Review Comment:
OK, I've fixed it now in the latest commit.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
kubkon requested sunfishcode for a review on PR #1725.
kubkon updated PR #1725 from path-filestat
to master
:
This commit effectively reverts too eager refactoring on my part which
resulted in incorrectpath_filestat_{get, set_times}
behaviour on
*nix hosts. In the presence of symlinks, neither of the calls would
work properly.In order to shield ourselves from similar errors in the future, I've
augmented thepath_filestat
test cases with symlink checks as well.
sunfishcode submitted PR Review.
sunfishcode merged PR #1725.
Last updated: Dec 23 2024 at 12:05 UTC