Skip to content

Add initial support for Fat12 and Fat16 filesystems #294

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

acheronfail
Copy link

@acheronfail acheronfail commented Apr 24, 2025

This is most definitely a draft right now, but seeking some early feedback.
For the filesystems I'm dealing with, this was able to read a FAT16 volume and list its root directory - which is awesome 🎉 since it's actually not that much code!

I did re-use the fat32 package though, since so much of its code can be re-used.
The implementation here is far from complete, things left:

  • decide on how we want these packages to be (question below)
  • have fat32.Filesystem.Type() return different types for fat12 and fat16
  • update text fixtures for FAT12/16
  • update bpb/bootsector code for FAT12/16
  • update Create() to handle creating FAT12/16
  • get all fat32_test.go tests passing

Main question before I go any further with this: Should this sit within the fat32 package, or should we split up the fat32 package into other packages? Do you have any other suggestions?

Fixes #293

@deitch
Copy link
Collaborator

deitch commented Apr 24, 2025

I commented on the original issue, but those comments may not hold up in light of your draft here.

I still think it is cleaner if we can have fat12 and fat16 and fat32 directories. But if it really is a significant amount shared, such that we would end up duplicating things or making public structs and funcs that should be private, then we should leave it in the single directory.

If you look at the Filesystem interface , it includes all of the functions an implementation should have, as well as Type() Type, which is enumerated at the end of the same file. If it is better in the same directory because of so much shared, then the logic probably should be to return different Type for fat12 or fat16 or fat32. I don't know if that means one struct fat32.FileSystem that implements filesystem.FileSystem and knows if it is fat32 or fat12 or fat16, or if that is 3 distinct struct, which happen to share a lot in the same file.

@acheronfail
Copy link
Author

acheronfail commented Apr 24, 2025

I'm happy to have a look at creating separate fat12 and fat16 packages. At a glance I think we'd need to:

  • move BPB parsing/structs into the fat12 package and export them
  • move MSDOS boot sector parsing/structs into the fat12 package and export them
  • move the File/FileInfo/Directory things into fat12 package and export them

Since the fat16 and fat32 packages would need to use those.

However, looking at the current package, I think most of the code actually lives in the filesystem.Filesystem implementation in fat32.go - this was the reason I started just patching it instead of creating a separate fat16 package, etc. That seems to be the bulk of the code, and most (if not all) of it is re-usable.

I haven't looked too closely, but I suspect that using fat32.Filesystem is easier to make backwards compatible since it has wider types (uint32 as opposed to uint16, etc) and we can just parse the fat12/16 things into wider types; which makes me think that implementing multiple filesystem.Filesystem interfaces likely will be a lot of duplicated code...

@deitch
Copy link
Collaborator

deitch commented Apr 24, 2025

As I wrote:

But if it really is a significant amount shared, such that we would end up duplicating things or making public structs and funcs that should be private, then we should leave it in the single directory.

It sounds like you are saying that it is a lot of (probably unnecessary) work to separate it out, so then I would leave it. Just make sure that the Filesystem implementation actually can distinguish between the various types, including reporting it upwards when Type() is called. Tests really would help.

@acheronfail
Copy link
Author

I had some more time and gave it some more work, and I've got most of the tests passing. There's been a significant amount of work required to update the test suites and fixtures to get it all working for fat12 and fat16...

I'm beginning to think that splitting them up might be the better option here, since the test code is looking more like spaghetti this way. 😅

I'm not sure how much time I'll have for this over the coming weeks, but I'll try to come back to it when I can.


i=0
until [ $i -gt 75 ]; do mmd -i /data/fat32.img ::/foo/dir${i}; i=$(( $i+1 )); done
dd if=/dev/zero of=${out}/disk.img bs=1M count=$(( n + 2 ))
Copy link
Author

Choose a reason for hiding this comment

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

This $(( n + 2 )) gives each FAT filesystem enough space to actually be a valid FAT partition.

In the original fixture the FAT32 image was given only 10mb, which resulted in errors such as:

WARNING: Number of clusters for 32 bit FAT is less then suggested minimum.

Warning: Filesystem is FAT32 according to fat_length and fat32_length fields,
but has only 20132 clusters, less than the required minimum of 65525.
This may lead to problems on some systems.

Since it was a "technically incorrect" FAT32 filesystem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, but you always can override it.

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

Successfully merging this pull request may close these issues.

FAT12 and FAT16 support?
2 participants