Skip to content

Conversation

@OmriRitblat
Copy link
Collaborator

Change --aggressiveness-level to medium

Description

Please provide a summary of the change.

What

Subject: what this PR is doing in one line.

Why ?

Justification for the PR. If there is existing issue/bug please reference.

How ?

It is optional but for complex PRs please provide information about the design,
architecture, approach, etc.

Change type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Tests
  • Other

Check list

  • Code follows the style de facto guidelines of this project
  • Comments have been inserted in hard to understand places
  • Documentation has been updated (if necessary)
  • Test has been added (if possible)

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Changed Coverity static analysis aggressiveness level from low to medium in the CI pipeline script.

Key Changes:

  • Modified cov-analyze command parameter from --aggressiveness-level low to --aggressiveness-level medium in contrib/jenkins_tests/cov.sh:43
  • This increases the depth and thoroughness of Coverity's static analysis, potentially catching more defects at the cost of slightly longer analysis time
  • No functional code changes, only CI configuration adjustment

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • Simple configuration change to a CI script that increases static analysis thoroughness. No impact on runtime code, build process, or functionality. The change is well-contained, clearly documented in the PR title referencing HPCINFRA-3898, and aligns with improving code quality detection.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
contrib/jenkins_tests/cov.sh 5/5 Changed Coverity analysis aggressiveness level from low to medium - simple configuration adjustment

Sequence Diagram

sequenceDiagram
    participant CI as CI Pipeline
    participant CovScript as cov.sh Script
    participant CovAnalyze as Coverity Analyze
    participant CovDB as Coverity Build DB
    
    CI->>CovScript: Execute coverity analysis
    CovScript->>CovScript: Configure & build project
    CovScript->>CovAnalyze: cov-analyze --aggressiveness-level medium
    Note over CovAnalyze: Changed from low to medium<br/>More thorough analysis
    CovAnalyze->>CovDB: Analyze compiled code
    CovDB-->>CovAnalyze: Return defects
    CovAnalyze-->>CovScript: Analysis complete
    CovScript->>CovScript: Format errors & generate report
    CovScript-->>CI: Return exit code
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@OmriRitblat
Copy link
Collaborator Author

bot:retest

@greptile-apps
Copy link

greptile-apps bot commented Nov 12, 2025

Greptile Summary

  • Increased Coverity aggressiveness level from low to medium and fixed multiple issues flagged by Coverity analysis
  • Added null pointer checks, return value validations, and Coverity suppression annotations across socket and hardware queue files

Confidence Score: 1/5

  • This PR contains critical logic bugs that will cause incorrect runtime behavior
  • Two critical logic errors were introduced: inverted success/failure check in sockinfo.cpp:1877 and broken return value handling in SYSCALL macro at sock-redirect.h:91
  • src/core/sock/sockinfo.cpp and src/core/sock/sock-redirect.h require immediate fixes for logic errors

Important Files Changed

Filename Overview
src/core/sock/sockinfo.cpp Contains critical inverted logic bug at line 1877 - logs "Failed" when detach succeeds, added null checks and Coverity annotations
src/core/sock/sock-redirect.h Modified SYSCALL macro with statement expression but has semicolon issue that breaks return values, added Coverity pragma support
src/core/sock/sockinfo_tcp.cpp Added null pointer checks for parent_listen_context, poor formatting at line 2908-2910, added Coverity annotations

Sequence Diagram

sequenceDiagram
    participant App as "Application"
    participant SI as "sockinfo"
    participant RFS as "rfs"
    participant HWQ as "hw_queue_rx"
    participant Ring as "Ring"
    
    App->>SI: "attach_receiver(flow_key)"
    SI->>SI: "Check for existing 3-tuple flow"
    alt 3-tuple exists
        SI->>SI: "detach_receiver(flow_key_3t)"
        Note over SI: "Logic bug: inverted check"
    end
    SI->>Ring: "Register receiver"
    
    App->>RFS: "attach_flow(sink)"
    RFS->>RFS: "create_flow()"
    alt create_flow fails
        RFS->>RFS: "Log failure"
    else create_flow succeeds
        RFS->>RFS: "Log success"
    end
    
    App->>HWQ: "configure_rq()"
    HWQ->>HWQ: "xlio_ib_mlx5_get_cq()"
    alt get_cq fails
        HWQ->>HWQ: "Log error"
    else get_cq succeeds
        HWQ->>Ring: "Create RQ with CQ"
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

6 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

if (!create_flow()) {
rfs_logdbg("Failed to create second rule for worker: %d", g_p_app->get_worker_id());
} else {
("Added second rule to worker: %d", g_p_app->get_worker_id());
Copy link

Choose a reason for hiding this comment

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

syntax: Missing function name in log statement - rfs_logdbg is missing

Suggested change
("Added second rule to worker: %d", g_p_app->get_worker_id());
rfs_logdbg("Added second rule to worker: %d", g_p_app->get_worker_id());

Comment on lines 2902 to 2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0)
return false;
Copy link

Choose a reason for hiding this comment

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

syntax: Missing closing brace and improper formatting

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0)
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile

