Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor pgp_encrypted_material_t to C++ classes. #2304

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

ni4
Copy link
Contributor

@ni4 ni4 commented Dec 25, 2024

Just few more types left to avoid unions and move pgp::mpi to std::vector.

#if defined(ENABLE_PQC)
case PGP_PKA_KYBER768_X25519:
FALLTHROUGH_STATEMENT;
// TODO add case PGP_PKA_KYBER1024_X448: FALLTHROUGH_STATEMENT;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Copy link

codecov bot commented Dec 25, 2024

Codecov Report

Attention: Patch coverage is 86.46154% with 44 lines in your changes missing coverage. Please review.

Project coverage is 85.14%. Comparing base (884b52b) to head (39e3e5e).

Files with missing lines Patch % Lines
src/lib/key_material.cpp 88.60% 9 Missing ⚠️
src/librepgp/stream-dump.cpp 87.32% 9 Missing ⚠️
src/lib/rnp.cpp 27.27% 8 Missing ⚠️
src/lib/sig_material.cpp 77.41% 7 Missing ⚠️
src/lib/enc_material.cpp 84.21% 6 Missing ⚠️
src/librepgp/stream-packet.cpp 85.71% 2 Missing ⚠️
src/lib/crypto/signatures.cpp 85.71% 1 Missing ⚠️
src/lib/sig_material.hpp 83.33% 1 Missing ⚠️
src/librepgp/stream-parse.cpp 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2304      +/-   ##
==========================================
+ Coverage   84.90%   85.14%   +0.24%     
==========================================
  Files         114      118       +4     
  Lines       22788    22780       -8     
==========================================
+ Hits        19349    19397      +48     
+ Misses       3439     3383      -56     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ni4 ni4 force-pushed the ni4-refactor-enc-material branch 2 times, most recently from 4a5ed2b to c438d4d Compare December 27, 2024 16:16
#if defined(ENABLE_PQC)
case PGP_PKA_DILITHIUM3_ED25519:
FALLTHROUGH_STATEMENT;
// TODO: add case PGP_PKA_DILITHIUM5_ED448: FALLTHROUGH_STATEMENT;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
src/librepgp/stream-dump.cpp Fixed Show fixed Hide fixed
@ni4 ni4 force-pushed the ni4-refactor-enc-material branch from c438d4d to c499a5d Compare January 6, 2025 09:58
@ni4 ni4 changed the title (WIP) Refactor pgp_encrypted_material_t to C++ classes. Refactor pgp_encrypted_material_t to C++ classes. Jan 6, 2025
@ni4 ni4 marked this pull request as ready for review January 6, 2025 09:58
@ni4 ni4 force-pushed the ni4-refactor-enc-material branch from 11f9fae to a670e2b Compare January 7, 2025 13:54
@ni4 ni4 force-pushed the ni4-refactor-enc-material branch from 21c0822 to 39e3e5e Compare January 8, 2025 13:00
@ni4 ni4 requested review from desvxx, ronaldtse and maxirmx January 8, 2025 13:40
@ni4 ni4 requested a review from antonsviridenko January 8, 2025 13:41
Copy link
Member

@maxirmx maxirmx left a comment

Choose a reason for hiding this comment

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

There are multiple constructs where a set of dynamic_cast's in placed inside case statement. I marked it in rnp.cpp but I saw it in stream-dump.cpp and other files

While not an error, such pattern (dynamic_cast's inside case) clearly indicate an option to create a virtual function.

May be you can consider it fot future refactoring

return add_json_mpis(jso, "r", &material.eg.r, "s", &material.eg.s, NULL);
case PGP_PKA_DSA:
return add_json_mpis(jso, "r", &material.dsa.r, "s", &material.dsa.s, NULL);
case PGP_PKA_ELGAMAL_ENCRYPT_OR_SIGN: {
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this piece of code call for virtual function

something like

virtual mpi* SigMaterial::get_mpi() = 0;

and appropriate redefinitions for EgSigMaterial et al

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the way it is currently done as well. The problem here is that different signature types may have different MPIs. In this case it is simple, one or two, but for EncMaterial and KeyMaterial it is more complicated, however I think we should use the same approach in both cases. Don't know a good solution yet.

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