Skip to content

feat: add rdma_reject, rdma_get_devices and address getters#116

Open
dragonJACson wants to merge 2 commits into
dev/add-private-datafrom
sideway-cm-improve
Open

feat: add rdma_reject, rdma_get_devices and address getters#116
dragonJACson wants to merge 2 commits into
dev/add-private-datafrom
sideway-cm-improve

Conversation

@dragonJACson

Copy link
Copy Markdown
Contributor

No description provided.

@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 adds support for rejecting connections, querying RDMA devices, and retrieving local and remote ports and socket addresses from an Identifier. The review feedback highlights several critical compilation issues, including calls to an undefined helper function port_from_raw_addr in tests, and incorrect pointer usage (self.cm_id.as_ref() instead of self.cm_id.as_ptr()) when calling FFI functions. Additionally, the get_devices function needs to be updated to correctly handle cases where no RDMA devices are present on the system without returning an error.

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
Comment thread src/rdmacm/communication_manager.rs
Comment thread src/rdmacm/communication_manager.rs
Comment thread src/rdmacm/communication_manager.rs
Comment on lines +730 to +747
pub fn get_devices() -> Result<Vec<Arc<DeviceContext>>, GetDevicesError> {
let mut num_devices = 0;
let devices = unsafe { rdma_get_devices(&mut num_devices) };
let devices = NonNull::new(devices).ok_or_else(|| GetDevicesErrorKind::Rdmacm(io::Error::last_os_error()))?;
let devices = RdmaDeviceList { devices };
let num_devices = usize::try_from(num_devices).map_err(|_| GetDevicesErrorKind::InvalidDeviceCount(num_devices))?;

let contexts = unsafe { std::slice::from_raw_parts(devices.devices.as_ptr(), num_devices) };
contexts
.iter()
.enumerate()
.map(|(index, &context)| {
NonNull::new(context)
.map(cached_device_context)
.ok_or_else(|| GetDevicesErrorKind::NullDeviceContext(index).into())
})
.collect()
}

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.

medium

If there are no RDMA devices on the system, rdma_get_devices returns NULL and sets num_devices to 0. Currently, this function treats any NULL return as an error, which will cause a false positive error when no devices are present. Instead, check if num_devices is 0 when devices is NULL and return an empty Vec in that case.

pub fn get_devices() -> Result<Vec<Arc<DeviceContext>>, GetDevicesError> {
    let mut num_devices = 0;
    let devices = unsafe { rdma_get_devices(&mut num_devices) };
    if devices.is_null() {
        if num_devices == 0 {
            return Ok(Vec::new());
        }
        return Err(GetDevicesErrorKind::Rdmacm(io::Error::last_os_error()).into());
    }
    let devices = RdmaDeviceList { devices: unsafe { NonNull::new_unchecked(devices) } };
    let num_devices = usize::try_from(num_devices).map_err(|_| GetDevicesErrorKind::InvalidDeviceCount(num_devices))?;

    let contexts = unsafe { std::slice::from_raw_parts(devices.devices.as_ptr(), num_devices) };
    contexts
        .iter()
        .enumerate()
        .map(|(index, &context)| {
            NonNull::new(context)
                .map(cached_device_context)
                .ok_or_else(|| GetDevicesErrorKind::NullDeviceContext(index).into())
        })
        .collect()
}

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.70270% with 33 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/rdmacm/communication_manager.rs 77.70% 33 Missing ⚠️
Files with missing lines Coverage Δ
src/rdmacm/communication_manager.rs 85.48% <77.70%> (-2.08%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

1 participant