nibblebits / PeachOS

Simple kernel designed for a online course
GNU General Public License v2.0
133 stars 55 forks source link

Calculation for ending_sector_pos wrong in fat16_get_root_directory #28

Open gierens opened 9 months ago

gierens commented 9 months ago

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: https://github.com/nibblebits/PeachOS/pull/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.