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

Calculation for ending_sector_pos wrong in fat16_get_root_directory #28

Open
gierens opened this issue Nov 16, 2023 · 0 comments
Open

Comments

@gierens
Copy link

gierens commented Nov 16, 2023

In the function fat16_get_root_directory we first calculate the total_sectors (https://github.com/nibblebits/PeachOS/blob/master/src/fs/fat/fat16.c#L215-L219):

    int total_sectors = root_dir_size / disk->sector_size;
    if (root_dir_size % disk->sector_size) {
        total_sectors += 1;
    }

and then at the end of the function set the ending_sector_pos like so (https://github.com/nibblebits/PeachOS/blob/master/src/fs/fat/fat16.c#L246):

    directory->ending_sector_pos = root_dir_sector_pos + (root_dir_size / disk->sector_size);

This not only leaves total_sectors completely unused but recalculates the division which is inefficient and introduces a bug if I'm not mistaken. This was also outlined in this PR from last year: #7 .

This should be fixed by changing the assignment similar to what the PR suggests to(https://github.com/nibblebits/PeachOS/pull/7/files):

    directory->ending_sector_pos = root_dir_sector_pos + total_sectors;

Note that the - 1 is missing in comparison to the PR, otherwise fread doesn't seem to work.

I just bring this up, so it's not overlooked when revising the code for part 2 of the course and people see it's gonna be addressed as not everybody is going to check the closed PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant