Skip to content

Fix Rust query_xfer_backend API #827

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

Closed
wants to merge 2 commits into from
Closed

Fix Rust query_xfer_backend API #827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9db57bc
Fix Rust query_xfer_backend API
evgeny-leksikov Sep 25, 2025
caca28c
Merge branch 'main' into rust_query_xfer_backend_fix
evgeny-leksikov Sep 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/api/cpp/nixl.h
View file
Open in desktop
Copy link
Contributor

@mkhazraee mkhazraee Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We should not add APIs to nixl just for this. I don't understand why in our cpp wrapper we can't call getType on the nixlBackendH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because nixl API defines nixlBackendH as an opaque type.
Dependency of the wrapper on internal core/agent_data.h may help here but IMO "get backend type" public API looks useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @evgeny-leksikov is right here, I don't see how it can be done w/o adding a new API, because of the opaque handle type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's ignore this one API then for now, as it's not used by users we know of as of today. Adding an API to C++ and Python just to handle Rust doesn't sound the best, while they can easily handle it. Also we might do a large refactor and add methods to the xferReqH object itself, will see, and we can decide on it then.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mkhazraee do you have any users of this API?
Otherwise we'll close this PR..

Copy link
Contributor

Choose a reason for hiding this comment

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

Not as of now specifically, if we have to add API to main agent for this, it's not wroth it. We can circle back to this when doing the minor API revisioning. Closing the PR.

Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,15 @@ class nixlAgent {
createBackend (const nixl_backend_t &type,
const nixl_b_params_t &params,
nixlBackendH* &backend);
/**
* @brief Get the type of a backend
*
* @param backend Backend handle
* @param type [out] Backend type
Copy link
Contributor

Choose a reason for hiding this comment

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

Alignment

* @return nixl_status_t Error code if call was not successful
*/
nixl_status_t
getBackendType(const nixlBackendH *backend, nixl_backend_t &type) const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Line break

/**
* @brief Register a memory/storage with NIXL. If a list of backends hints is provided
* (via extra_params), the registration is limited to the specified backends.
Expand Down
34 changes: 28 additions & 6 deletions src/bindings/rust/src/agent.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,23 +931,45 @@ impl Agent {
/// * `req` - Transfer request handle after `post_xfer_req`
///
/// # Returns
/// A handle to the backend used for the transfer
/// Name of the backend used for the transfer
Copy link
Contributor

Choose a reason for hiding this comment

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

Type?

///
/// # Errors
/// Returns a NixlError if the operation fails
pub fn query_xfer_backend(&self, req: &XferRequest) -> Result<Backend, NixlError> {
let mut backend = std::ptr::null_mut();
pub fn query_xfer_backend(&self, req: &XferRequest) -> Result<String, NixlError> {
let mut backend_type_ptr: *mut std::ffi::c_void = std::ptr::null_mut();
let mut backend_type_size: usize = 0;

let inner_guard = self.inner.write().unwrap();
let status = unsafe {
nixl_capi_query_xfer_backend(
nixl_capi_query_xfer_backend_type(
inner_guard.handle.as_ptr(),
req.handle(),
&mut backend
&mut backend_type_ptr,
&mut backend_type_size,
)
};
match status {
NIXL_CAPI_SUCCESS => {
Ok(Backend{ inner: NonNull::new(backend).ok_or(NixlError::FailedToCreateBackend)? })
if backend_type_ptr.is_null() || backend_type_size == 0 {
return Err(NixlError::BackendError);
}

// Extract the backend type string from the binary data
let backend_type = unsafe {
let slice = std::slice::from_raw_parts(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe

CStr::from_ptr(backend_type_ptr as *const 8)
                        .to_string_lossy()
                        .into_owned()
```?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c-str may lose data because it's null-teminated. Need to keep c++ like string with length

backend_type_ptr as *const u8,
backend_type_size
);
// Handle potential embedded nulls and convert to String
String::from_utf8_lossy(slice).to_string()
};

// Verify this backend type exists in our backends map
if inner_guard.backends.contains_key(&backend_type) {
Ok(backend_type)
} else {
Err(NixlError::BackendError)
}
}
NIXL_CAPI_ERROR_INVALID_PARAM => Err(NixlError::InvalidParam),
_ => Err(NixlError::BackendError),
Expand Down
4 changes: 2 additions & 2 deletions src/bindings/rust/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,8 @@ use bindings::{
nixl_capi_query_resp_list_size, nixl_capi_query_resp_list_has_value,
nixl_capi_query_resp_list_get_params, nixl_capi_prep_xfer_dlist, nixl_capi_release_xfer_dlist_handle,
nixl_capi_make_xfer_req, nixl_capi_get_local_partial_md,
nixl_capi_send_local_partial_md, nixl_capi_query_xfer_backend, nixl_capi_opt_args_set_ip_addr,
nixl_capi_opt_args_set_port
nixl_capi_send_local_partial_md, nixl_capi_opt_args_set_ip_addr,
nixl_capi_opt_args_set_port, nixl_capi_query_xfer_backend_type
};

// Re-export status codes
Expand Down
6 changes: 3 additions & 3 deletions src/bindings/rust/tests/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1324,10 +1324,10 @@ fn test_query_xfer_backend_success() {
None
).expect("Failed to create transfer request");
// Query which backend will be used for this transfer
let result: Result<Backend, NixlError> = agent1.query_xfer_backend(&xfer_req);
let result: Result<String, NixlError> = agent1.query_xfer_backend(&xfer_req);
assert!(result.is_ok(), "query_xfer_backend failed with error: {:?}", result.err());
let backend = result.unwrap();
println!("Transfer will use backend: {:?}", backend);
let backend_name = result.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

name -> type? (consistency)

println!("Transfer will use backend: {}", backend_name);
}
}
#[test]
Expand Down
26 changes: 26 additions & 0 deletions src/bindings/rust/wrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1507,6 +1507,32 @@ nixl_capi_query_xfer_backend(nixl_capi_agent_t agent,
}
}

nixl_capi_status_t
nixl_capi_query_xfer_backend_type(nixl_capi_agent_t agent,
nixl_capi_xfer_req_t req_hndl,
void **backend_type,
size_t *backend_type_size) {
if (!agent || !req_hndl || !backend_type || !backend_type_size) {
return NIXL_CAPI_ERROR_INVALID_PARAM;
}

nixl_capi_backend_t backend_handle;
auto ret = nixl_capi_query_xfer_backend(agent, req_hndl, &backend_handle);
if (ret != NIXL_CAPI_SUCCESS) {
return ret;
}

static thread_local nixl_backend_t type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is static thread_local really needed? It doesn't appear anywhere else in this file..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's needed to avoid memory management in rust layer and thread safety in wrapper

auto nixl_ret = agent->inner->getBackendType(backend_handle->backend, type);
Copy link
Contributor

Choose a reason for hiding this comment

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

try { ... here

if (nixl_ret != NIXL_SUCCESS) {
return NIXL_CAPI_ERROR_BACKEND;
}

*backend_type = type.data();
*backend_type_size = type.size();
return NIXL_CAPI_SUCCESS;
}

nixl_capi_status_t
nixl_capi_destroy_xfer_req(nixl_capi_xfer_req_t req)
{
Expand Down
6 changes: 6 additions & 0 deletions src/bindings/rust/wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,12 @@ nixl_capi_query_xfer_backend(nixl_capi_agent_t agent,
nixl_capi_xfer_req_t req_hndl,
nixl_capi_backend_t *backend);

nixl_capi_status_t
nixl_capi_query_xfer_backend_type(nixl_capi_agent_t agent,
Copy link
Contributor

Choose a reason for hiding this comment

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

query -> get?

nixl_capi_xfer_req_t req_hndl,
void **backend_type,
size_t *backend_type_size);

nixl_capi_status_t nixl_capi_release_xfer_req(nixl_capi_agent_t agent, nixl_capi_xfer_req_t req);

nixl_capi_status_t nixl_capi_destroy_xfer_req(nixl_capi_xfer_req_t req);
Expand Down
16 changes: 16 additions & 0 deletions src/core/nixl_agent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,22 @@ nixlAgent::createBackend(const nixl_backend_t &type,
return NIXL_ERR_BACKEND;
}

nixl_status_t
nixlAgent::getBackendType(const nixlBackendH *backend, nixl_backend_t &type) const {
if (!backend) {
NIXL_ERROR_FUNC << "backend handle is not provided";
return NIXL_ERR_INVALID_PARAM;
}

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the try.
It is doesn't exist anywhere else in this file.
And it is never used for getType()..

type = backend->getType();
return NIXL_SUCCESS;
}
catch (...) {
return NIXL_ERR_BACKEND;
}
}

nixl_status_t
nixlAgent::queryMem(const nixl_reg_dlist_t &descs,
std::vector<nixl_query_resp_t> &resp,
Expand Down