-
Notifications
You must be signed in to change notification settings - Fork 123
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
base: master
Are you sure you want to change the base?
Conversation
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 If you look at the |
I'm happy to have a look at creating separate
Since the However, looking at the current package, I think most of the code actually lives in the I haven't looked too closely, but I suspect that using |
As I wrote:
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 |
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 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 )) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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:
fat32.Filesystem.Type()
return different types for fat12 and fat16Create()
to handle creating FAT12/16fat32_test.go
tests passingMain question before I go any further with this: Should this sit within the
fat32
package, or should we split up thefat32
package into other packages? Do you have any other suggestions?Fixes #293