diskfs / go-diskfs

MIT License
515 stars 116 forks source link

add more test file creation #256

Closed mrbojangles3 closed 1 month ago

mrbojangles3 commented 1 month ago

I am using this project (its great!) I am seeing an issue when I am creating a bootable disk with two partitions. the first partition is a "normal" efi partition that boots and works well. The second partition is a fat32 filesystem that hold all sorts of files and folders. some of the files are 1 GiB in size. When I am writing to my second partition I see a segfault at the 3rd 1 GiB file. The others seem to do okay.

This is a PR that will create a tree on a partition and exposes the same error message that I was seeing on my system. The annoying thing is, I have to have the random-sized file creation turned on. to see my issue, so I don't have a simple test case. I saw that you called for a test program in #109 . This is hopefully that program. Happy to discuss further. Again, thank you for this project.

mrbojangles3 commented 1 month ago

I realized a little bit later that I can seed the random number generator so there can be a deterministic test case. I will update the code.

deitch commented 1 month ago

Hi @mrbojangles3

So when writing a 1GB file you get a segfault, but only on the 3rd one? Does this occur with any fat32 filesystem? Or only in a disk with 2 partitions?

If your test case can highlight the issue, then hopefully we can resolve it. Post here when you have the test complete, we can run it and then get it fixed.

deitch commented 1 month ago

Something weird about the go proxy. Leave it for an hour and we can rerun tests.

mrbojangles3 commented 1 month ago

@deitch , Thank you for your quick replies and willingness to work with on this. I have updated the test code. The error I am seeing is:

$ ./fat32 
panic: could not read directory entries for /b/sub50/blob

goroutine 1 [running]:
main.mkGigFile({0x575550?, 0xc000120000?}, {0x5452a3?, 0x54e64b?})
        /home/lblyth/repos/go-diskfs/filesystem/fat32/testdata/fat32.go:133 +0xa5
main.main()
        /home/lblyth/repos/go-diskfs/filesystem/fat32/testdata/fat32.go:31 +0x331

My limited (but expanding) knowledge of the FAT file system make me suspect that a directory entry is getting overwritten somehow. In the test code if you remove lines 30 and 31, so that only a single Gig file is made, the error changes from:

panic: could not read directory entries for /b/sub50/blob

to

panic: error reading directory /b/sub50/blob: path /b not found
deitch commented 1 month ago

@mrbojangles3 is that the error you were seeing before? In other words, does this test case now faithfully reproduce your issue?

deitch commented 1 month ago

Why are you getting lint errors on unchanged files? That is really strange.

Yeah, I just ran it locally, same version as in there v1.59.0. Maybe go version, it is sensitive to 1.23 vs 1.22? I will try locally.

Frostman commented 1 month ago

@deitch yes, it should match the go version too

deitch commented 1 month ago

Yeah, that is it. Sigh. I will fix them.

mrbojangles3 commented 1 month ago

To the best of my understanding, this reproduces my test case. On my system I actually see a a segfault. The path related errors are the same. my "/b" directory is unable to be found, when I call open on a file that will be a 1G file.

deitch commented 1 month ago

Oh, that linting is a pain. They include gosec, which has this aggressive G115, which complains about all of the integer overflow conversions, which are necessary.

deitch commented 1 month ago

That was harder than I wanted, but if you rebase, you should get past lint stuff

deitch commented 1 month ago

Do you mind squashing the commits do and cleaning up the base, so it is rebased on master, a clean single commit, rather than the merge commit?

deitch commented 1 month ago

go proxy misbehaving again.

mrbojangles3 commented 1 month ago

apologies, I think I squashed one commit too many. Since I have your lint changes in.

mrbojangles3 commented 1 month ago

Okay, I think this is what you wanted.

deitch commented 1 month ago

Yeah that looks right. Let's let CI run, and hope the go proxy issues have been resolved, but that's not us.

deitch commented 1 month ago

So now I don't get it. You added a test that should fail, and the tests all pass?

mrbojangles3 commented 1 month ago

Apologies for being unclear. I modified the test code in fat32/testdata/fat32.go to show how I and perhaps the commenter in #109 were seeing issues with file creation, there was a request for example code. Perhaps I should have added this as a gist?

deitch commented 1 month ago

I am confused. #109 said that there is a failure when writing certain files, and I think you had said the same thing. Isn't the test you added to trigger that failure? Such that the tests will fail, and then we can fix it such that the tests will pass?

mrbojangles3 commented 1 month ago

My takeaway from #109 was that an error occurred in file writing at around 50 files. I was seeing something similar in my usage of go-diskfs. I have not modified the automated tests, but instead the code that resides inside of the testdata directory. I am still getting acclimated to the golang ecosystem and modifying the code I did was easier / quicker than adding a test. I want to be a good contributor so it seems like the right thing to do is for me to modify one of the _internal_test.go files? I don't know for sure which internal test file will trigger the issue I have seen. But I suspect the fat32_internal_test.go?

Does the code I submitted run without issue for you?

deitch commented 1 month ago

Yeah, it does. Look at the CI right here. No problems. If you have an issue, it would be great to get a reproduction into the tests.

mrbojangles3 commented 1 month ago

Closing to add test case in future PR