Comment on lines +1875 to +1878
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950 in sockinfo.cpp), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Comment on lines 2902 to 2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0)
return false;
Copy link

Choose a reason for hiding this comment

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

syntax: Missing closing brace and improper formatting - the if statement is missing its closing brace

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0)
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 2902 to 2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0)
return false;
Copy link

Choose a reason for hiding this comment

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

syntax: Missing closing brace - this if statement is not properly closed which will cause compilation error

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0)
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

@OmriRitblat
Copy link
Collaborator Author

bot:retest

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines 2902 to 2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0)
return false;
Copy link

Choose a reason for hiding this comment

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

syntax: Missing closing brace - this will cause compilation failure

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0)
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Comment on lines 75 to 78
#define SYSCALL(_func, ...)
((VALID_SYSCALL(_func) ? (void)0 : get_orig_funcs()),
/* coverity[var_deref_op] */
orig_os_api._func(__VA_ARGS__))
Copy link

Choose a reason for hiding this comment

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

syntax: Macro split across lines without backslash continuation - will cause preprocessor error

Suggested change
#define SYSCALL(_func, ...)
((VALID_SYSCALL(_func) ? (void)0 : get_orig_funcs()),
/* coverity[var_deref_op] */
orig_os_api._func(__VA_ARGS__))
#define SYSCALL(_func, ...) \
((VALID_SYSCALL(_func) ? (void)0 : get_orig_funcs()), \
/* coverity[var_deref_op] */ \
orig_os_api._func(__VA_ARGS__))

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

if (!create_flow()) {
rfs_logdbg("Failed to create second rule for worker: %d", g_p_app->get_worker_id());
} else {
("Added second rule to worker: %d", g_p_app->get_worker_id());
Copy link

Choose a reason for hiding this comment

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

syntax: Missing function name - this is an orphaned function call that will cause compilation error

Suggested change
("Added second rule to worker: %d", g_p_app->get_worker_id());
rfs_logdbg("Added second rule to worker: %d", g_p_app->get_worker_id());

Comment on lines +1875 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950 in sockinfo.cpp), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Comment on lines +2902 to +2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
Copy link

Choose a reason for hiding this comment

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

syntax: Missing closing brace - this will cause compilation failure

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

7 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1875 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950 returns result of destroy_nd_resources, which returns true on success at line 1061), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Comment on lines +2902 to +2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
Copy link

Choose a reason for hiding this comment

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

style: Formatting needs improvement - add space after if, around !=, and proper indentation

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1875 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on success (line 950 returns result of destroy_nd_resources, which returns true on success at line 1061), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Comment on lines +2902 to +2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
Copy link

Choose a reason for hiding this comment

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

style: Inconsistent formatting - missing space after if and around !=

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1875 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950 returns result of destroy_nd_resources which returns true on success at line 1061), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Comment on lines +2902 to +2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
Copy link

Choose a reason for hiding this comment

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

style: Missing space after if and around != operator - inconsistent with codebase formatting

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

((VALID_SYSCALL(_func) ? (void)0 : get_orig_funcs()), orig_os_api._func(__VA_ARGS__))
#define SYSCALL(_func, ...) \
((VALID_SYSCALL(_func) ? (void)0 : get_orig_funcs()), \
COVERITY_VAR_DEREF_OP \
Copy link

Choose a reason for hiding this comment

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

syntax: COVERITY_VAR_DEREF_OP is undefined - will cause compilation error. Should be a comment:

Suggested change
COVERITY_VAR_DEREF_OP \
/* coverity[var_deref_op] */ \

Comment on lines +1875 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1875 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950 returns result of destroy_nd_resources, which returns true on success at line 1061), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Comment on lines +2902 to +2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
Copy link

Choose a reason for hiding this comment

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

style: Inconsistent formatting - missing space after if and around != operator

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

10 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1875 to +1878
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950 returns result of destroy_nd_resources which returns true on success at line 1061), so this logs "Failed" when detach actually succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Comment on lines +2902 to +2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
Copy link

Choose a reason for hiding this comment

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

style: Missing space after if and around != operator - inconsistent with codebase formatting

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

13 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1875 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Comment on lines +2902 to +2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
Copy link

Choose a reason for hiding this comment

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

style: Add space after if, around !=, and wrap in braces for consistency with codebase style

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

15 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

get_orig_funcs(); \
} \
COVERITY_VAR_DEREF_OP \
orig_os_api._func(__VA_ARGS__); \
Copy link

Choose a reason for hiding this comment

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

syntax: The semicolon at the end prevents this statement expression from returning a value. GNU statement expressions require the last expression to be without a semicolon to return its value.

Suggested change
orig_os_api._func(__VA_ARGS__); \
orig_os_api._func(__VA_ARGS__)

Comment on lines +1875 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: Inverted logic - detach_receiver returns true on SUCCESS (line 950 returns result of destroy_nd_resources, which returns true on success at line 1061), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

20 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1877 to +1878
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
Copy link

Choose a reason for hiding this comment

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

logic: inverted logic - detach_receiver returns true on SUCCESS, so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

