Separate key algorithm and core code for rp2 and rp3 - [merged]

[icex2]

In GitLab by @33c17f40 on May 12, 2022, 03:45

_Merges rp2refactor2 -> master

Summary

Separated the key generation and the payload generation core code for rp2 and rp3.

Description

Discussed in the last PR, the rp2 and rp3 code could be merged further. I attempted to separate the code required for the keys from the code that generates the actual dongle payload data.

Related Issue

https://dev.s-ul.net/djhackers/bemanitools/-/merge_requests/111

How Has This Been Tested?

security-rp2-test and security-rp3-test test cases.

Checklist

• [ ] Implemented (unit) test(s) which prove that the introduced changes are working as expected.
• Tested with the following games:
  • [ ] ...
  • [ ] ... <!-- insert game name 2--->
• [ ] Followed the developer (style) guidelines.
• [ ] Updated existing doc of or add new doc to README file(s).
• [ ] Updated development documentation.

[icex2]

In GitLab by @33c17f40 on May 12, 2022, 03:50

I feel like you could probably throw security_rp3_generate_signed_eeprom_data into rp2.c but then I'm not sure what to do with the function naming.

Maybe something like this? security_rp2_generate_signed_eeprom_data_v1 security_rp2_generate_signed_eeprom_data_v2

Throwing security_rp3_generate_signed_eeprom_data into rp2.c as-is with that name feels like it breaks the intended file structure because why would you look for rp3 in rp2.c.

Please let me know if you have any ideas.

Edit: Actually, nevermind. Got too focused on the code itself and didn't consider the output. I think separating rp2.c and rp3.c is probably good enough because of the obvious difference in returned data structure.

[icex2]

I guess when you are in doubt right now, the anticipated gain is low and the current structure is not entirely messed up, I suggest to just leave it as is. Further iterations can always be taken when anyone comes back to this.

[icex2]

lgtm, waiting until the thread is resolved by you to give you the chance to reply if there is anything else to bring up.

[icex2]

approved this merge request

[icex2]

In GitLab by @33c17f40 on May 12, 2022, 23:28

I'm fine with the PR in its current form. I don't think there's much to gain from further refactoring it.

[icex2]

resolved all threads