More refactors, fixes, etc #141

mmlb · 2024-05-01T15:36:49Z

I've split these commits out from #136 where they came up. The commits here are grouped into fixes and qol stuff. The QoL stuff are changes to Executor and some missed code comments/fixes to nvme that were meant to go in during #129.

The other big chunk of changes is to utils/watermark where I ran into incorrect params to rand.Int that would cause us to seek to a negative position. I fixed that and added some documentation and renamed some variables for clarity. Also tweaked the tests a little and added a new test case.

mmlb · 2024-05-01T17:32:12Z

depends on #137

joelrebel

These changes seem fine to me

This is just easier to use.

Since there's only one Exec func there's no need to have WithContext in the name to differentiate against a non-existent context-less Exec.

Callers might want to treat ineffective wipes differently than other errors, for example fallback to a different option/tool. I've also changed the error message a little bit to mimic what it looks like following stdlib errors. For example: ``` fmt.Println(os.Open("non-existent-file")) ``` returns: ``` <nil> open non-existent-file: no such file or directory ```

Mostly to move file size checks into writeWatermarks and get rid of global variables.

A few simple refactors to make the tests easier to reason about. Instead of sharing the testfile and wiping it we now use a small function and t.TempDir that creates the testfile per test and cleans up after exit. No need to wipe the test file at the beginning of each test. Used more assertions from stretchr/testify instead of `if` checks, they just read better. Added a test for unsuccessful wipe. Renamed the subtests to be more descriptive.

The code was not quite right before, even though there was a test that seemed to be checking this. The test actually failed when seeking past the end of the file. Some might say same thing but it breaks other code and test that I was writing that fixes something else in here.

Just looks like a better order to debug/read through. 0 < too small < fails < success!

The previous code was subtly wrong and only caught by random chance when I ran some tests. We need to limit the random offset so that we can be sure to write the full buffer without crossing over into the next chunk, but this needs to happen when getting the random offset and not calculating the position. Consider the following situation: * Disk size is 5120B (chunks are 512B) * First iteration of the loop so chunkStart = 0 * rand.Int randomly returns 1 * randomPosition is calculated as (0 + 1 - 512) == -511 * We try to seek to position -511 of the disk * Error Where now it looks like: * Disk size is 5120B (chunks are 512B) * First iteration of the loop so chunkStart = 0 * rand.Int randomly returns 1 * randomPosition is calculated as (0 + 1) == 1 * We try to seek to position 1 of the disk * Ok Likewise we are still save at the other extreme: * Disk size is 5120B (chunks are 512B) * First iteration of the loop so chunkStart = 0 * rand.Int will only ever return 0 since chunkSize-watermarksSize=512-512=0 * randomPosition is calculated as (0 + 0) == 0 * We try to seek to position 0 of the disk * Ok Or at the end of the disk: * Disk size is 5120B (chunks are 512B) * Last iteration of the loop so chunkStart = 4608 * rand.Int will only ever return 0 since chunkSize-watermarksSize=512-512=0 * randomPosition is calculated as (4608 + 0) == 4608 * We try to seek to position 4608 of the disk * Ok * Write 512B * Cursor is now at 5120, right at the end of the disk and Ok

Bit twiddling/parsing isn't very common in Go code so lets add a simple blurb for anyone checking out the code.

No need to right shift when we are just confirming if the value is all-zeros or not, 4096 is just as much !0 as 1 is.

I added this in here to prematurely appease a linter. No such linter complaint exists so lets drop it.

Makes more sense.

mmlb requested review from joelrebel, splaspood and turegano-equinix as code owners May 1, 2024 15:36

mmlb force-pushed the refactors-fixes-etc branch 2 times, most recently from b12973c to 413d493 Compare May 1, 2024 17:31

mmlb mentioned this pull request May 1, 2024

Add DiskWipe support to nvme using sanitize #136

Merged

joelrebel approved these changes May 2, 2024

View reviewed changes

mmlb added 12 commits May 2, 2024 09:00

utils/executor: Change SetArgs from []string -> ...string

337d713

This is just easier to use.

utils/executor: Rename .ExecWithContext -> .Exec

9b45eba

Since there's only one Exec func there's no need to have WithContext in the name to differentiate against a non-existent context-less Exec.

utils/watermark: Small refactors

c36f911

Mostly to move file size checks into writeWatermarks and get rid of global variables.

utils/watermark: Organize the tests a little better

d94505d

Just looks like a better order to debug/read through. 0 < too small < fails < success!

utils/nvme: Add some bit twiddling info to parseFna/parseSanicap

18332bb

Bit twiddling/parsing isn't very common in Go code so lets add a simple blurb for anyone checking out the code.

utils/nvme: Simplify bits -> bool logic

50437e0

No need to right shift when we are just confirming if the value is all-zeros or not, 4096 is just as much !0 as 1 is.

utils/nvme: Remove "unreachable" panic

24b989b

I added this in here to prematurely appease a linter. No such linter complaint exists so lets drop it.

utils/nvme: Rename logicalPath arg -> device

9dcc9c0

Makes more sense.

mmlb force-pushed the refactors-fixes-etc branch from 413d493 to 9dcc9c0 Compare May 2, 2024 13:00

mmlb merged commit f526bc1 into metal-toolbox:main May 2, 2024
5 checks passed

mmlb deleted the refactors-fixes-etc branch May 2, 2024 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More refactors, fixes, etc #141

More refactors, fixes, etc #141

mmlb commented May 1, 2024

mmlb commented May 1, 2024

joelrebel left a comment

More refactors, fixes, etc #141

More refactors, fixes, etc #141

Conversation

mmlb commented May 1, 2024

mmlb commented May 1, 2024

joelrebel left a comment

Choose a reason for hiding this comment