Checksum op #7
Checksum op #7
Conversation
disk_store.go
Outdated
| timestamp := uint32(time.Now().Unix()) | ||
| h := Header{TimeStamp: timestamp, KeySize: uint32(len(key)), ValueSize: uint32(len(value))} | ||
| h := Header{CheckSum: crc32.ChecksumIEEE([]byte(value)), TimeStamp: timestamp, KeySize: uint32(len(key)), ValueSize: uint32(len(value))} |
There was a problem hiding this comment.
checksum should be calculated on all the values. What if my KeySize / Key is corrupted?
|
@PaulisMatrix this again misses one more thing: what if my timestamp field is corrupted? the best way would be to calculate checksum using all fields of the record |
format.go
Outdated
| kvBuf := append([]byte(key), []byte(value)...) | ||
|
|
||
| buf := append(headerBuf.Bytes()[4:], kvBuf...) | ||
| return crc32.ChecksumIEEE(buf) |
There was a problem hiding this comment.
@avinassh , I have considered all fields only, except the checksum field itself while calculating.
There was a problem hiding this comment.
ahh, I saw only CalculateCheckSum(key, value string), assumed something else. Right, you do use other fields.
but this looks buggy? Because you are calling EncodeHeader, which includes checksum (which could be empty bytes for now)
err := binary.Write(buf, binary.LittleEndian, &h.CheckSum)
I would suggest moving CalculateCheckSum to be part of Record struct and make it idempotent, it should return same value whether header already contains checksum or not
There was a problem hiding this comment.
Yeah, I just thought of reusing the EncodeHeader and picking up bytes from 4th index(effectively excluding CheckSum field). But I get your point, have made the change!
|
Thank you! 🎉 Feel free to send a follow up PR which tests for the checkum |
|
btw I had this paper in my reading listing related to checksums and data corruption, I haven't read it yet but you might find it interesting - Detecting silent data corruptions in the wild |
No description provided.