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

Add missing iterations in tests #786

Merged
merged 1 commit into from
May 4, 2023

Conversation

sosthene-nitrokey
Copy link
Contributor

The tests don't actually test every file.
The iteration skips the two . and .. directories, which means that the last two files were not read. This PR fixes that. This seems to reach an edge case reported in #785.

As can be seen, the tests fail at a call to lfs_dir_seek, when they shouldn't.

@geky
Copy link
Member

geky commented Apr 18, 2023

Hi @sosthene-nitrokey, thanks for creating an issue.

I'm trying to dig into the bug, but I'm not sure this test was actually missing files?

It's a bit convoluted, but the lfs_dir_reads at the beginning of the loop should be skipping the . and .. entries before the inner loop iterates over directory entries from 0 to j.

for (int j = 2; j < COUNT; j++) {
lfs_mount(&lfs, &cfg) => 0;
lfs_dir_open(&lfs, &dir, "/") => 0;
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(strcmp(info.name, ".") == 0);
assert(info.type == LFS_TYPE_DIR);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(strcmp(info.name, "..") == 0);
assert(info.type == LFS_TYPE_DIR);
lfs_soff_t pos;
for (int i = 0; i < j; i++) {
sprintf(path, "hi%03d", i);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(strcmp(info.name, path) == 0);
assert(info.type == LFS_TYPE_DIR);
pos = lfs_dir_tell(&lfs, &dir);
assert(pos >= 0);
}
lfs_dir_seek(&lfs, &dir, pos) => 0;
sprintf(path, "hi%03d", j);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(strcmp(info.name, path) == 0);
assert(info.type == LFS_TYPE_DIR);

Correct me if I'm wrong. I'll look into the error it's throwing.

@geky
Copy link
Member

geky commented Apr 18, 2023

Ah, the issue is it should be i <= j, so this does show this did hide the directory bug.

@geky
Copy link
Member

geky commented Apr 18, 2023

Well, no, the i < j iterates the all previous entries, the final lfs_dir_tell should hold the correct pos.

The j = 2 is incorrect in the loop though, so we are missing the first two entries. Though this doesn't reveal the underlying bug.

I think this needs its own test.

@geky geky added the fixed? label Apr 18, 2023
@geky
Copy link
Member

geky commented Apr 18, 2023

I went ahead and created #805, thanks for creating a PR!

@sosthene-nitrokey
Copy link
Contributor Author

Thank you for the investigation and the fix!

Feel free to close this PR as your PR also adds tests that cover this. This PR was meant mostly as an example reproducer, and not really with the goal to be merged.

@geky geky merged commit 24795e6 into littlefs-project:master May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed? needs investigation no idea what is wrong
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants