USF-OS / FogOS

Other
1 stars 24 forks source link

Add CKSUM - Updated #48

Open marcusaltman opened 3 months ago

marcusaltman commented 3 months ago

Used a POSIX CRC32-based CRC algorithm with polynomial 0x04C11DB7 to calculate the CRC checksum value using the standard full-byte table-lookup method.

Update: Added a cksumtest program and a CKSUM_README file in docs.

par-sodhi commented 3 months ago

Noga and I (Param) will code review this

AlexBareli commented 3 months ago

Hey, I will be code reviewing this project

AlexBareli commented 3 months ago

Code Review:

Your CKSUM project is very impressive and complex, which you did an amazing job on. The documentation was well written, and I liked how you specified which files you modified which made it easier to code review. The CKSUM program worked as expected when I was playing around with it, and the test program did a good job at testing the functionality by creating the files itself and showcasing the error checking. One minor change I would make is if the user enters no arguments, then the program should print usage instructions. I noticed that the program works for more than two files, I think you could mention it in the readme that you can input as many files as you'd like in the argument. One bug that I’ve noticed is in the error checking, if for example you input “cksum short.txt . long.txt” then the program will end before processing the last argument. I think maybe your error checking could simply print something to the console rather than ending the program if you’d like to let the program process all the arguments before exiting. Overall fantastic work and great job!!!

Summarized Suggested Changes:

par-sodhi commented 3 months ago

Code Review:

Hi Marcus!

Overall amazing job on the project.

I tested your project a lot and seemed to not find any bugs that stood out to me.

Here are some things I really think you did well:

  1. Adding Comments: I believe you wrote a bunch of comments and a solid README that helped me dissect and understand the code in much more detail than I would normally have. This came especially in handy when it comes to the CRCtable.h file.

  2. Test File Rigor: The rigor of the test file was very nice. It seemed you had caught almost all, if not all of the cases that you needed to be successful.

  3. The lookup table approach: I though that the lookup table approach helped create a significant speedup in your code. If you had done the series of computations as bit level operations, I believe that your code would have been very slow.

Some changes I believe you can make:

  1. I agree with the above suggestions about printing out instructions when the user enters no arguments. Would make reading easier

Overall, though, great job!

par-sodhi commented 3 months ago

One last comment:

A small proposal if you have any time or would like to implement a new utility, may I suggest MD5?

From my understanding, MD5 is a hashing algorithm that takes in a message and returns a fixed sized hash value. It is not usually used for encryption but can be used for data integrity and could serve as a checksum to verify data integrity in certain protocols.

Your program could take in an input, create a unique hash value out of it, keeping the same size consistently for comparison purposes and is used for data integrity verification

malensek commented 2 months ago

After reviewing this, the project is solid as-is for P1. For future work: