Skip to content

Conversation

lplewa
Copy link
Contributor

@lplewa lplewa commented Jun 11, 2025

fixes: #1365

Description

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used
  • All API changes are reflected in docs and def/map files, and are tested

@lplewa lplewa requested a review from a team as a code owner June 11, 2025 14:15
@KFilipek KFilipek requested a review from Copilot June 13, 2025 12:45
Copy link

@Copilot Copilot AI left a comment

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 updates the controller (ctl) APIs to use the new umf_result_t return type and standardized result codes (such as UMF_RESULT_SUCCESS and UMF_RESULT_ERROR_INVALID_ARGUMENT) instead of plain integers. The changes span various modules including unit tests, debug handlers, provider modules, pool management, and the ctl core functions to improve API consistency and error handling.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/ctl/ctl_unittest.cpp Updated test assertions to compare against UMF_RESULT_SUCCESS
test/ctl/ctl_debug.c Changed handler return types to umf_result_t and updated error codes
src/provider/provider_os_memory.c Adjusted read handler to return UMF_RESULT_SUCCESS instead of int
src/provider/provider_ctl_stats_impl.h Updated stats handlers to return umf_result_t
src/pool/pool_disjoint.c Modified name handlers to return standardized error codes
src/memory_provider.c Updated subtree handler to use umf_result_t
src/memory_pool.c Revised pool query and subtree functions for umf_result_t returns
src/libumf.c Adjusted ctl query calls and removed redundant ternary error mapping
src/ctl/ctl.h Updated function prototypes to use umf_result_t
src/ctl/ctl.c Refactored query execution and config load functions to use umf_result_t
Comments suppressed due to low confidence (3)

test/ctl/ctl_unittest.cpp:51

  • [nitpick] Consider checking for a specific error code rather than merely asserting not equal to UMF_RESULT_SUCCESS, to make the failure condition more explicit.
ASSERT_NE(ctl_query(ctl_handler, NULL, CTL_QUERY_PROGRAMMATIC, "debug.heap.non_existent", CTL_QUERY_READ, &value, sizeof(value)), UMF_RESULT_SUCCESS);

src/ctl/ctl.c:364

  • [nitpick] Consider clarifying the error propagation logic by mapping specific errors from lower-level functions instead of defaulting to UMF_RESULT_ERROR_UNKNOWN.
umf_result_t ret = UMF_RESULT_ERROR_UNKNOWN;

src/libumf.c:117

  • [nitpick] Review the change from using a ternary operator for error mapping to returning the raw result from ctl_query; ensure this behavior aligns with the intended error handling semantics.
return ctl_query(NULL, ctx, CTL_QUERY_PROGRAMMATIC, name, CTL_QUERY_WRITE, arg, size);

Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

PLease re-base

@lplewa lplewa merged commit 24610f5 into oneapi-src:main Jun 16, 2025
173 of 174 checks passed
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.

Fix ctl error handling

4 participants