Skip to content

feat(rdmacm): add wrapper for rdma_migrate_id#115

Open
dragonJACson wants to merge 1 commit into
dev/add-private-datafrom
dev/add-migrate-id
Open

feat(rdmacm): add wrapper for rdma_migrate_id#115
dragonJACson wants to merge 1 commit into
dev/add-private-datafrom
dev/add-migrate-id

Conversation

@dragonJACson

@dragonJACson dragonJACson commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

For #112

@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 introduces the ability to migrate an Identifier to a different EventChannel by wrapping the identifier's event channel in a Mutex and implementing the migrate method using rdma_migrate_id. It also adds corresponding error types, a test helper wait_for_cm_event, and unit tests. However, a critical lifetime and memory safety issue was identified: migrating an identifier can lead to a use-after-free or undefined behavior if there are unacknowledged events from the old event channel, as Event does not currently keep its source EventChannel alive. It is recommended to update the Event struct to hold an Arc<EventChannel> to prevent this.

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
@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.78261% with 6 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/rdmacm/communication_manager.rs 94.78% 6 Missing ⚠️
Files with missing lines Coverage Δ
src/rdmacm/communication_manager.rs 88.64% <94.78%> (+1.12%) ⬆️
🚀 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

Adds a safe Rust wrapper around rdma_migrate_id(3) in the rdmacm communication manager layer, enabling an Identifier (rdma_cm_id) to be migrated to a different EventChannel while keeping the underlying channel alive across migrations.

Changes:

  • Introduced Identifier::migrate plus MigrateError/MigrateErrorKind to wrap rdma_migrate_id.
  • Updated Identifier to hold its current EventChannel in a Mutex<Arc<EventChannel>> to keep lifetimes correct across migrations and serialize concurrent migrations.
  • Added/updated tests with a small polling-based helper (wait_for_cm_event) and new test coverage for event delivery after migration.

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

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 1 out of 1 changed files in this pull request and generated no new comments.

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