Stream: git-wasmtime

Topic: wasmtime / PR #1725 Revert fstatat on *nix and test symli...

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:17):

kubkon opened PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:31):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:47):

kubkon requested sunfishcode for a review on PR #1725.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:48):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:48):

sunfishcode created PR Review Comment:

It's confusing, because there's both AT_SYMLINK_FOLLOW and AT_SYMLINK_NOFOLLOW, and they're both non-zero flags, because some system calls default to following by default, while others don't. It looks like fstatat defaults to following symbolic links, so it looks like you should need to pass AtFlag::SYMLINK_NOFOLLOW here.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:48):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:48):

sunfishcode created PR Review Comment:

Same as fstatat above.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:48):

sunfishcode created PR Review Comment:

Same as above wrt atim.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:48):

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.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:48):

sunfishcode created PR Review Comment:

I'm unclear on what this is testing. new_mtim is stat.mtim - 100 from above. sym_stat.mtim is from the stat we just got after setting the time to sym_new_mtim which is sym_stat.mtim - 200. It's not clear why those should be equal. Also, the message here "mtim should change" doesn't seem to fit using assert_eq to check that two things are the same.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:52):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:52):

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 to fstatat here as otherwise we'll end up with EINVAL since path will always be resolved to an actual path by now. Symlink dereferencing happens in path::get call level above. Does that make sense?

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:55):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 15:55):

kubkon created PR Review Comment:

Oh you're right! It was meant to be sys_new_mtim rather than new_mtim. Now it makes we wonder why it passed the tests at all.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:00):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:00):

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.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:20):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:20):

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 that fstatat always dereferences unless we specify AT_SYMLINK_NOFOLLOW.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:21):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:21):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:21):

kubkon created PR Review Comment:

Makes sense! I've addressed that in the latest commit.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:21):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:21):

kubkon created PR Review Comment:

OK, I've fixed it now in the latest commit.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:23):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 16:52):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 18:17):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 19 2020 at 18:19):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2020 at 15:06):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2020 at 15:23):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2020 at 16:33):

kubkon requested sunfishcode for a review on PR #1725.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2020 at 17:03):

kubkon updated PR #1725 from path-filestat to master:

This commit effectively reverts too eager refactoring on my part which
resulted in incorrect path_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 the path_filestat test cases with symlink checks as well.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2020 at 19:02):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2020 at 19:02):

sunfishcode merged PR #1725.

Last updated: Nov 22 2024 at 16:03 UTC