Konamiman / Nextor

A disk operating system for MSX computers, forked from MSX-DOS 2.31
Other
183 stars 35 forks source link

FAT size rounding issue in fdisk? #113

Closed grauw closed 1 month ago

grauw commented 1 year ago

On the if here:

https://github.com/Konamiman/Nextor/blob/v2.1/source/kernel/bank5/fdisk2.c#L416

sectorsPerFat = (clusterCount + 2) >> 8;

if(((clusterCount + 2) & 0x3FF) != 0) {
    sectorsPerFat++;
}

If the intent is to round up the division above it (>> 8), should this not be 0xFF?

It looks like a copy / pasta from the FAT12 code, but the calculation there is different.

grauw commented 1 year ago

Additionally, should PARTYPE_EXTENDED not be 0x0F instead of 0x05? As I understand it that’s the type to use for LBA addressing, which seems appropriate since FDISK writes only LBA information. If that’s changed the kernel should be as well.

Lastly, if I understand the wiki correctly, it also says that the second (link) EBR entry’s size field is always the size of the next partition, whereas FDISK sets the first one to the total size of the entire EBR chain. I wonder which is correct.

Konamiman commented 1 year ago

Hi @grauw, thanks for your report!

Additionally, should PARTYPE_EXTENDED not be 0x0F instead of 0x05

Probably, but I don't think it would be a good idea to change that at this point (for new partitions created with FDISK) as that could cause issues with older versions of MAPDRV (and apparently, using 0x05 instead of 0x0F hasn't caused any issues so far).

That said, what Nextor should definitely do is recognizing 0x0F as an equivalent to 0x05 when searching for partitions in devices; and that's exactly what https://github.com/Konamiman/Nextor/pull/121 does (feel free to review if you want).

Lastly, if I understand the wiki correctly, it also says that the second (link) EBR entry’s size field is always the size of the next partition, whereas FDISK sets the first one to the total size of the entire EBR chain. I wonder which is correct.

I wonder too as in the past I have found conflicting information regarding this. Anyway that specific field of the partition table isn't effectively used by Nextor, and apparently neither by other OSes; so I'll leave that part as is unless a specific issue is detected.

ATroubleshooter commented 1 year ago

Hello, @Konamiman

Additionally, should PARTYPE_EXTENDED not be 0x0F instead of 0x05

Probably, but I don't think it would be a good idea to change that at this point (for new partitions created with FDISK) as that could cause issues with older versions of MAPDRV (and apparently, using 0x05 instead of 0x0F hasn't caused any issues so far).

That said, what Nextor should definitely do is recognizing 0x0F as an equivalent to 0x05 when searching for partitions in devices; and that's exactly what #121 does (feel free to review if you want).

Well, yes, the troubles pop up only when media, partitioned with NEXTOR's fdisk, are accessed outside MSX, on systems that practically respect the difference between EXTENDED and EXTENDED LBA.

ATroubleshooter commented 1 year ago

Well, yes, the troubles pop up only when media, partitioned with NEXTOR's fdisk, are accessed outside MSX, on systems that practically respect the difference between EXTENDED and EXTENDED LBA.

On Linux, for instance, you get this: partx -a parttest.dsk partx: /dev/loop17: error adding partition 5

Konamiman commented 1 year ago

the troubles pop up only when media, partitioned with NEXTOR's fdisk, are accessed outside MSX, on systems that practically respect the difference between EXTENDED and EXTENDED LBA.

That makes sense. I think I'll change it so that FDISK creates partitions with the LBA code, and will also create a tool to change the code in existing partitions to either the old or the new value.