Skip to content
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

Wrap errors, not Unwrap #240

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Wrap errors, not Unwrap #240

merged 1 commit into from
Jul 22, 2024

Conversation

quite
Copy link
Contributor

@quite quite commented Jul 21, 2024

Fixes #237

@deitch
Copy link
Collaborator

deitch commented Jul 21, 2024

There are 2 tests failing, both related to converting superblock to bytes. Yet you didn't change anything there.

Can you run the tests on the master branch and on your own locally? You just need to check go test ./filesystem/ext4

@quite
Copy link
Contributor Author

quite commented Jul 21, 2024

go test ./filesystem/ext4

After running the ./buildimg.sh they pass on both

@deitch
Copy link
Collaborator

deitch commented Jul 21, 2024

Then why doesn't it pass in CI? 🤷‍♂️

@quite
Copy link
Contributor Author

quite commented Jul 21, 2024

Something is up with that runner/image? Same here now #241

@deitch
Copy link
Collaborator

deitch commented Jul 22, 2024

After running the ./buildimg.sh they pass on both

Same here. You don't need to run it manually, anyways. Just remove the contents of (or the directory itself) filesystem/ext4/testdata/dist/ and it will autogenerate it.

@deitch
Copy link
Collaborator

deitch commented Jul 22, 2024

I just ran a test on macOS and on Ubuntu 22.04. It shouldn't matter, since the actual images generating from within buildimg.sh are run inside docker, but I did it anyways. Everything passes.

@deitch
Copy link
Collaborator

deitch commented Jul 22, 2024

There are 2 sections of the superblock that are mismatching. The second is just the checksum, which is different because of the first. Resolve the first, the checksum issue should go away.

The first is total bytes written. The actual is showing 0x9400 = 37888, while the expected is 0x95da = 38362, a difference of 474 (the total KiB written is at position 0x178 in the superblock, little-endian).

Ah, I see it, the issue is here. After buildimg.sh builds the filesystem, we need some way to know the actual value there, so we can test that diskfs is reading it correctly. It uses the output of dumpe2fs ext4.img > stats.txt, which also is in testdata/dist/. The test utility testGetValidSuperblockAndGDTs() reads the output of that. For example, the line from the stats.txt file that gives total written:

Lifetime writes:          37 MB

Unfortunately, that is in MB and not KB, and is not precise. So a small variant can throw it off. We try to catch it here:

	sbKBWritten := float64(sb.totalKBWritten)
	parsedKBWritten := float64(parsed.totalKBWritten)
	KBdiff := math.Abs(parsedKBWritten - sbKBWritten)
	if KBdiff/sbKBWritten < 0.01 {
		sb.totalKBWritten = parsed.totalKBWritten
	}

But it is a bit of a guessing game. It would be good if dumpe2fs (or some other reliable tool) gave precise stats in KB written. debugfs -R gives the same output.

@deitch
Copy link
Collaborator

deitch commented Jul 22, 2024

#242 fixes this, it has been merged in. Rebase on master and try again.

@deitch deitch merged commit bd61c20 into diskfs:master Jul 22, 2024
20 checks passed
@quite quite deleted the do-wrap branch July 22, 2024 09:37
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.

Use of errors.Unwrap makes it harder to use the API
2 participants