Replace use of readdir()/dirent.d_type with stat()/st_mode. #1874
Conversation
d_type is documented as only working with some filesystems, and that all programs must handle a value of DT_UNKNOWN and make a stat() call to retrieve the full information. To keep the code simpler, always make the stat() call. Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
| struct stat stat_result; | ||
| int rc = ::stat(full_path.c_str(), &stat_result); | ||
| if (rc != 0) { | ||
| throw EnvoyException(fmt::format("unable to stat file: '{}'", full_path)); |
There was a problem hiding this comment.
Can we add a test for this case? Perhaps move to utility function so can test with a bogus path?
There was a problem hiding this comment.
I can add some test coverage if I move stat() to OsSysCalls.
I think in the real world, the only way this fails is if someone deletes/moves the file after readdir() has returned it but before we stat() it.
There was a problem hiding this comment.
Either that or make it RELEASE_ASSERT. Up to you.
There was a problem hiding this comment.
(I test hack would be to just move the stat call to a small helper, and then pass in a bad file name, and I think it should fail and throw).
| @@ -72,12 +75,20 @@ class RuntimeImplTest : public testing::Test { | |||
| EXPECT_CALL(dispatcher, createFilesystemWatcher_()) | |||
| .WillOnce(ReturnNew<NiceMock<Filesystem::MockWatcher>>()); | |||
|
|
|||
| if (os_sys_calls_ == nullptr) { | |||
There was a problem hiding this comment.
Does setup actually get called more than once in a test? Otherwise I would just ASSERT it's nullptr or just let it leak and fail the test.
There was a problem hiding this comment.
No, that was my hack so I could create this and set an EXPECT_CALL before this function creates the LoaderImpl. A cleaner way to do it would be to have a setup() and run() function, and insert my EXPECT_CALL between the two. Want me to do that?
There was a problem hiding this comment.
I would just remove the if check. We do this all over the place in tests. If it leaks the leak checker will find it.
There was a problem hiding this comment.
Oh, nevermind. I see. Yeah I would switch to setup() and run() if you don't mind.
Signed-off-by: Greg Greenway <ggreenway@apple.com>
| if (recursive && entry->d_type == DT_DIR && std::string(entry->d_name) != "." && | ||
| struct stat stat_result; | ||
| int rc = ::stat(file_name.c_str(), &stat_result); | ||
| EXPECT_EQ(rc, 0); |
There was a problem hiding this comment.
Nit: I would make this ASSERT_EQ, since if this fails, the code below is using junk.
Signed-off-by: Greg Greenway <ggreenway@apple.com>
…tions Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
#1874) Description: When running on iOS 12 or later, use the more modern and more featureful alternative to `SCNetworkReachability` from the Network framework. Risk Level: Medium. It's likely that this new approach to observing network reachability will be more correct and reliable than the `SCNetworkReachability` approach, but it's still possible for differences in behavior to cause issues where the wrong radio type is selected on devices running iOS 12 or later. To mitigate, the new path monitor is opt-in only. Testing: I've tested this on a physical iPhone, confirming that the logs about switching preferred network matches when either the reachability or path monitor mechanisms are used. Unit tests cover that appending `.enableNetworkPathMonitor()` to the builder commands flips the `useNetworkPathMonitor` flag to true. Docs Changes: Added. Release Notes: Added. Signed-off-by: JP Simard <jp@jpsim.com>
#1874) Description: When running on iOS 12 or later, use the more modern and more featureful alternative to `SCNetworkReachability` from the Network framework. Risk Level: Medium. It's likely that this new approach to observing network reachability will be more correct and reliable than the `SCNetworkReachability` approach, but it's still possible for differences in behavior to cause issues where the wrong radio type is selected on devices running iOS 12 or later. To mitigate, the new path monitor is opt-in only. Testing: I've tested this on a physical iPhone, confirming that the logs about switching preferred network matches when either the reachability or path monitor mechanisms are used. Unit tests cover that appending `.enableNetworkPathMonitor()` to the builder commands flips the `useNetworkPathMonitor` flag to true. Docs Changes: Added. Release Notes: Added. Signed-off-by: JP Simard <jp@jpsim.com>
d_type is documented as only working with some filesystems, and that
all programs must handle a value of DT_UNKNOWN and make a stat() call
to retrieve the full information. To keep the code simpler, always make
the stat() call.
Signed-off-by: Greg Greenway ggreenway@apple.com