Refactor backend return code by justin-stephenson · Pull Request #8862 · SSSD/sssd

justin-stephenson · 2026-06-30T15:26:23Z

Summary of changes (written by me, not AI):

Return only a single error code through sssd.dataprovider interfaces
Remove sdap_ret/sdap_err from codebase
- Handle lost/broken ENOENT after removing sdap_ret with commit sdap: Add output variable to store sdap result of ENOENT

    Object lookup sdap `_done()` functions call tevent_req_done() for EOK and ENOENT,
    removal of 'sdap_ret/sdap_err' causes us to lose ENOENT which breaks ENOENT
    checks done by some callers.
    
    Previously actual sdap return code(sdap_ret) was stored. Callers still receive
    tevent error code from tevent_req_error(), but callers need some way to check
    if the sdap lookup function results in ENOENT.

sdap_id_op_{connect}_done returns ERR_OFFLINE when offline instead of EAGAIN
sdap_id_op_done() now returns ERR_SERVER_FAILURE when previously setting DP_ERR_FATAL
sdap_id_op_done() returns EAGAIN when advising retry and dp_error is no longer, callers performing retry logic with this code:

    ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
    if (dp_error == DP_ERR_OK && ret != EOK) {

are converted to

    ret = sdap_id_op_done(state->sdap_op, ret);
    if (ret == EAGAIN) 
       /* retry */
    ...

LDAP connection failure error codes are converted from EIO/ERR_NETWORK_IO/ETIMEDOUT to ERR_SERVER_FAILURE. Callers checking for ERR_NETWORK_IO/ETIMEDOUT now check for ERR_SERVER_FAILURE
Switch checks of if (dp_error == DP_ERR_OFFLINE) to if (ret == ERR_OFFLINE)

gemini-code-assist

Code Review

This pull request refactors and simplifies error handling across SSSD providers and responders by removing the redundant dp_error (Data Provider error) parameter and unifying error reporting under standard SSSD error codes. However, this refactoring introduces a critical bug: by discarding the sdap_ret status (which contains ENOENT when a user or group is not found), lookup operations incorrectly return EOK instead of ENOENT. This breaks critical AD failover and fallback mechanisms, as the system can no longer detect missing entries to trigger alternative connection attempts.

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.

Assisted by: Claude code (Sonnet 4.5)

Replace LDAP connection related failure error codes EIO/ERR_NETWORK_IO/ETIMEDOUT with ERR_SERVER_FAILURE

The data provider handler methods now return a single output argument. Remove 'dp_error/dp_err' and 'error_message' usage across provider code. The getAccountDomain method still needs to return 'domain_name' string. Assisted by: Claude code (Sonnet 4.5)

Object lookup sdap `_done()` functions call tevent_req_done() for EOK and ENOENT, removal of 'sdap_ret/sdap_err' causes us to lose ENOENT which breaks ENOENT checks done by some callers. Previously actual sdap return code(sdap_ret) was stored. Callers still receive tevent error code from tevent_req_error(), but callers need some way to check if the sdap lookup function results in ENOENT.

pbrezina · 2026-07-14T11:14:13Z

Handle lost/broken ENOENT after removing sdap_ret with commit sdap: Add output variable to store sdap result of ENOENT

Uhm, this looks weird and breaks the purpose of having one return code. Why don't you call tevent_req_error(req, ENOENT)? This looks like the cleanest thing to do.

justin-stephenson · 2026-07-14T13:46:27Z

Handle lost/broken ENOENT after removing sdap_ret with commit sdap: Add output variable to store sdap result of ENOENT

Uhm, this looks weird and breaks the purpose of having one return code. Why don't you call tevent_req_error(req, ENOENT)? This looks like the cleanest thing to do.

Hi Pavel, thanks for taking a look at this PR.

I tried going with the approach you mention here first, but I found it to be much more invasive and require a lot more changes in the provider to adapt to this behavior change. In my testing I see much of the provider code was not written with ENOENT as tevent error in mind, and calling functions expect to see 'tevent_req_done' when the connection is successful and the ldap search does not find the object.

Of course I am okay going with your approach, in general I was attempting to lower the amount of invasive changes if possible., however this PR will be invasive in the end regardless.

pbrezina · 2026-07-14T17:17:06Z

I think we should go for it and adapt the code to correctly handle ENOENT and one return code. But we can continue with this pull request as is so we move forward and then implement this refactoring on top of it. I will continue with review tomorrow.

pbrezina · 2026-07-15T11:44:24Z

+    bool sdap_enoent = false;
    struct sdap_id_conn_ctx *user_conn;

-    ret = groups_get_recv(subreq, &state->dp_error, &state->sdap_ret);
+    ret = groups_get_recv(subreq, &sdap_enoent);
    talloc_zfree(subreq);

-    if (ret != EOK) {           /* Fatal error while looking up group */
-        tevent_req_error(req, ret);
-        return;
-    }
+    state->sdap_enoent = sdap_enoent;

-    if (state->sdap_ret == EOK) {   /* Matching group found */
+    if (ret == EOK) {   /* Matching group found */
        tevent_req_done(req);
        return;
-    } else if (state->sdap_ret != ENOENT) {
-        tevent_req_error(req, EIO);
+    } else if (ret != EOK && !sdap_enoent) {  /* Fatal error while looking up group */
+        tevent_req_error(req, ret);
        return;
    }


This nicely demonstrates how multiple return codes are bad design practice.

The current code can return EOK and sdap_enoent = true from groups_get_recv, the code behind the if statement that handles ENOENT will not execute.

pbrezina · 2026-07-15T11:45:57Z

    switch (retval) {
        case EIO:
        case ETIMEDOUT:
            /* this currently the only possible communication error after connection is established */
            communication_error = true;
            break;

        default:
            communication_error = false;
            break;
    }


EIO and ETIMEDOUT were removed and we should not get them anymore. But ERR_SERVER_FAILURE must be handled here.

pbrezina · 2026-07-15T11:47:18Z

    state->conn = conn;
-    state->dp_error = DP_ERR_FATAL;
    state->noexist_delete = noexist_delete;
+    state->sdap_enoent = true;


Should be set to false, as you later set it to true on ENOENT.

pbrezina · 2026-07-15T11:49:23Z

              "Trying LDAP search while not connected.\n");
-        ret = ERR_NETWORK_IO;
+        ret = ERR_SERVER_FAILURE;
        goto fail;


ERR_SERVER_FAILURE is switched for ERR_AUTH_FAILED in "fail".

pbrezina · 2026-07-15T11:52:03Z

        DEBUG(SSSDBG_CRIT_FAILURE,
              "Trying LDAP rebind while not connected.\n");
-        return ERR_NETWORK_IO;
+        return ERR_SERVER_FAILURE;


This is ldap callback, expecting ldap return codes. We should return LDAP_SERVER_DOWN or perhaps LDAP_LOCAL_ERROR.

pbrezina · 2026-07-15T12:01:58Z

        <method name="sudoHandler">
            <annotation name="codegen.CustomInputHandler" value="true" />
-            <arg name="dp_error" type="q" direction="out" />
            <arg name="error" type="u" direction="out" />
-            <arg name="error_message" type="s" direction="out" />
        </method>


This still returns two error codes: dbus and operation. Let's just return the dbus one.

Instead of finishing with a valid reply (dbus returns success, sends reply that contains the error code), just fail with dbus error (dbus returns error).

pbrezina · 2026-07-15T12:03:50Z

+    msg = reply->message;
+}
+
+void dp_req_reply_std_with_msg(const char *request_name,


We don't need the message in the dbus reply, we want to carry all information in the return code. Potential message can be logged in the provider.

pbrezina · 2026-07-15T12:04:48Z

+ * Data provider error code is returned:
+ *   ERR_OK - connection established
+ *   ERR_OFFLINE - backend is offline, operation result is set EAGAIN
+ *   ERR_FATAL - operation failed


ERR_SERVER_FAILURE?

pbrezina · 2026-07-15T12:05:44Z

- *   DP_ERR_FATAL - operation failed
+ * In data provider error code is returned:
+ *   ERR_OK - connection established
+ *   ERR_OFFLINE - backend is offline, operation result is set EAGAIN


not EAGAIN anymore

pbrezina · 2026-07-15T12:06:12Z

@@ -230,7 +230,6 @@ struct ad_handle_pac_initgr_state {
    struct dp_id_data *ar;
    const char *err;
    int dp_error;


pbrezina · 2026-07-15T12:12:55Z

There is /* FIXME: do we need some special handling of ENOENT */inipa_get_subdom_acct_done`. It might be good to think about it and either fix or remote the comment when we are touching this part.

justin-stephenson added the no-backport This should go to target branch only. label Jun 30, 2026

gemini-code-assist Bot reviewed Jun 30, 2026

View reviewed changes

Comment thread src/providers/ldap/ldap_id.c

Comment thread src/providers/ad/ad_id.c Outdated

justin-stephenson force-pushed the refactor_backend_return_code_master_2 branch 9 times, most recently from b171fc9 to 830cebb Compare July 1, 2026 18:38

justin-stephenson marked this pull request as ready for review July 2, 2026 14:01

justin-stephenson assigned alexey-tikhonov Jul 2, 2026

justin-stephenson requested a review from alexey-tikhonov July 2, 2026 14:01

justin-stephenson added the Waiting for review label Jul 2, 2026

justin-stephenson changed the title ~~Refactor backend return code master~~ Refactor backend return code Jul 2, 2026

justin-stephenson force-pushed the refactor_backend_return_code_master_2 branch 2 times, most recently from 698bdc1 to 0a4d279 Compare July 7, 2026 14:15

justin-stephenson added 7 commits July 13, 2026 14:11

dp: Remove sdap_ret and sdap_error

ee96156

Assisted by: Claude code (Sonnet 4.5)

util: Add ERR_NO_MORE_SERVERS

2d4c1ec

ldap: Update connection failure error codes

fc1f39b

Replace LDAP connection related failure error codes EIO/ERR_NETWORK_IO/ETIMEDOUT with ERR_SERVER_FAILURE

ad: Update connection failure error codes

904b90e

Replace LDAP connection related failure error codes EIO/ERR_NETWORK_IO/ETIMEDOUT with ERR_SERVER_FAILURE

ipa: Update connection failure error codes

28bbcc9

Replace LDAP connection related failure error codes EIO/ERR_NETWORK_IO/ETIMEDOUT with ERR_SERVER_FAILURE

ldap: Return ERR_OFFLINE instead of EAGAIN

9d32736

justin-stephenson force-pushed the refactor_backend_return_code_master_2 branch from 0a4d279 to a122699 Compare July 13, 2026 18:11

justin-stephenson force-pushed the refactor_backend_return_code_master_2 branch from a122699 to 56c8d38 Compare July 13, 2026 18:43

pbrezina self-assigned this Jul 15, 2026

pbrezina requested changes Jul 15, 2026

View reviewed changes

Uh oh!

Conversation

justin-stephenson commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

pbrezina commented Jul 14, 2026

Uh oh!

justin-stephenson commented Jul 14, 2026

Uh oh!

pbrezina commented Jul 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbrezina commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

justin-stephenson commented Jun 30, 2026 •

edited

Loading