20 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1877 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: inverted logic - detach_receiver returns true on SUCCESS (line 950 returns result of destroy_nd_resources, which returns true on success at line 1061), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

NirWolfer and others added 5 commits November 19, 2025 18:23
Change --aggressiveness-level to medium

Signed-off-by: NirWolfer <[email protected]>
Fix multiple Coverity-reported CHECKED_RETURN issues across core modules.

Several functions did not properly check the return values of internal
calls, leading to potential missed error handling or silent failures.
This patch adds explicit return-value checks (and error handling where
appropriate) to ensure robust error propagation and compliance with
Coverity CHECKED_RETURN analysis.

Fix by adding conditional checks for return values and appropriate
handling logic or logging where missing.

This change improves code reliability and consistency across the
affected modules.

Signed-off-by: Omri Ritblat <[email protected]>
Add Coverity suppression comments for repeated FORWARD_NULL false positives.

Coverity reported multiple potential null dereferences in syscall-related
macros. Added `/* coverity[var_deref_op] */` to suppress these false
positives. No functional changes.

Signed-off-by: Omri Ritblat <[email protected]>
Fix build errors in src/core/sock/sock-redirect.h and
src/core/sock/sockinfo_tcp.cpp by updating declarations and adjusting
dependencies to restore successful compilation.

Signed-off-by: Omri Ritblat <[email protected]>
Fix multiple Coverity-reported CHECKED_RETURN issues across core modules.

Added explicit suppression comments and (void) casts to document intentional
ignoring of return values and ensure consistency with Coverity expectations.

Updated files:
- src/core/event/entity_context.cpp
- src/core/proto/neighbour.cpp
- src/core/sock/sockinfo_tcp.cpp

Signed-off-by: Omri Ritblat <[email protected]>
Fix Coverity-reported FORWARD_NULL warnings by adding suppression comments
to document intentional null forwarding and eliminate false positives.

Updated file:
- src/core/sock/sock-redirect.h

Signed-off-by: Omri Ritblat <[email protected]>
Fix Coverity-reported var_deref_op warnings by adding macro-safe Coverity
_Pragma() suppressions inside the SYSCALL wrapper to document intentional
pointer dereferencing and eliminate false positives.

Updated file:
- src/core/sock/sock-redirect.h

Signed-off-by: Omri Ritblat <[email protected]>
Fix Coverity-reported null pointer dereference warnings by adding
explicit null checks and Coverity suppression comments to document
intentional pointer dereferencing after validation, eliminating false
positives.

Changes:
- Refactored SYSCALL macro to use statement expression with explicit
  if/else for clearer control flow to Coverity static analyzer
- Added suppression comment for dbrec pointer which is guaranteed valid
  after successful xlio_ib_mlx5dv_init_obj() call
- Added suppression comment for p_iov pointer which is validated before
  reaching OS transmit fallback path

Updated files:
- src/core/sock/sock-redirect.h
- src/core/ib/mlx5/ib_mlx5.cpp
- libxlio/src/core/ib/mlx5/ib_mlx5.cpp
- src/core/sock/sockinfo_udp.cpp
- libxlio/src/core/sock/sockinfo_udp.cpp

Signed-off-by: Omri Ritblat <[email protected]>
Coverity NULL_RETURNS fixes

Add NULL pointer checks before dereferencing return values from
functions that can return NULL:

- json_object_get_string(): Check before std::string construction
- get_parent_listen_context(): Check before accessing parent methods
- strdup(): Check for allocation failure
- m_used_containers.back(): Check for empty container list

These defensive checks prevent crashes when:
- JSON schema has malformed type fields
- Memory allocation fails
- Parent context is uninitialized (non-RSS sockets)
- Container allocation fails

Signed-off-by: Omri Ritblat <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

20 files reviewed, 3 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Comment on lines +1877 to +1879
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
Copy link

Choose a reason for hiding this comment

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

logic: inverted logic - detach_receiver returns true on SUCCESS (line 950 returns result of destroy_nd_resources, which returns true on success at line 1061), so this logs "Failed" when detach succeeds

Suggested change
if (detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}
if (!detach_receiver(detach_key)) {
si_logdbg("Failed to detach receiver for socket %p", this);
}

get_orig_funcs(); \
} \
COVERITY_VAR_DEREF_OP \
orig_os_api._func(__VA_ARGS__); \
Copy link

Choose a reason for hiding this comment

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

logic: semicolon prevents statement expression from returning value - GNU statement expressions need last statement without semicolon to return its value (SYSCALL is used to capture return values in utils.cpp:479, 493, etc.)

Suggested change
orig_os_api._func(__VA_ARGS__); \
orig_os_api._func(__VA_ARGS__)

Comment on lines +2908 to +2910
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
Copy link

Choose a reason for hiding this comment

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

style: missing space after if, around !=, and body should be in braces for consistency

Suggested change
if(unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6)!=0))
return false;
if (unlikely(tcp_bind(&m_pcb, reinterpret_cast<const ip_addr_t *>(&m_bound.get_ip_addr()),
ntohs(m_bound.get_in_port()), m_pcb.is_ipv6) != 0)) {
return false;
}

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