Skip to content

Conversation

@StoneMonarch
Copy link
Contributor

I think this was a relatively straightforward fix. I noticed the failed test timestamps were off by multiples of 2^32 seconds, both in the runner and when running local tests, so something wasn't calculating properly.

Casting Values:
It looked like something was off with the way bytes were being converted to ints on lines where we get the seconds like this: accessTimeSeconds := uint64(binary.LittleEndian.Uint32(b[0x8:0xc])). Here we are casting to a uint64 when the value outlined in the kernel doc is a signed int32. This could cause problems if we encounter a date before the Unix epoch, so it's best to use an int32 like the kernel does for the seconds value.

Decoding Timestamps:
Since the decoding tasks were very repetitive, it seemed easier to extract the decoding logic into a function and use the kernel doc provided formula: We use the 32-bit signed time value plus (2^32 * (extra epoch bits))

Encoding Timestamps:
The original code simply does this:

binary.LittleEndian.PutUint64(accessTime, uint64(i.accessTime.Unix()))
binary.LittleEndian.PutUint32(accessTime[4:8], uint32(i.accessTime.Nanosecond()))

This may be problematic because I don't see where the extra epoch bits are being set. However, we can just extract a function and do the inverse of the decode operation.

Tests:
Updated to include the valid time range provided by the kernel doc.

make lint and make test both pass.

kernel doc

Copy link
Collaborator

@deitch deitch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hats off! This was driving me a little crazy, as depending on how the specific timestamp ended up in a random test, it might fail or it might not. I was going to try and figure it out right after I finish that last bit on GPT partitions, so I am quite grateful you beat me to it.

@deitch deitch merged commit e5c6c6f into diskfs:master Jan 5, 2026
20 checks passed
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.

2 participants