Skip to content

Refactor backend to use only one return code instead of 3#8494

Closed
justin-stephenson wants to merge 5 commits intoSSSD:masterfrom
justin-stephenson:failover_redesign_remove_dp_err
Closed

Refactor backend to use only one return code instead of 3#8494
justin-stephenson wants to merge 5 commits intoSSSD:masterfrom
justin-stephenson:failover_redesign_remove_dp_err

Conversation

@justin-stephenson
Copy link
Contributor

  • Remove DP_ERR_* codes
  • Reduce sssd.dataprovider methods to single 'output' return code

Copy link

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

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 is a large-scale refactoring to simplify error handling by removing the DP_ERR_* error codes and unifying them into a single error code system. The changes span many files and involve updating function signatures, data structures, and call sites. While the majority of the changes appear correct and consistent with the goal, I've found a few critical issues where function calls were not updated to their new signatures, which will lead to compilation errors. There is also a case where an error return value is no longer checked. These issues need to be addressed before this pull request can be merged.

Note: Security Review did not run due to the size of the PR.

@alexey-tikhonov alexey-tikhonov added the no-backport This should go to target branch only. label Mar 4, 2026
@SSSD SSSD deleted a comment from gemini-code-assist bot Mar 5, 2026
@justin-stephenson justin-stephenson force-pushed the failover_redesign_remove_dp_err branch 2 times, most recently from 58fb63c to 76bb8cb Compare March 5, 2026 14:31
@justin-stephenson
Copy link
Contributor Author

/gemini review

Copy link

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

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 refactors the data provider error handling by removing the DP_ERR_* codes and using a single error code for replies. The changes are extensive and affect many provider implementations. While the refactoring is a good step towards simplification, I've found a recurring critical issue where the actual data provider error code is ignored in several places, potentially leading to silent failures. I've also noted a case where a change in logging reduces the amount of useful information. Please address these points to ensure the refactoring is robust.

@justin-stephenson justin-stephenson force-pushed the failover_redesign_remove_dp_err branch from 76bb8cb to 02debbe Compare March 5, 2026 15:02
@justin-stephenson
Copy link
Contributor Author

/gemini review

Copy link

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

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 is a large-scale refactoring to simplify the data provider's error handling by using a single return code instead of three. The changes are extensive and touch many files, removing the DP_ERR_* error codes and replacing them with a more unified error reporting mechanism. The implementation looks solid and consistent across the codebase. I've found one minor issue in error logging where strerror is used incorrectly, which could lead to unhelpful log messages. My specific comment addresses this.

@justin-stephenson justin-stephenson force-pushed the failover_redesign_remove_dp_err branch 2 times, most recently from 6898535 to e853630 Compare March 5, 2026 20:54
@justin-stephenson
Copy link
Contributor Author

/gemini review

Copy link

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

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 is a large-scale refactoring to simplify error handling by removing the DP_ERR_* codes and using a single return code. The changes are extensive and touch many files. While the overall direction is good and most changes are correct, I've found a recurring critical issue in several places where an incorrect variable is used for error reporting, which could lead to silent failures. Additionally, there are a couple of minor issues with logging format strings and content that could affect debuggability. I've provided specific comments and suggestions for these issues.

Assisted-By: Claude Code (Sonnet 4.5)
Assisted-By: Claude Code (Sonnet 4.5)
if (err_min == ENOENT) branch was unreachable code, fix this
and update to the updated single sssd error return code.
@justin-stephenson justin-stephenson force-pushed the failover_redesign_remove_dp_err branch from e853630 to ede7f2b Compare March 6, 2026 13:57
@justin-stephenson
Copy link
Contributor Author

  • Remove DP_ERR_* codes
  • Reduce sssd.dataprovider methods to single 'output' argument, removing error and error_message output args (getAccountDomain still needs domain_name arg)

Notes:

  • Value of 'ret' and 'dp_error' returned from _recv() functions in provider can be non-matching error codes, functions like groups_get_done() perform conditional branching based on this behavior.

  • Gemini points out various places in provider code where updated dp_reply_std_set() is called passing 'ret' as an argument instead of 'dp_error' and dp_error becomes lost (See function calls with comment /* TODO For backward compatibility we always return EOK to DP now. */) . This is the same as previous behavior, I left it as-is.

@justin-stephenson justin-stephenson marked this pull request as ready for review March 6, 2026 16:45
@justin-stephenson
Copy link
Contributor Author

@pbrezina Could you review this?

@justin-stephenson
Copy link
Contributor Author

I started working on porting LDAP to the new failover code, and now I understand the implementation of this PR needs to be modified. I will close this for now.

@justin-stephenson justin-stephenson removed Waiting for review no-backport This should go to target branch only. labels Mar 12, 2026
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