-
Notifications
You must be signed in to change notification settings - Fork 117
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
added test for file tree creation with large files #258
Conversation
Heh, it didn't even get to your test, because listing failed. Please check that. |
Lint still is complaining. Run |
Well, the thing is, I have been. But I am getting lint errors in code I didn't touch (in the standard library) and the errors I am seeing here aren't the ones I have had show up in my local runs of |
What go version are you using? |
Just 3 lint errors left. |
go version go1.23.0 linux/amd64 |
So that should do it. Maybe you have a different version of golangci-lint. Either way, you can see the last 3 lint errors here |
Lint is all clean. Huzzah! |
Is the error what you expected? |
I am glad I got my local env matching the CI so this will hopefully NEVER happen again. This is the "right" error. |
"never" is such a strong word. 😆 Can you squash the commits to a single commit? Do you understand the cause of the error that this test triggers? |
0fa5414
to
4f6a2dc
Compare
lol, yep Squashed! I do not understand what is causing this error. It was odd to see it pop up in our env. Based on our use case (making a boot-able image with two partitions) it only happened with the big files. |
Well, with an actual test case, we can try and solve it now. |
I am looking forward to learning more about FAT32 because of this. |
Do you know why the tests take so long now? On |
Do you mind rebasing? I fixed some of the missing error reporting. Does not fix your issue, but does make the errors easier to see. |
I am not sure why the test time ballooned. I am making a 6GiB file so that might take time depending on whats happening on the local machine. Good suggestion on the temp file. I assume there will be more changes so if its okay, I will hold off on the squashes until the end. |
Better, thanks. Looking at it. If you can squash the commits into a single one, rather than having the merge-from-master commit, that would help. |
Sure, we can wait for that. |
OK, I have some practical information. Up until you create the first gigfile, everything is fine. I checked the state of the
After the first gigfile:
Clearly, something there is wiping them clean. My first suspicion is that the calculations break down somewhere at a certain size. That should be something we can test. |
Next data point. If I skip your whole tree, and just make the gigfile, it works fine. So it is some combination of the two. |
Can I ask a methodology question? - Are you using delve, breakpoints, then hexdumping the image? |
Exactly that. Although hexdumping in a separate window so I can work in parallel. |
It is an overflow. We use uint32 for the read offset here, and for the write offset here, but it can be up to 64 bits (filesystem offset calls on go usually take int64). Either way, I managed to recreate it where it gets to a position where it overflows, then starts overwriting back from the beginning. PR coming soon. |
See #260 . And I incorporated your tests in the. I rewrote them a bit to fit with the style, but they go right on in. |
Great find! |
Yeah, but filesystem code does all sorts of things with individual bytes. Can we close this now? |
Would you be willing to tag this version ? |
Sure. |
This is the PR alluded to in #256. I have added a test case and some supporting functions to trigger an issue I am seeing on my system.
TestCreateFileTree
is the case I am adding. This creates a 6GiB fat32 file system, so hopefully the CI doesn't fall over because of it.This is a great project, and you have been very responsive. Thank you.