Skip to content

feat(rdmacm): support setting private data in rdmacm conn param#114

Open
dragonJACson wants to merge 2 commits into
mainfrom
dev/add-private-data
Open

feat(rdmacm): support setting private data in rdmacm conn param#114
dragonJACson wants to merge 2 commits into
mainfrom
dev/add-private-data

Conversation

@dragonJACson

@dragonJACson dragonJACson commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

For #111

Signed-off-by: Luke Yue <lukedyue@gmail.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request removes the Cirrus CI configuration, adds new keywords to Cargo.toml, and introduces helper methods in src/rdmacm/communication_manager.rs to retrieve event private data and configure connection parameters. Feedback on the changes highlights two critical safety issues: first, unconditionally accessing the param.conn union member in Event::private_data can cause undefined behavior for datagram port spaces or invalid event types; second, storing a raw pointer to a short-lived slice in ConnectionParameter::setup_private_data without lifetime tracking can lead to use-after-free bugs, meaning the function should be marked as unsafe or refactored.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/rdmacm/communication_manager.rs
Comment thread src/rdmacm/communication_manager.rs
@codecov

codecov Bot commented Jun 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.30612% with 36 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/rdmacm/communication_manager.rs 85.30% 36 Missing ⚠️
Files with missing lines Coverage Δ
src/rdmacm/communication_manager.rs 87.52% <85.30%> (+80.34%) ⬆️

... and 7 files with indirect coverage changes

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

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the RDMA CM wrapper to support sending and receiving connection private data via RDMA CM connection parameters/events, and updates CI metadata to better validate RDMA functionality.

Changes:

  • Add Event::private_data() to expose peer-provided private data on connection events.
  • Add ConnectionParameter::setup_private_data() to configure private data for connect/accept.
  • Update CI/config metadata (GitHub Actions RXE test job, crate keywords, remove Cirrus CI config).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File Description
src/rdmacm/communication_manager.rs Adds APIs to set connection private data and to read private data from connection events (plus extensive docs).
Cargo.toml Extends crate keywords for discoverability.
.github/workflows/test.yml Adds a new RXE-based CI job to run RDMA tests in a simulated environment and upload coverage.
.cirrus.yml Removes Cirrus CI configuration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/rdmacm/communication_manager.rs
Comment thread src/rdmacm/communication_manager.rs
Comment thread src/rdmacm/communication_manager.rs Outdated
Comment thread .github/workflows/test.yml
Comment thread src/rdmacm/communication_manager.rs Outdated
Comment thread src/rdmacm/communication_manager.rs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread .github/workflows/test.yml
Comment thread src/rdmacm/communication_manager.rs
@dragonJACson dragonJACson force-pushed the dev/add-private-data branch 2 times, most recently from 63340c3 to d44d8da Compare June 9, 2026 01:04
Signed-off-by: Luke Yue <lukedyue@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread Cargo.toml
repository = "https://github.com/RDMA-Rust/sideway"
readme = "README.md"
keywords = ["RDMA", "verbs", "cm", "libibverbs", "librdmacm"]
keywords = ["RDMA", "verbs", "cm", "libibverbs", "librdmacm", "ibverbs", "rdmacm"]
Comment thread .github/workflows/test.yml
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