Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IOS/FS: Fix CreateFullPath to not create directories that already exist #8594

Merged
merged 1 commit into from Feb 2, 2020

Conversation

leoetlino
Copy link
Member

Follow up to #8593.

This fixes CreateFullPath to not create directories when it is known
that they already exist, instead of calling CreateDirectory anyway
and checking if the error is AlreadyExists. (That doesn't work
now that we have an accurate implementation of CreateDirectory
that performs permission checks before checking for existence.)

I'm not sure what I was thinking when I wrote that function.

Also adds some tests for CreateFullPath.

This fixes CreateFullPath to not create directories when it is known
that they already exist, instead of calling CreateDirectory anyway
and checking if the error is AlreadyExists. (That doesn't work
now that we have an accurate implementation of CreateDirectory
that performs permission checks before checking for existence.)

I'm not sure what I was thinking when I wrote that function.

Also adds some tests for CreateFullPath.
@@ -434,3 +434,21 @@ TEST_F(FileSystemTest, ReadDirectoryOrdering)
ASSERT_EQ(result->size(), file_names.size());
EXPECT_TRUE(std::equal(result->begin(), result->end(), file_names.rbegin()));
}

TEST_F(FileSystemTest, CreateFullPath)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should any of these pathes be deleted after the test is finished? Otherwise the next time it runs the test won't be operating on a clean slate

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, they should. This is automatically done by the testing framework since we're using a fixture. Each test that is defined with TEST_F uses its own instance of FileSystemTest and (by extension) its own file system root, so the tests are independent.

Copy link
Contributor

@iwubcode iwubcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor comment, code seems fine. Untested.

@iwubcode
Copy link
Contributor

iwubcode commented Feb 1, 2020

Slightly off-topic but hoping we can eventually move to std::filesystem. In my own experience, I found this custom solution filesystem stuff to be a bit lacking in certain situations.

@leoetlino
Copy link
Member Author

leoetlino commented Feb 1, 2020

Slightly off-topic but hoping we can eventually move to std::filesystem. In my own experience, I found this custom solution filesystem stuff to be a bit lacking in certain situations.

Well, the reason we have an abstraction layer over the Wii file system is that we need to keep filesystem metadata accurately (see #8539 for reasons) and enforce various constraints. In the past various parts of Dolphin accessed the filesystem directly and that made it very easy to break things or implement things the wrong way (see all the FS and ES code that's been fixed over the last 3 years), while at the same time making it very difficult to have accurate implementations of certain commands. (How do you return file lists that are sorted in the correct order if the filesystem can be modified without the Wii filesystem code being aware of it -- or even without the actions being recorded?)

Forcing all accesses to go through the Wii FS layer ensures that Dolphin simply cannot do anything stupid with the filesystem or anything that would be obviously impossible on a console, since now both emulated software and the rest of Dolphin are mostly using the same code paths. It also allows filesystem stuff to be properly unit tested.

@leoetlino leoetlino merged commit 06d0b1a into dolphin-emu:master Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants