Skip to content

Refactor stream-dump to C++. #2345

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Refactor stream-dump to C++. #2345

wants to merge 7 commits into from
+697 −706

Conversation

ni4
Copy link
Contributor

@ni4 ni4 commented Apr 24, 2025

As title says

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 24, 2025
View reviewed changes
src/librepgp/stream-dump.cpp
}

void
DumpContextDst::dump_signature_subpacket(const pkt::sigsub::Raw &subpkt)

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 168 lines.

Copilot Autofix

AI about 1 hour ago

To address the issue, we will add comments to document the purpose and behavior of the DumpContextDst::dump_signature_subpacket function. Specifically, we will:

  1. Add a high-level comment above the function to describe its overall purpose and how it fits into the larger system.
  2. Add inline comments within the function to explain the logic of each case in the switch statement, particularly the nuances of handling different subpacket types.
  3. Ensure that the comments are concise but informative, focusing on the "why" behind the code rather than the "what," which is often self-evident.

These changes will improve the function's readability and maintainability without altering its functionality.

Suggested changeset 1
src/librepgp/stream-dump.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/librepgp/stream-dump.cpp b/src/librepgp/stream-dump.cpp
--- a/src/librepgp/stream-dump.cpp
+++ b/src/librepgp/stream-dump.cpp
@@ -633,2 +633,13 @@
 
+/**
+ * DumpContextDst::dump_signature_subpacket
+ * ----------------------------------------
+ * This function processes and outputs information about a signature subpacket.
+ * It identifies the subpacket type, extracts relevant data, and formats it for
+ * output using the destination context (`dst`). The function supports various
+ * subpacket types, such as creation time, expiration time, and exportable
+ * certification, and handles each type accordingly.
+ *
+ * @param subpkt The raw signature subpacket to process.
+ */
 void
@@ -636,6 +647,9 @@
 {
+    // Lookup the subpacket type name for display purposes
     auto sname = id_str_pair::lookup(sig_subpkt_type_map, subpkt.raw_type(), "Unknown");
 
+    // Handle the subpacket based on its type
     switch (subpkt.type()) {
     case pkt::sigsub::Type::CreationTime: {
+        // Process a subpacket containing the creation time of the signature
         auto &sub = dynamic_cast<const pkt::sigsub::CreationTime &>(subpkt);
@@ -645,2 +659,3 @@
     case pkt::sigsub::Type::ExpirationTime: {
+        // Process a subpacket containing the expiration time of the signature
         auto &sub = dynamic_cast<const pkt::sigsub::ExpirationTime &>(subpkt);
@@ -650,2 +665,3 @@
     case pkt::sigsub::Type::ExportableCert: {
+        // Process a subpacket indicating whether the certification is exportable
         auto &sub = dynamic_cast<const pkt::sigsub::ExportableCert &>(subpkt);
EOF
@@ -633,2 +633,13 @@

/**
* DumpContextDst::dump_signature_subpacket
* ----------------------------------------
* This function processes and outputs information about a signature subpacket.
* It identifies the subpacket type, extracts relevant data, and formats it for
* output using the destination context (`dst`). The function supports various
* subpacket types, such as creation time, expiration time, and exportable
* certification, and handles each type accordingly.
*
* @param subpkt The raw signature subpacket to process.
*/
void
@@ -636,6 +647,9 @@
{
// Lookup the subpacket type name for display purposes
auto sname = id_str_pair::lookup(sig_subpkt_type_map, subpkt.raw_type(), "Unknown");

// Handle the subpacket based on its type
switch (subpkt.type()) {
case pkt::sigsub::Type::CreationTime: {
// Process a subpacket containing the creation time of the signature
auto &sub = dynamic_cast<const pkt::sigsub::CreationTime &>(subpkt);
@@ -645,2 +659,3 @@
case pkt::sigsub::Type::ExpirationTime: {
// Process a subpacket containing the expiration time of the signature
auto &sub = dynamic_cast<const pkt::sigsub::ExpirationTime &>(subpkt);
@@ -650,2 +665,3 @@
case pkt::sigsub::Type::ExportableCert: {
// Process a subpacket indicating whether the certification is exportable
auto &sub = dynamic_cast<const pkt::sigsub::ExportableCert &>(subpkt);
Copilot is powered by AI and may make mistakes. Always verify output.
src/librepgp/stream-dump.cpp
static rnp_result_t
stream_dump_packets_raw(rnp_dump_ctx_t *ctx, pgp_source_t *src, pgp_dest_t *dst)
rnp_result_t
DumpContextDst::dump_raw_packets()

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 135 lines.

Copilot Autofix

AI about 1 hour ago

To fix the issue, we will add comments to the DumpContextDst::dump_raw_packets function to document its purpose, logic, and key steps. This includes:

  1. Adding a function-level comment to describe the overall purpose of the function.
  2. Adding inline comments to explain significant blocks of code, such as the handling of nested layers, the main loop, and error conditions.
  3. Ensuring that the comments are concise and relevant, without overwhelming the code.
Suggested changeset 1
src/librepgp/stream-dump.cpp

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/librepgp/stream-dump.cpp b/src/librepgp/stream-dump.cpp
--- a/src/librepgp/stream-dump.cpp
+++ b/src/librepgp/stream-dump.cpp
@@ -1447,6 +1447,11 @@
 {
-    char         msg[1024 + PGP_MAX_HEADER_SIZE] = {0};
-    char         smsg[128] = {0};
-    rnp_result_t ret = RNP_ERROR_GENERIC;
+    // This function processes and dumps raw OpenPGP packets from the source stream.
+    // It handles nested packet layers, ensuring that the maximum nesting level is not exceeded.
+    // The function returns RNP_SUCCESS on successful processing or an appropriate error code.
+
+    char         msg[1024 + PGP_MAX_HEADER_SIZE] = {0}; // Buffer for packet messages
+    char         smsg[128] = {0}; // Buffer for short messages
+    rnp_result_t ret = RNP_ERROR_GENERIC; // Default return value
 
+    // Check if the source stream has reached the end
     if (src.eof()) {
@@ -1455,3 +1460,3 @@
 
-    /* do not allow endless recursion */
+    // Prevent infinite recursion by limiting the nesting level of OpenPGP packets
     if (++layers > MAXIMUM_NESTING_LEVEL) {
@@ -1462,3 +1467,5 @@
 
+    // Main loop to process packets until the end of the source stream
     while (!src.eof()) {
+        // Read and process each packet (details omitted for brevity)
         pgp_packet_hdr_t hdr{};
@@ -1578,2 +1585,4 @@
     }
+
+    // Return success after processing all packets
     return RNP_SUCCESS;
EOF
@@ -1447,6 +1447,11 @@
{
char msg[1024 + PGP_MAX_HEADER_SIZE] = {0};
char smsg[128] = {0};
rnp_result_t ret = RNP_ERROR_GENERIC;
// This function processes and dumps raw OpenPGP packets from the source stream.
// It handles nested packet layers, ensuring that the maximum nesting level is not exceeded.
// The function returns RNP_SUCCESS on successful processing or an appropriate error code.

char msg[1024 + PGP_MAX_HEADER_SIZE] = {0}; // Buffer for packet messages
char smsg[128] = {0}; // Buffer for short messages
rnp_result_t ret = RNP_ERROR_GENERIC; // Default return value

// Check if the source stream has reached the end
if (src.eof()) {
@@ -1455,3 +1460,3 @@

/* do not allow endless recursion */
// Prevent infinite recursion by limiting the nesting level of OpenPGP packets
if (++layers > MAXIMUM_NESTING_LEVEL) {
@@ -1462,3 +1467,5 @@

// Main loop to process packets until the end of the source stream
while (!src.eof()) {
// Read and process each packet (details omitted for brevity)
pgp_packet_hdr_t hdr{};
@@ -1578,2 +1585,4 @@
}

// Return success after processing all packets
return RNP_SUCCESS;
Copilot is powered by AI and may make mistakes. Always verify output.
@codecov Codecov
Copy link

codecov bot commented Apr 24, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 95.90164% with 15 lines in your changes missing coverage. Please review.

Project coverage is 85.43%. Comparing base (3e31477) to head (4a1ae65).

Files with missing lines Patch % Lines
src/librepgp/stream-dump.cpp 95.58% 15 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2345      +/-   ##
==========================================
+ Coverage   85.39%   85.43%   +0.04%     
==========================================
  Files         125      126       +1     
  Lines       22740    22719      -21     
==========================================
- Hits        19418    19410       -8     
+ Misses       3322     3309      -13

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ni4 ni4 force-pushed the ni4-refactor-stream-dumping branch from d08c6d5 to d0b60c2 Compare April 25, 2025 14:44
@ni4 ni4 force-pushed the ni4-refactor-stream-dumping branch from d0b60c2 to d0d0bd2 Compare April 26, 2025 08:16
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 26, 2025
View reviewed changes
src/librepgp/stream-dump.cpp
@@ -264,56 +264,48 @@
indent_dst_close(pgp_dest_t *dst, bool discard)
{
pgp_dest_indent_param_t *param = (pgp_dest_indent_param_t *) dst->param;
if (!param) {
return;
if (param) {

Check notice

Code scanning / CodeQL

Guarded Free Note

unnecessary NULL check before call to
free
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ni4