[PATCH 0/5] Rev 3: Fix various tevent_req style and naming issues

[Simo Sorce]

This revision should address all concerns raised by Pavel for code that I
actually changed from the original. For patch 1 a calrification I can't
make was asked. I 'fixed' the code as I realized I copied a debug statement
wrapped in an if statement that wasn't in the original code I move around,
so I simply dropped it (the statement is still present but later on in another
part of the code, where it remains from the original code).

For patch for I completely changed approach and simplified the code even more by
removing the switch statement completely, Thanks Pavel, that code looks even
better to me now :)

Simo Sorce (5):
  Fix tevent_req style for krb5_auth
  Fix ipa_subdomain_id names and tevent_req style
  Fix tevent_req style for get_netgroup in ipa_id
  Streamline ipa_account_info handler
  Use an entry type mask macro to filter entry types

 src/providers/data_provider.h         |    1 +
 src/providers/ipa/ipa_id.c            |  294 +++++++--------
 src/providers/ipa/ipa_id.h            |   10 +-
 src/providers/ipa/ipa_subdomains_id.c |   75 ++---
 src/providers/krb5/krb5_access.c      |    6 +-
 src/providers/krb5/krb5_auth.c        |  683 ++++++++++++++++-----------------
 src/providers/krb5/krb5_auth.h        |    6 +-
 src/providers/krb5/krb5_wait_queue.c  |   12 +-
 src/providers/ldap/ldap_id.c          |    2 +-
 src/providers/proxy/proxy_id.c        |    2 +-
 10 files changed, 505 insertions(+), 586 deletions(-)

[PATCH 1/5] Fix tevent_req style for krb5_auth

[Simo Sorce]

No functionality changes,
just make the code respect the tevent_req style and naming conventions
and enhance readability by adding some helper functions.
---
 src/providers/krb5/krb5_access.c     |    6 +-
 src/providers/krb5/krb5_auth.c       |  683 ++++++++++++++++------------------
 src/providers/krb5/krb5_auth.h       |    6 +-
 src/providers/krb5/krb5_wait_queue.c |   12 +-
 4 files changed, 335 insertions(+), 372 deletions(-)

diff --git a/src/providers/krb5/krb5_access.c b/src/providers/krb5/krb5_access.c
index 25612807d701a392f697caa82b3f31182416b824..970633eb2f02b68cd3a3e38432c10f6a347edc91
100644
--- a/src/providers/krb5/krb5_access.c
+++ b/src/providers/krb5/krb5_access.c
@@ -38,7 +38,7 @@ struct krb5_access_state {
     bool access_allowed;
 };
 
-static void krb5_access_child_done(struct tevent_req *subreq);
+static void krb5_access_done(struct tevent_req *subreq);
 struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx,
                                     struct tevent_context *ev,
                                     struct be_ctx *be_ctx,
@@ -141,7 +141,7 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx,
         goto done;
     }
 
-    tevent_req_set_callback(subreq, krb5_access_child_done, req);
+    tevent_req_set_callback(subreq, krb5_access_done, req);
     return req;
 
 done:
@@ -154,7 +154,7 @@ done:
     return req;
 }
 
-static void krb5_access_child_done(struct tevent_req *subreq)
+static void krb5_access_done(struct tevent_req *subreq)
 {
     struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
     struct krb5_access_state *state = tevent_req_data(req,
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index a4bd631cb637211dac00b6ffcb4ed77144383afe..7104b3127e04891fed7370a6241c60865372632b
100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -259,7 +259,7 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
         return ENOMEM;
     }
     kr->is_offline = false;
-    kr->active_ccache_present = true;
+    kr->active_ccache = true;
     kr->run_as_user = true;
     talloc_set_destructor((TALLOC_CTX *) kr, krb5_cleanup);
 
@@ -271,12 +271,165 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
     return EOK;
 }
 
-static void krb5_resolve_kdc_done(struct tevent_req *subreq);
-static void krb5_resolve_kpasswd_done(struct tevent_req *subreq);
-static void krb5_find_ccache_step(struct tevent_req *req);
-static void krb5_save_ccname_done(struct tevent_req *req);
-static void krb5_child_done(struct tevent_req *req);
-static void krb5_pam_handler_cache_auth_step(struct tevent_req *req);
+
+static void krb5_auth_cache_creds(struct krb5_ctx *krb5_ctx,
+                                  struct sysdb_ctx *sysdb,
+                                  struct confdb_ctx *cdb,
+                                  struct pam_data *pd, uid_t uid,
+                                  int *pam_status, int *dp_err)
+{
+    errno_t ret;
+
+    ret = sysdb_cache_auth(sysdb, pd->user, pd->authtok,
+                           pd->authtok_size, cdb, true, NULL,
+                           NULL);
+    if (ret != EOK) {
+        DEBUG(1, ("Offline authentication failed\n"));
+        *pam_status = cached_login_pam_status(ret);
+        *dp_err = DP_ERR_OK;
+        return;
+    }
+
+    ret = add_user_to_delayed_online_authentication(krb5_ctx, pd, uid);
+    if (ret != EOK) {
+        /* This error is not fatal */
+        DEBUG(1, ("add_user_to_delayed_online_authentication failed.\n"));
+    }
+    *pam_status = PAM_AUTHINFO_UNAVAIL;
+    *dp_err = DP_ERR_OFFLINE;
+}
+
+static errno_t krb5_auth_prepare_ccache_file(struct krb5child_req *kr,
+                                             struct be_ctx *be_ctx,
+                                             int *pam_status, int *dp_err)
+{
+    const char *ccname_template;
+    bool private_path = false;
+    errno_t ret;
+
+    if (!kr->is_offline) {
+        kr->is_offline = be_is_offline(be_ctx);
+    }
+
+    /* The ccache file should be (re)created if one of the following conditions
+     * is true:
+     * - it doesn't exist (kr->ccname == NULL)
+     * - the backend is online and the current ccache file is not used, i.e
+     *   the related user is currently not logged in and it is not a renewal
+     *   request
+     *   (!kr->is_offline && !kr->active_ccache &&
kr->pd->cmd != SSS_CMD_RENEW)
+     * - the backend is offline and the current cache file not used and
+     *   it does not contain a valid tgt
+     *   (kr->is_offline && !kr->active_ccache &&
!kr->valid_tgt)
+     */
+    if (kr->ccname == NULL ||
+        (kr->is_offline && !kr->active_ccache && !kr->valid_tgt)
||
+        (!kr->is_offline && !kr->active_ccache && kr->pd->cmd
!= SSS_CMD_RENEW)) {
+            DEBUG(9, ("Recreating  ccache file.\n"));
+            ccname_template = dp_opt_get_cstring(kr->krb5_ctx->opts,
+                                                 KRB5_CCNAME_TMPL);
+            kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
+                                                be_ctx->domain->case_sensitive,
+                                                &private_path);
+            if (kr->ccname == NULL) {
+                DEBUG(1, ("expand_ccname_template failed.\n"));
+                return ENOMEM;
+            }
+
+            if (kr->cc_be == NULL) {
+                kr->cc_be = get_cc_be_ops_ccache(kr->ccname);
+            }
+            if (kr->cc_be == NULL) {
+                DEBUG(SSSDBG_CRIT_FAILURE,
+                      ("Cannot get operations on new ccache %s\n",
kr->ccname));
+                return EINVAL;
+            }
+
+            ret = kr->cc_be->create(kr->ccname,
+                                    kr->krb5_ctx->illegal_path_re,
+                                    kr->uid, kr->gid, private_path);
+            if (ret != EOK) {
+                DEBUG(SSSDBG_OP_FAILURE, ("ccache creation failed.\n"));
+                return ret;
+            }
+    } else {
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              ("Saved ccache %s if of different type than ccache in "
+               "configuration file, reusing the old ccache\n",
+               kr->old_ccname));
+
+        kr->cc_be = get_cc_be_ops_ccache(kr->old_ccname);
+        if (kr->cc_be == NULL) {
+            DEBUG(SSSDBG_CRIT_FAILURE,
+                  ("Cannot get operations on saved ccache %s\n",
+                   kr->old_ccname));
+            return EINVAL;
+        }
+    }
+
+    return EOK;
+}
+
+static void krb5_auth_store_creds(struct sysdb_ctx *sysdb, struct pam_data *pd)
+{
+    TALLOC_CTX *tmp_ctx;
+    char *password = NULL;
+    int ret = EOK;
+
+    tmp_ctx = talloc_new(NULL);
+    if (!tmp_ctx) {
+        DEBUG(0, ("Out of memory when trying to store credentials\n"));
+        return;
+    }
+
+    switch(pd->cmd) {
+        case SSS_CMD_RENEW:
+            /* The authtok is set to the credential cache
+             * during renewal. We don't want to save this
+             * as the cached password.
+             */
+            break;
+        case SSS_PAM_AUTHENTICATE:
+        case SSS_PAM_CHAUTHTOK_PRELIM:
+            password = talloc_size(tmp_ctx, pd->authtok_size + 1);
+            if (password != NULL) {
+                memcpy(password, pd->authtok, pd->authtok_size);
+                password[pd->authtok_size] = '\0';
+            }
+            break;
+        case SSS_PAM_CHAUTHTOK:
+            password = talloc_size(tmp_ctx, pd->newauthtok_size + 1);
+            if (password != NULL) {
+                memcpy(password, pd->newauthtok, pd->newauthtok_size);
+                password[pd->newauthtok_size] = '\0';
+            }
+            break;
+        default:
+            DEBUG(0, ("unsupported PAM command [%d].\n", pd->cmd));
+    }
+
+    if (password == NULL) {
+        if (pd->cmd != SSS_CMD_RENEW) {
+            DEBUG(0, ("password not available, offline auth may not
work.\n"));
+            /* password caching failures are not fatal errors */
+        }
+        talloc_zfree(tmp_ctx);
+        return;
+    }
+
+    talloc_set_destructor((TALLOC_CTX *)password, password_destructor);
+
+    ret = sysdb_cache_password(sysdb, pd->user, password);
+    if (ret) {
+        DEBUG(2, ("Failed to cache password, offline auth may not work."
+                  " (%d)[%s]!?\n", ret, strerror(ret)));
+        /* password caching failures are not fatal errors */
+    }
+
+    talloc_zfree(tmp_ctx);
+}
+
+/* krb5_auth request */
 
 struct krb5_auth_state {
     struct tevent_context *ev;
@@ -286,24 +439,14 @@ struct krb5_auth_state {
     struct krb5_ctx *krb5_ctx;
     struct krb5child_req *kr;
 
+    bool search_kpasswd;
+
     int pam_status;
     int dp_err;
 };
 
-int krb5_auth_recv(struct tevent_req *req, int *pam_status, int *dp_err)
-{
-    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-
-    *pam_status = state->pam_status;
-    *dp_err = state->dp_err;
-
-    TEVENT_REQ_RETURN_ON_ERROR(req);
-
-    return EOK;
-}
-
-static struct tevent_req *krb5_next_kdc(struct tevent_req *req);
-static struct tevent_req *krb5_next_kpasswd(struct tevent_req *req);
+static void krb5_auth_resolve_done(struct tevent_req *subreq);
+static void krb5_auth_done(struct tevent_req *subreq);
 
 struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
                                   struct tevent_context *ev,
@@ -465,22 +608,22 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
                                                   NULL);
         if (ccache_file != NULL) {
             ret = check_old_ccache(ccache_file, kr, realm,
-                                   &kr->active_ccache_present,
-                                   &kr->valid_tgt_present);
+                                   &kr->active_ccache,
+                                   &kr->valid_tgt);
             if (ret != EOK) {
                 DEBUG(SSSDBG_CRIT_FAILURE,
                       ("check_if_ccache_file_is_used failed.\n"));
                 goto done;
             }
         } else {
-            kr->active_ccache_present = false;
-            kr->valid_tgt_present = false;
+            kr->active_ccache = false;
+            kr->valid_tgt = false;
             DEBUG(4, ("No ccache file for user [%s] found.\n", pd->user));
         }
         DEBUG(9, ("Ccache_file is [%s] and is %s active and TGT is %s
valid.\n",
                   ccache_file ? ccache_file : "not set",
-                  kr->active_ccache_present ? "" : "not",
-                  kr->valid_tgt_present ? "" : "not"));
+                  kr->active_ccache ? "" : "not",
+                  kr->valid_tgt ? "" : "not"));
         if (ccache_file != NULL) {
             kr->ccname = ccache_file;
             kr->old_ccname = talloc_strdup(kr, ccache_file);
@@ -505,12 +648,16 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
     kr->srv = NULL;
     kr->kpasswd_srv = NULL;
 
-    subreq = krb5_next_kdc(req);
+    state->search_kpasswd = false;
+    subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
+                                    state->krb5_ctx->service->name,
+                                    state->kr->srv == NULL ? true : false);
     if (!subreq) {
-        DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_next_kdc failed.\n"));
+        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed resolver request.\n"));
         ret = EIO;
         goto done;
     }
+    tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
 
     return req;
 
@@ -524,168 +671,86 @@ done:
     return req;
 }
 
-static void krb5_resolve_kdc_done(struct tevent_req *subreq)
+static void krb5_auth_resolve_done(struct tevent_req *subreq)
 {
     struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
     struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
     struct krb5child_req *kr = state->kr;
+    char *msg;
     int ret;
 
     ret = be_resolve_server_recv(subreq, &kr->srv);
     talloc_zfree(subreq);
-    if (ret) {
-        /* all servers have been tried and none
-         * was found good, setting offline,
-         * but we still have to call the child to setup
-         * the ccache file if we are performing auth */
-        be_mark_offline(state->be_ctx);
-        kr->is_offline = true;
 
-        if (kr->pd->cmd == SSS_PAM_CHAUTHTOK ||
-            kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) {
-            DEBUG(SSSDBG_TRACE_FUNC,
-                  ("No KDC suitable for password change is available\n"));
+    if (state->search_kpasswd) {
+        if ((ret != EOK) &&
+            (kr->pd->cmd == SSS_PAM_CHAUTHTOK ||
+             kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) {
+            /* all kpasswd servers have been tried and none was found good,
+             * but the kdc seems ok. Password changes are not possible but
+             * authentication is. We return an PAM error here, but do not
+             * mark the backend offline. */
             state->pam_status = PAM_AUTHTOK_LOCK_BUSY;
             state->dp_err = DP_ERR_OK;
-            tevent_req_done(req);
-            return;
+            ret = EOK;
+            goto done;
         }
     } else {
-        if (kr->krb5_ctx->kpasswd_service != NULL) {
-            subreq = krb5_next_kpasswd(req);
-            if (subreq == NULL) {
-                DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_next_kpasswd failed.\n"));
-                ret = EIO;
-                goto failed;
-            }
-            return;
-        }
-    }
-
-    krb5_find_ccache_step(req);
-    return;
-
-failed:
-    tevent_req_error(req, ret);
-}
-
-static void krb5_resolve_kpasswd_done(struct tevent_req *subreq)
-{
-    struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
-    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-    int ret;
-
-    ret = be_resolve_server_recv(subreq, &state->kr->kpasswd_srv);
-    talloc_zfree(subreq);
-    if (ret != EOK &&
-        (state->kr->pd->cmd == SSS_PAM_CHAUTHTOK ||
-         state->kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) {
-        /* all kpasswd servers have been tried and none was found good, but the
-         * kdc seems ok. Password changes are not possible but
-         * authentication is. We return an PAM error here, but do not mark the
-         * backend offline. */
-        state->pam_status = PAM_AUTHTOK_LOCK_BUSY;
-        state->dp_err = DP_ERR_OK;
-        tevent_req_done(req);
-        return;
-    }
-
-    krb5_find_ccache_step(req);
-}
-
-static void krb5_find_ccache_step(struct tevent_req *req)
-{
-    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-    int ret;
-    struct krb5child_req *kr = state->kr;
-    struct pam_data *pd = kr->pd;
-    char *msg;
-    bool private_path = false;
-    struct tevent_req *subreq = NULL;
-
-    if (!kr->is_offline) {
-        kr->is_offline = be_is_offline(state->be_ctx);
-    }
-
-    /* The ccache file should be (re)created if one of the following conditions
-     * is true:
-     * - it doesn't exist (kr->ccname == NULL)
-     * - the backend is online and the current ccache file is not used, i.e
-     *   the related user is currently not logged in and it is not a renewal
-     *   request
-     *   (!kr->is_offline && !kr->active_ccache_present &&
-     *    pd->cmd != SSS_CMD_RENEW)
-     * - the backend is offline and the current cache file not used and
-     *   it does not contain a valid tgt
-     *   (kr->is_offline &&
-     *    !kr->active_ccache_present && !kr->valid_tgt_present)
-     */
-    if (kr->ccname == NULL ||
-        (kr->is_offline && !kr->active_ccache_present &&
-            !kr->valid_tgt_present) ||
-        (!kr->is_offline && !kr->active_ccache_present &&
-         pd->cmd != SSS_CMD_RENEW)) {
-            DEBUG(9, ("Recreating  ccache file.\n"));
-            kr->ccname = expand_ccname_template(kr, kr,
-                                          dp_opt_get_cstring(kr->krb5_ctx->opts,
-                                                             KRB5_CCNAME_TMPL),
-                                                true,
-                                               
state->be_ctx->domain->case_sensitive,
-                                                &private_path);
-            if (kr->ccname == NULL) {
-                DEBUG(1, ("expand_ccname_template failed.\n"));
-                ret = ENOMEM;
+        if (ret != EOK) {
+            /* all servers have been tried and none
+             * was found good, setting offline,
+             * but we still have to call the child to setup
+             * the ccache file if we are performing auth */
+            be_mark_offline(state->be_ctx);
+            kr->is_offline = true;
+
+            if (kr->pd->cmd == SSS_PAM_CHAUTHTOK ||
+                kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) {
+                DEBUG(SSSDBG_TRACE_FUNC,
+                      ("No KDC suitable for password change is available\n"));
+                state->pam_status = PAM_AUTHTOK_LOCK_BUSY;
+                state->dp_err = DP_ERR_OK;
+                ret = EOK;
                 goto done;
             }
-
-            if (!kr->cc_be) {
-                kr->cc_be = get_cc_be_ops_ccache(kr->ccname);
-                if (kr->cc_be == NULL) {
-                    DEBUG(SSSDBG_CRIT_FAILURE,
-                          ("Cannot get operations on new ccache %s\n",
-                           kr->ccname));
-                    ret = EINVAL;
+        } else {
+            if (kr->krb5_ctx->kpasswd_service != NULL) {
+                state->search_kpasswd = true;
+                subreq = be_resolve_server_send(state,
+                                    state->ev, state->be_ctx,
+                                    state->krb5_ctx->kpasswd_service->name,
+                                    kr->kpasswd_srv == NULL ? true : false);
+                if (subreq == NULL) {
+                    DEBUG(SSSDBG_CRIT_FAILURE, ("Resolver request
failed.\n"));
+                    ret = EIO;
                     goto done;
                 }
+                tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
+                return;
             }
-
-            ret = kr->cc_be->create(kr->ccname,
-                                    kr->krb5_ctx->illegal_path_re,
-                                    kr->uid, kr->gid, private_path);
-            if (ret != EOK) {
-                DEBUG(SSSDBG_OP_FAILURE, ("ccache creation failed.\n"));
-                goto done;
-            }
-    } else {
-        DEBUG(SSSDBG_MINOR_FAILURE,
-              ("Saved ccache %s if of different type than ccache in "
-               "configuration file, reusing the old ccache\n",
-               kr->old_ccname));
-
-        kr->cc_be = get_cc_be_ops_ccache(kr->old_ccname);
-        if (kr->cc_be == NULL) {
-            DEBUG(SSSDBG_CRIT_FAILURE,
-                  ("Cannot get operations on saved ccache %s\n",
-                   kr->old_ccname));
-            ret = EINVAL;
-            goto done;
         }
+    }
 
+    ret = krb5_auth_prepare_ccache_file(kr, state->be_ctx,
+                                        &state->pam_status,
&state->dp_err);
+    if (ret) {
+        goto done;
     }
 
     if (kr->is_offline) {
         DEBUG(9, ("Preparing for offline operation.\n"));
 
-        if (kr->valid_tgt_present || kr->active_ccache_present) {
+        if (kr->valid_tgt || kr->active_ccache) {
             DEBUG(9, ("Valid TGT available or "
                       "ccache file is already in use.\n"));
             kr->ccname = kr->old_ccname;
-            msg = talloc_asprintf(pd, "%s=%s", CCACHE_ENV_NAME,
kr->ccname);
+            msg = talloc_asprintf(kr->pd,
+                                  "%s=%s", CCACHE_ENV_NAME, kr->ccname);
             if (msg == NULL) {
                 DEBUG(1, ("talloc_asprintf failed.\n"));
             } else {
-                ret = pam_add_response(pd, SSS_PAM_ENV_ITEM, strlen(msg) + 1,
-                                       (uint8_t *) msg);
+                ret = pam_add_response(kr->pd, SSS_PAM_ENV_ITEM,
+                                       strlen(msg) + 1, (uint8_t *) msg);
                 if (ret != EOK) {
                     DEBUG(1, ("pam_add_response failed.\n"));
                 }
@@ -693,12 +758,15 @@ static void krb5_find_ccache_step(struct tevent_req *req)
 
             if (dp_opt_get_bool(kr->krb5_ctx->opts,
                                 KRB5_STORE_PASSWORD_IF_OFFLINE)) {
-                krb5_pam_handler_cache_auth_step(req);
-                return;
+                krb5_auth_cache_creds(state->kr->krb5_ctx,
+                                      state->be_ctx->sysdb,
+                                      state->be_ctx->cdb,
+                                      kr->pd, kr->uid,
+                                      &state->pam_status, &state->dp_err);
+            } else {
+                state->pam_status = PAM_AUTHINFO_UNAVAIL;
+                state->dp_err = DP_ERR_OFFLINE;
             }
-
-            state->pam_status = PAM_AUTHINFO_UNAVAIL;
-            state->dp_err = DP_ERR_OFFLINE;
             ret = EOK;
             goto done;
 
@@ -712,7 +780,7 @@ static void krb5_find_ccache_step(struct tevent_req *req)
      * case we can drop the privileges, too. */
     if ((dp_opt_get_bool(kr->krb5_ctx->opts, KRB5_VALIDATE) ||
          kr->krb5_ctx->use_fast) &&
-        !kr->is_offline) {
+        (!kr->is_offline)) {
         kr->run_as_user = false;
     } else {
         kr->run_as_user = true;
@@ -724,8 +792,7 @@ static void krb5_find_ccache_step(struct tevent_req *req)
         ret = ENOMEM;
         goto done;
     }
-
-    tevent_req_set_callback(subreq, krb5_child_done, req);
+    tevent_req_set_callback(subreq, krb5_auth_done, req);
     return;
 
 done:
@@ -736,14 +803,10 @@ done:
     }
 }
 
-static struct tevent_req *krb5_next_server(struct tevent_req *req);
-static struct tevent_req *krb5_next_kpasswd(struct tevent_req *req);
-
-static void krb5_child_done(struct tevent_req *subreq)
+static void krb5_auth_done(struct tevent_req *subreq)
 {
     struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
     struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-
     struct krb5child_req *kr = state->kr;
     struct pam_data *pd = state->pd;
     int ret;
@@ -751,21 +814,58 @@ static void krb5_child_done(struct tevent_req *subreq)
     ssize_t len = -1;
     struct krb5_child_response *res;
     const char *store_ccname;
+    struct fo_server *search_srv;
 
     ret = handle_child_recv(subreq, pd, &buf, &len);
     talloc_zfree(subreq);
-    if (ret != EOK) {
+    if (ret == ETIMEDOUT) {
+
+        DEBUG(1, ("child timed out!\n"));
+
+        switch (pd->cmd) {
+        case SSS_PAM_AUTHENTICATE:
+        case SSS_CMD_RENEW:
+            state->search_kpasswd = false;
+            search_srv = kr->srv;
+            break;
+        case SSS_PAM_CHAUTHTOK:
+        case SSS_PAM_CHAUTHTOK_PRELIM:
+            if (state->kr->kpasswd_srv) {
+                state->search_kpasswd = true;
+                search_srv = kr->kpasswd_srv;
+                break;
+            } else {
+                state->search_kpasswd = false;
+                search_srv = kr->srv;
+                break;
+            }
+        default:
+            DEBUG(1, ("Unexpected PAM task\n"));
+            ret = EINVAL;
+            goto done;
+        }
+
+        be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
+                              search_srv, PORT_NOT_WORKING);
+        subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
+                                        state->krb5_ctx->kpasswd_service->name,
+                                        search_srv == NULL ? true : false);
+        if (subreq == NULL) {
+            DEBUG(1, ("Failed resolved request.\n"));
+            ret = ENOMEM;
+            goto done;
+        }
+        tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
+        return;
+
+    } else if (ret != EOK) {
+
         DEBUG(1, ("child failed (%d [%s])\n", ret, strerror(ret)));
-        if (ret == ETIMEDOUT) {
-            if (krb5_next_server(req) == NULL) {
-                tevent_req_error(req, ENOMEM);
-            }
-        } else {
-            tevent_req_error(req, ret);
-        }
-        return;
+        goto done;
     }
 
+    /* EOK */
+
     ret = parse_krb5_child_response(state, buf, len, pd,
                         state->be_ctx->domain->pwd_expiration_warning,
                         &res);
@@ -831,7 +931,7 @@ static void krb5_child_done(struct tevent_req *subreq)
      * cache and disk if it is not actively used anymore. This will allow to
      * create a new random ccache if sshd with privilege separation is used. */
     if (res->msg_status == PAM_NEW_AUTHTOK_REQD) {
-        if (pd->cmd == SSS_PAM_AUTHENTICATE && !kr->active_ccache_present)
{
+        if (pd->cmd == SSS_PAM_AUTHENTICATE && !kr->active_ccache) {
             if (kr->old_ccname != NULL) {
                 ret = safe_remove_old_ccache_file(kr->cc_be, kr->upn,
                                                   kr->old_ccname, "dummy");
@@ -872,9 +972,16 @@ static void krb5_child_done(struct tevent_req *subreq)
                                   state->krb5_ctx->service->name,
                                   kr->kpasswd_srv, PORT_NOT_WORKING);
             /* ..try to resolve next kpasswd server */
-            if (krb5_next_kpasswd(req) == NULL) {
-                tevent_req_error(req, ENOMEM);
+            state->search_kpasswd = true;
+            subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
+                                state->krb5_ctx->kpasswd_service->name,
+                                state->kr->kpasswd_srv == NULL ?  true : false);
+            if (subreq == NULL) {
+                DEBUG(1, ("Resolver request failed.\n"));
+                ret = ENOMEM;
+                goto done;
             }
+            tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
             return;
         } else {
             be_fo_set_port_status(state->be_ctx,
@@ -892,9 +999,16 @@ static void krb5_child_done(struct tevent_req *subreq)
             be_fo_set_port_status(state->be_ctx,
state->krb5_ctx->service->name,
                                   kr->srv, PORT_NOT_WORKING);
             /* ..try to resolve next KDC */
-            if (krb5_next_kdc(req) == NULL) {
-                tevent_req_error(req, ENOMEM);
+            state->search_kpasswd = false;
+            subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
+                                            state->krb5_ctx->service->name,
+                                            kr->srv == NULL ?  true : false);
+            if (subreq == NULL) {
+                DEBUG(1, ("Resolver request failed.\n"));
+                ret = ENOMEM;
+                goto done;
             }
+            tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
             return;
         }
     } else if (kr->srv != NULL) {
@@ -950,157 +1064,25 @@ static void krb5_child_done(struct tevent_req *subreq)
         }
     }
 
-    krb5_save_ccname_done(req);
-
-    return;
-
-done:
-    if (ret == EOK) {
-        tevent_req_done(req);
-    } else {
-        tevent_req_error(req, ret);
-    }
-}
-
-static struct tevent_req *krb5_next_server(struct tevent_req *req)
-{
-    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-    struct pam_data *pd = state->pd;
-    struct tevent_req *next_req = NULL;
-
-    switch (pd->cmd) {
-        case SSS_PAM_AUTHENTICATE:
-        case SSS_CMD_RENEW:
-            be_fo_set_port_status(state->be_ctx,
state->krb5_ctx->service->name,
-                                  state->kr->srv, PORT_NOT_WORKING);
-            next_req = krb5_next_kdc(req);
-            break;
-        case SSS_PAM_CHAUTHTOK:
-        case SSS_PAM_CHAUTHTOK_PRELIM:
-            if (state->kr->kpasswd_srv) {
-                be_fo_set_port_status(state->be_ctx,
state->krb5_ctx->service->name,
-                                      state->kr->kpasswd_srv, PORT_NOT_WORKING);
-                next_req = krb5_next_kpasswd(req);
-                break;
-            } else {
-                be_fo_set_port_status(state->be_ctx,
state->krb5_ctx->service->name,
-                                      state->kr->srv, PORT_NOT_WORKING);
-                next_req = krb5_next_kdc(req);
-                break;
-            }
-        default:
-            DEBUG(1, ("Unexpected PAM task\n"));
-    }
-
-    return next_req;
-}
-
-static struct tevent_req *krb5_next_kdc(struct tevent_req *req)
-{
-    struct tevent_req *next_req;
-    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-
-    next_req = be_resolve_server_send(state, state->ev,
-                                      state->be_ctx,
-                                      state->krb5_ctx->service->name,
-                                      state->kr->srv == NULL ? true : false);
-    if (next_req == NULL) {
-        DEBUG(1, ("be_resolve_server_send failed.\n"));
-        return NULL;
-    }
-    tevent_req_set_callback(next_req, krb5_resolve_kdc_done, req);
-
-    return next_req;
-}
-
-static struct tevent_req *krb5_next_kpasswd(struct tevent_req *req)
-{
-    struct tevent_req *next_req;
-    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-
-    next_req = be_resolve_server_send(state, state->ev,
-                                state->be_ctx,
-                                state->krb5_ctx->kpasswd_service->name,
-                                state->kr->kpasswd_srv == NULL ? true : false);
-    if (next_req == NULL) {
-        DEBUG(1, ("be_resolve_server_send failed.\n"));
-        return NULL;
-    }
-    tevent_req_set_callback(next_req, krb5_resolve_kpasswd_done, req);
-
-    return next_req;
-}
-
-static void krb5_save_ccname_done(struct tevent_req *req)
-{
-    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-    struct krb5child_req *kr = state->kr;
-    struct pam_data *pd = state->pd;
-    int ret;
-    char *password = NULL;
-
     if (kr->is_offline) {
-        if (dp_opt_get_bool(kr->krb5_ctx->opts,KRB5_STORE_PASSWORD_IF_OFFLINE)) {
-            krb5_pam_handler_cache_auth_step(req);
-            return;
+        if (dp_opt_get_bool(kr->krb5_ctx->opts,
+                            KRB5_STORE_PASSWORD_IF_OFFLINE)) {
+            krb5_auth_cache_creds(state->kr->krb5_ctx,
+                                  state->be_ctx->sysdb,
+                                  state->be_ctx->cdb,
+                                  state->pd, state->kr->uid,
+                                  &state->pam_status, &state->dp_err);
+        } else {
+            DEBUG(4, ("Backend is marked offline, retry later!\n"));
+            state->pam_status = PAM_AUTHINFO_UNAVAIL;
+            state->dp_err = DP_ERR_OFFLINE;
         }
-
-        DEBUG(4, ("Backend is marked offline, retry later!\n"));
-        state->pam_status = PAM_AUTHINFO_UNAVAIL;
-        state->dp_err = DP_ERR_OFFLINE;
         ret = EOK;
         goto done;
     }
 
     if (state->be_ctx->domain->cache_credentials == TRUE) {
-
-        /* password caching failures are not fatal errors */
-        state->pam_status = PAM_SUCCESS;
-        state->dp_err = DP_ERR_OK;
-
-        switch(pd->cmd) {
-            case SSS_CMD_RENEW:
-                /* The authtok is set to the credential cache
-                 * during renewal. We don't want to save this
-                 * as the cached password.
-                 */
-                break;
-            case SSS_PAM_AUTHENTICATE:
-            case SSS_PAM_CHAUTHTOK_PRELIM:
-                password = talloc_size(state, pd->authtok_size + 1);
-                if (password != NULL) {
-                    memcpy(password, pd->authtok, pd->authtok_size);
-                    password[pd->authtok_size] = '\0';
-                }
-                break;
-            case SSS_PAM_CHAUTHTOK:
-                password = talloc_size(state, pd->newauthtok_size + 1);
-                if (password != NULL) {
-                    memcpy(password, pd->newauthtok, pd->newauthtok_size);
-                    password[pd->newauthtok_size] = '\0';
-                }
-                break;
-            default:
-                DEBUG(0, ("unsupported PAM command [%d].\n", pd->cmd));
-        }
-
-        if (password == NULL) {
-            if (pd->cmd != SSS_CMD_RENEW) {
-                DEBUG(0, ("password not available, offline auth may not
work.\n"));
-                /* password caching failures are not fatal errors */
-            }
-            ret = EOK;
-            goto done;
-        }
-
-        talloc_set_destructor((TALLOC_CTX *)password, password_destructor);
-
-        ret = sysdb_cache_password(state->sysdb, pd->user, password);
-        if (ret) {
-            DEBUG(2, ("Failed to cache password, offline auth may not work."
-                      " (%d)[%s]!?\n", ret, strerror(ret)));
-            /* password caching failures are not fatal errors */
-        }
+        krb5_auth_store_creds(state->sysdb, pd);
     }
 
     state->pam_status = PAM_SUCCESS;
@@ -1116,32 +1098,15 @@ done:
 
 }
 
-static void krb5_pam_handler_cache_auth_step(struct tevent_req *req)
+int krb5_auth_recv(struct tevent_req *req, int *pam_status, int *dp_err)
 {
     struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
-    struct pam_data *pd = state->pd;
-    struct krb5_ctx *krb5_ctx = state->kr->krb5_ctx;
-    int ret;
+    *pam_status = state->pam_status;
+    *dp_err = state->dp_err;
 
-    ret = sysdb_cache_auth(state->sysdb, pd->user, pd->authtok,
-                           pd->authtok_size, state->be_ctx->cdb, true, NULL,
-                           NULL);
-    if (ret != EOK) {
-        DEBUG(1, ("Offline authentication failed\n"));
-        state->pam_status = cached_login_pam_status(ret);
-        state->dp_err = DP_ERR_OK;
-    } else {
-        ret = add_user_to_delayed_online_authentication(krb5_ctx, pd,
-                                                       state->kr->uid);
-        if (ret != EOK) {
-            /* This error is not fatal */
-            DEBUG(1, ("add_user_to_delayed_online_authentication failed.\n"));
-        }
-        state->pam_status = PAM_AUTHINFO_UNAVAIL;
-        state->dp_err = DP_ERR_OFFLINE;
-    }
+    TEVENT_REQ_RETURN_ON_ERROR(req);
 
-    tevent_req_done(req);
+    return EOK;
 }
 
 static void krb_reply(struct be_req *req, int dp_err, int result)
@@ -1149,8 +1114,8 @@ static void krb_reply(struct be_req *req, int dp_err, int result)
     req->fn(req, dp_err, result, NULL);
 }
 
-void krb5_auth_done(struct tevent_req *req);
-static void krb5_access_done(struct tevent_req *req);
+void krb5_pam_handler_auth_done(struct tevent_req *req);
+static void krb5_pam_handler_access_done(struct tevent_req *req);
 
 void krb5_pam_handler(struct be_req *be_req)
 {
@@ -1194,7 +1159,7 @@ void krb5_pam_handler(struct be_req *be_req)
                 goto done;
             }
 
-            tevent_req_set_callback(req, krb5_auth_done, be_req);
+            tevent_req_set_callback(req, krb5_pam_handler_auth_done, be_req);
             break;
         case SSS_PAM_ACCT_MGMT:
             req = krb5_access_send(be_req, be_req->be_ctx->ev, be_req->be_ctx,
@@ -1204,7 +1169,7 @@ void krb5_pam_handler(struct be_req *be_req)
                 goto done;
             }
 
-            tevent_req_set_callback(req, krb5_access_done, be_req);
+            tevent_req_set_callback(req, krb5_pam_handler_access_done, be_req);
             break;
         case SSS_PAM_SETCRED:
         case SSS_PAM_OPEN_SESSION:
@@ -1226,7 +1191,7 @@ done:
     krb_reply(be_req, dp_err, pd->pam_status);
 }
 
-void krb5_auth_done(struct tevent_req *req)
+void krb5_pam_handler_auth_done(struct tevent_req *req)
 {
     int ret;
     struct be_req *be_req = tevent_req_callback_data(req, struct be_req);
@@ -1256,7 +1221,7 @@ void krb5_auth_done(struct tevent_req *req)
     krb_reply(be_req, dp_err, pd->pam_status);
 }
 
-static void krb5_access_done(struct tevent_req *req)
+static void krb5_pam_handler_access_done(struct tevent_req *req)
 {
     int ret;
     struct be_req *be_req = tevent_req_callback_data(req, struct be_req);
diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h
index 9133472abb9ab0a42f1bac861a52f867d393e644..078a31d8e5f7a2729ada011df4643f709e150536
100644
--- a/src/providers/krb5/krb5_auth.h
+++ b/src/providers/krb5/krb5_auth.h
@@ -51,8 +51,8 @@ struct krb5child_req {
     bool is_offline;
     struct fo_server *srv;
     struct fo_server *kpasswd_srv;
-    bool active_ccache_present;
-    bool valid_tgt_present;
+    bool active_ccache;
+    bool valid_tgt;
     bool run_as_user;
     bool upn_from_different_realm;
 };
@@ -61,6 +61,7 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
                    struct krb5_ctx *krb5_ctx, struct krb5child_req **krb5_req);
 
 void krb5_pam_handler(struct be_req *be_req);
+void krb5_pam_handler_auth_done(struct tevent_req *req);
 
 struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
                                   struct tevent_context *ev,
@@ -68,7 +69,6 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
                                   struct pam_data *pd,
                                   struct krb5_ctx *krb5_ctx);
 int krb5_auth_recv(struct tevent_req *req, int *pam_status, int *dp_err);
-void krb5_auth_done(struct tevent_req *req);
 
 struct tevent_req *handle_child_send(TALLOC_CTX *mem_ctx,
                                      struct tevent_context *ev,
diff --git a/src/providers/krb5/krb5_wait_queue.c b/src/providers/krb5/krb5_wait_queue.c
index 3863b1bdc1386739ca0651278c9d941d85e46909..da1e35b7db8956950e9b1164ae983d21994dde69
100644
--- a/src/providers/krb5/krb5_wait_queue.c
+++ b/src/providers/krb5/krb5_wait_queue.c
@@ -41,20 +41,18 @@ struct queue_entry {
 static void wait_queue_auth(struct tevent_context *ev, struct tevent_timer *te,
                              struct timeval current_time, void *private_data)
 {
-    struct queue_entry *queue_entry = talloc_get_type(private_data,
-                                                      struct queue_entry);
+    struct queue_entry *qe = talloc_get_type(private_data, struct queue_entry);
     struct tevent_req *req;
 
-    req = krb5_auth_send(queue_entry->be_req,
queue_entry->be_req->be_ctx->ev,
-                         queue_entry->be_req->be_ctx, queue_entry->pd,
-                         queue_entry->krb5_ctx);
+    req = krb5_auth_send(qe->be_req, qe->be_req->be_ctx->ev,
+                         qe->be_req->be_ctx, qe->pd, qe->krb5_ctx);
     if (req == NULL) {
         DEBUG(1, ("krb5_auth_send failed.\n"));
     } else {
-        tevent_req_set_callback(req, krb5_auth_done, queue_entry->be_req);
+        tevent_req_set_callback(req, krb5_pam_handler_auth_done, qe->be_req);
     }
 
-    talloc_zfree(queue_entry);
+    talloc_zfree(qe);
 }
 
 static void wait_queue_del_cb(hash_entry_t *entry, hash_destroy_enum type,
-- 
1.7.1

[PATCH 1/5] Fix tevent_req style for krb5_auth

[Pavel Březina]

On 11/28/2012 05:24 AM, Simo Sorce wrote:
>  +static errno_t krb5_auth_prepare_ccache_file(struct krb5child_req
> *kr,
>  +                                             struct be_ctx *be_ctx,
>  +                                             int *pam_status, int *dp_err)
>  +{
>  +    const char *ccname_template;
>  +    bool private_path = false;
>  +    errno_t ret;
>  +
>  +    if (!kr->is_offline) {
>  +        kr->is_offline = be_is_offline(be_ctx);
>  +    } 
Why do we need this condition? Wouldn't be sufficient to use the 
assignment directly? Like
-    if (!kr->is_offline) {
+        kr->is_offline = be_is_offline(be_ctx);
-    }

If not, can you put a comment there?

>  +
>  +    /* The ccache file should be (re)created if one of the following conditions
>  +     * is true:
>  +     * - it doesn't exist (kr->ccname == NULL)
>  +     * - the backend is online and the current ccache file is not used, i.e
>  +     *   the related user is currently not logged in and it is not a renewal
>  +     *   request
>  +     *   (!kr->is_offline && !kr->active_ccache &&
> kr->pd->cmd != SSS_CMD_RENEW)
>  +     * - the backend is offline and the current cache file not used and
>  +     *   it does not contain a valid tgt
>  +     *   (kr->is_offline && !kr->active_ccache &&
> !kr->valid_tgt)
>  +     */
>  +    if (kr->ccname == NULL ||
>  +        (kr->is_offline && !kr->active_ccache &&
> !kr->valid_tgt) ||
>  +        (!kr->is_offline && !kr->active_ccache &&
> kr->pd->cmd != SSS_CMD_RENEW)) {
>  +            DEBUG(9, ("Recreating  ccache file.\n"));
>  +            ccname_template = dp_opt_get_cstring(kr->krb5_ctx->opts,
>  +                                                 KRB5_CCNAME_TMPL);
>  +            kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
>  +                                                be_ctx->domain->case_sensitive,
>  +                                                &private_path);
>  +            if (kr->ccname == NULL) {
>  +                DEBUG(1, ("expand_ccname_template failed.\n"));
>  +                return ENOMEM;
>  +            }
>  +
>  +            if (kr->cc_be == NULL) {
>  +                kr->cc_be = get_cc_be_ops_ccache(kr->ccname);
>  +            }
>  +            if (kr->cc_be == NULL) {
>  +                DEBUG(SSSDBG_CRIT_FAILURE,
>  +                      ("Cannot get operations on new ccache %s\n",
> kr->ccname));
>  +                return EINVAL;
>  +            } 
Can you insert this condition inside the first one please? You are 
testing returned value of get_cc_be_ops_ccache() more then actual 
content of the variable.

>  +
>  +            ret = kr->cc_be->create(kr->ccname,
>  +                                    kr->krb5_ctx->illegal_path_re,
>  +                                    kr->uid, kr->gid, private_path);
>  +            if (ret != EOK) {
>  +                DEBUG(SSSDBG_OP_FAILURE, ("ccache creation failed.\n"));
>  +                return ret;
>  +            }
>  +    } else {
>  +        DEBUG(SSSDBG_MINOR_FAILURE,
>  +              ("Saved ccache %s if of different type than ccache in "
>  +               "configuration file, reusing the old ccache\n",
>  +               kr->old_ccname));
>  +
>  +        kr->cc_be = get_cc_be_ops_ccache(kr->old_ccname);
>  +        if (kr->cc_be == NULL) {
>  +            DEBUG(SSSDBG_CRIT_FAILURE,
>  +                  ("Cannot get operations on saved ccache %s\n",
>  +                   kr->old_ccname));
>  +            return EINVAL;
>  +        }
>  +    }
>  +
>  +    return EOK;
>  +} 
Otherwise it looks good.

[PATCH 1/5] Fix tevent_req style for krb5_auth

[Simo Sorce]

On Thu, 2012-11-29 at 11:47 +0100, Pavel Březina wrote:
>  On 11/28/2012 05:24 AM, Simo Sorce wrote:
>  > +static errno_t krb5_auth_prepare_ccache_file(struct krb5child_req *kr,
>  > +                                             struct be_ctx *be_ctx,
>  > +                                             int *pam_status, int *dp_err)
>  > +{
>  > +    const char *ccname_template;
>  > +    bool private_path = false;
>  > +    errno_t ret;
>  > +
>  > +    if (!kr->is_offline) {
>  > +        kr->is_offline = be_is_offline(be_ctx);
>  > +    }
>  
>  Why do we need this condition? Wouldn't be sufficient to use the 
>  assignment directly? Like
>  -    if (!kr->is_offline) {
>  +        kr->is_offline = be_is_offline(be_ctx);
>  -    }
>  
>  If not, can you put a comment there? 
As I said already I just copied this code verbatim, I do not know why it
was done that way, and I would not be able to put a comment in there.

Please ask the original author if it is really important, but this piece
of code while odd is not changing from the original so shouldn't be
really holding up the review IMO.

>  > +    /* The ccache file should be (re)created if one of the
> following conditions
>  > +     * is true:
>  > +     * - it doesn't exist (kr->ccname == NULL)
>  > +     * - the backend is online and the current ccache file is not used, i.e
>  > +     *   the related user is currently not logged in and it is not a renewal
>  > +     *   request
>  > +     *   (!kr->is_offline && !kr->active_ccache &&
> kr->pd->cmd != SSS_CMD_RENEW)
>  > +     * - the backend is offline and the current cache file not used and
>  > +     *   it does not contain a valid tgt
>  > +     *   (kr->is_offline && !kr->active_ccache &&
> !kr->valid_tgt)
>  > +     */
>  > +    if (kr->ccname == NULL ||
>  > +        (kr->is_offline && !kr->active_ccache &&
> !kr->valid_tgt) ||
>  > +        (!kr->is_offline && !kr->active_ccache &&
> kr->pd->cmd != SSS_CMD_RENEW)) {
>  > +            DEBUG(9, ("Recreating  ccache file.\n"));
>  > +            ccname_template = dp_opt_get_cstring(kr->krb5_ctx->opts,
>  > +                                                 KRB5_CCNAME_TMPL);
>  > +            kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
>  > +                                               
> be_ctx->domain->case_sensitive,
>  > +                                                &private_path);
>  > +            if (kr->ccname == NULL) {
>  > +                DEBUG(1, ("expand_ccname_template failed.\n"));
>  > +                return ENOMEM;
>  > +            }
>  > +
>  > +            if (kr->cc_be == NULL) {
>  > +                kr->cc_be = get_cc_be_ops_ccache(kr->ccname);
>  > +            }
>  > +            if (kr->cc_be == NULL) {
>  > +                DEBUG(SSSDBG_CRIT_FAILURE,
>  > +                      ("Cannot get operations on new ccache %s\n",
> kr->ccname));
>  > +                return EINVAL;
>  > +            }
>  
>  Can you insert this condition inside the first one please? You are 
>  testing returned value of get_cc_be_ops_ccache() more then actual 
>  content of the variable. 
Uhmm I though I did ... odd, maybe this pattern was repeated and didn't
realize it. Again this is just moved around code, and I prefer not to
touch it unless there is a good reason, because changing arbitrarily
code while refactoring is just a good recipe for getting bugs in.
But in this case it is clear I can do the change if it really is
important to you.

>  > +
>  > +            ret = kr->cc_be->create(kr->ccname,
>  > +                                    kr->krb5_ctx->illegal_path_re,
>  > +                                    kr->uid, kr->gid, private_path);
>  > +            if (ret != EOK) {
>  > +                DEBUG(SSSDBG_OP_FAILURE, ("ccache creation failed.\n"));
>  > +                return ret;
>  > +            }
>  > +    } else {
>  > +        DEBUG(SSSDBG_MINOR_FAILURE,
>  > +              ("Saved ccache %s if of different type than ccache in "
>  > +               "configuration file, reusing the old ccache\n",
>  > +               kr->old_ccname));
>  > +
>  > +        kr->cc_be = get_cc_be_ops_ccache(kr->old_ccname);
>  > +        if (kr->cc_be == NULL) {
>  > +            DEBUG(SSSDBG_CRIT_FAILURE,
>  > +                  ("Cannot get operations on saved ccache %s\n",
>  > +                   kr->old_ccname));
>  > +            return EINVAL;
>  > +        }
>  > +    }
>  > +
>  > +    return EOK;
>  > +}
>  
>  Otherwise it looks good. 
Ok, let me kinow how strong you feel about point 2, for point 1 let me
know what comment would you put in there, I don' know what to put there,
doesn't makes much sense to me and could be just bad code, but didn't
want to bother during a style refactoring as I said above.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[PATCH 1/5] Fix tevent_req style for krb5_auth

[Pavel Březina]

On 11/29/2012 03:45 PM, Simo Sorce wrote:
>  On Thu, 2012-11-29 at 11:47 +0100, Pavel Březina wrote:
> > On 11/28/2012 05:24 AM, Simo Sorce wrote:
> >> +static errno_t krb5_auth_prepare_ccache_file(struct krb5child_req *kr,
> >> +                                             struct be_ctx *be_ctx,
> >> +                                             int *pam_status, int *dp_err)
> >> +{
> >> +    const char *ccname_template;
> >> +    bool private_path = false;
> >> +    errno_t ret;
> >> +
> >> +    if (!kr->is_offline) {
> >> +        kr->is_offline = be_is_offline(be_ctx);
> >> +    }
> >
> > Why do we need this condition? Wouldn't be sufficient to use the
> > assignment directly? Like
> > -    if (!kr->is_offline) {
> > +        kr->is_offline = be_is_offline(be_ctx);
> > -    }
> >
> > If not, can you put a comment there?
>
>  As I said already I just copied this code verbatim, I do not know why it
>  was done that way, and I would not be able to put a comment in there.
>
>  Please ask the original author if it is really important, but this piece
>  of code while odd is not changing from the original so shouldn't be
>  really holding up the review IMO.
>
> >> +    /* The ccache file should be (re)created if one of the following conditions
> >> +     * is true:
> >> +     * - it doesn't exist (kr->ccname == NULL)
> >> +     * - the backend is online and the current ccache file is not used, i.e
> >> +     *   the related user is currently not logged in and it is not a renewal
> >> +     *   request
> >> +     *   (!kr->is_offline && !kr->active_ccache &&
> kr->pd->cmd != SSS_CMD_RENEW)
> >> +     * - the backend is offline and the current cache file not used and
> >> +     *   it does not contain a valid tgt
> >> +     *   (kr->is_offline && !kr->active_ccache &&
> !kr->valid_tgt)
> >> +     */
> >> +    if (kr->ccname == NULL ||
> >> +        (kr->is_offline && !kr->active_ccache &&
> !kr->valid_tgt) ||
> >> +        (!kr->is_offline && !kr->active_ccache &&
> kr->pd->cmd != SSS_CMD_RENEW)) {
> >> +            DEBUG(9, ("Recreating  ccache file.\n"));
> >> +            ccname_template = dp_opt_get_cstring(kr->krb5_ctx->opts,
> >> +                                                 KRB5_CCNAME_TMPL);
> >> +            kr->ccname = expand_ccname_template(kr, kr, ccname_template,
> true,
> >> +                                               
> be_ctx->domain->case_sensitive,
> >> +                                                &private_path);
> >> +            if (kr->ccname == NULL) {
> >> +                DEBUG(1, ("expand_ccname_template failed.\n"));
> >> +                return ENOMEM;
> >> +            }
> >> +
> >> +            if (kr->cc_be == NULL) {
> >> +                kr->cc_be = get_cc_be_ops_ccache(kr->ccname);
> >> +            }
> >> +            if (kr->cc_be == NULL) {
> >> +                DEBUG(SSSDBG_CRIT_FAILURE,
> >> +                      ("Cannot get operations on new ccache %s\n",
> kr->ccname));
> >> +                return EINVAL;
> >> +            }
> >
> > Can you insert this condition inside the first one please? You are
> > testing returned value of get_cc_be_ops_ccache() more then actual
> > content of the variable.
>
>  Uhmm I though I did ... odd, maybe this pattern was repeated and didn't
>  realize it. Again this is just moved around code, and I prefer not to
>  touch it unless there is a good reason, because changing arbitrarily
>  code while refactoring is just a good recipe for getting bugs in.
>  But in this case it is clear I can do the change if it really is
>  important to you.
>
> >> +
> >> +            ret = kr->cc_be->create(kr->ccname,
> >> +                                    kr->krb5_ctx->illegal_path_re,
> >> +                                    kr->uid, kr->gid, private_path);
> >> +            if (ret != EOK) {
> >> +                DEBUG(SSSDBG_OP_FAILURE, ("ccache creation
> failed.\n"));
> >> +                return ret;
> >> +            }
> >> +    } else {
> >> +        DEBUG(SSSDBG_MINOR_FAILURE,
> >> +              ("Saved ccache %s if of different type than ccache in "
> >> +               "configuration file, reusing the old ccache\n",
> >> +               kr->old_ccname));
> >> +
> >> +        kr->cc_be = get_cc_be_ops_ccache(kr->old_ccname);
> >> +        if (kr->cc_be == NULL) {
> >> +            DEBUG(SSSDBG_CRIT_FAILURE,
> >> +                  ("Cannot get operations on saved ccache %s\n",
> >> +                   kr->old_ccname));
> >> +            return EINVAL;
> >> +        }
> >> +    }
> >> +
> >> +    return EOK;
> >> +}
> >
> > Otherwise it looks good.
>
>  Ok, let me kinow how strong you feel about point 2, for point 1 let me
>  know what comment would you put in there, I don' know what to put there,
>  doesn't makes much sense to me and could be just bad code, but didn't
>  want to bother during a style refactoring as I said above.
>
>  Simo.
>  
OK. I don't insist since you are only moving the code around.

Ack.

[PATCH 2/5] Fix ipa_subdomain_id names and tevent_req style

[Simo Sorce]

---
 src/providers/ipa/ipa_id.c            |    5 +-
 src/providers/ipa/ipa_id.h            |   10 ++--
 src/providers/ipa/ipa_subdomains_id.c |   73 +++++++++++++--------------------
 3 files changed, 36 insertions(+), 52 deletions(-)

diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
index c74268b9aadff7dd1f97d25c78993e1463f4715e..58bb1c11608acd4a7b341c80623eb37df17dec77
100644
--- a/src/providers/ipa/ipa_id.c
+++ b/src/providers/ipa/ipa_id.c
@@ -78,8 +78,7 @@ void ipa_account_info_handler(struct be_req *breq)
     ar = talloc_get_type(breq->req_data, struct be_acct_req);
 
     if (strcasecmp(ar->domain, breq->be_ctx->domain->name) != 0) {
-        req = ipa_get_subdomain_account_info_send(breq, breq->be_ctx->ev, ctx,
-                                                  ar);
+        req = ipa_get_subdom_acct_send(breq, breq->be_ctx->ev, ctx, ar);
         if (!req) {
             return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, "Out of
memory");
         }
@@ -148,7 +147,7 @@ static void ipa_account_info_users_done(struct tevent_req *req)
     struct be_req *breq = tevent_req_callback_data(req, struct be_req);
     int ret, dp_error;
 
-    ret = ipa_user_get_recv(req, &dp_error);
+    ret = ipa_get_subdom_acct_recv(req, &dp_error);
     talloc_zfree(req);
 
     ipa_account_info_complete(breq, dp_error, ret, "User lookup failed");
diff --git a/src/providers/ipa/ipa_id.h b/src/providers/ipa/ipa_id.h
index c264286f07dd940c280c12ed00625db611caaf2c..82f2f489d0dc29235018cfee1f9488f3f25d9b74
100644
--- a/src/providers/ipa/ipa_id.h
+++ b/src/providers/ipa/ipa_id.h
@@ -60,9 +60,9 @@ struct tevent_req *ipa_s2n_get_acct_info_send(TALLOC_CTX *mem_ctx,
                                               uid_t uid);
 int ipa_s2n_get_acct_info_recv(struct tevent_req *req);
 
-struct tevent_req *ipa_get_subdomain_account_info_send(TALLOC_CTX *memctx,
-                                                      struct tevent_context *ev,
-                                                      struct sdap_id_ctx *ctx,
-                                                      struct be_acct_req *ar);
-int ipa_user_get_recv(struct tevent_req *req, int *dp_error_out);
+struct tevent_req *ipa_get_subdom_acct_send(TALLOC_CTX *memctx,
+                                            struct tevent_context *ev,
+                                            struct sdap_id_ctx *ctx,
+                                            struct be_acct_req *ar);
+int ipa_get_subdom_acct_recv(struct tevent_req *req, int *dp_error_out);
 #endif
diff --git a/src/providers/ipa/ipa_subdomains_id.c
b/src/providers/ipa/ipa_subdomains_id.c
index 563fdf2f63684e75b0f4937ee090de72c863aee9..518ff85d7b2caa78e1ff779d9cbfb332dd447b1f
100644
--- a/src/providers/ipa/ipa_subdomains_id.c
+++ b/src/providers/ipa/ipa_subdomains_id.c
@@ -32,7 +32,7 @@
 #include "providers/ipa/ipa_id.h"
 #include "providers/ipa/ipa_subdomains.h"
 
-struct ipa_user_get_state {
+struct ipa_get_subdom_acct {
     struct tevent_context *ev;
     struct sdap_id_ctx *ctx;
     struct sdap_id_op *op;
@@ -48,19 +48,20 @@ struct ipa_user_get_state {
     int dp_error;
 };
 
-static int ipa_get_subdomain_account_info_retry(struct tevent_req *req);
-static void ipa_get_subdomain_account_info_connect_done(struct tevent_req *subreq);
-static void ipa_get_subdomain_account_info_done(struct tevent_req *subreq);
-struct tevent_req *ipa_get_subdomain_account_info_send(TALLOC_CTX *memctx,
-                                                      struct tevent_context *ev,
-                                                      struct sdap_id_ctx *ctx,
-                                                      struct be_acct_req *ar)
+static void ipa_get_subdom_acct_connected(struct tevent_req *subreq);
+static void ipa_get_subdom_acct_done(struct tevent_req *subreq);
+
+struct tevent_req *ipa_get_subdom_acct_send(TALLOC_CTX *memctx,
+                                            struct tevent_context *ev,
+                                            struct sdap_id_ctx *ctx,
+                                            struct be_acct_req *ar)
 {
     struct tevent_req *req;
-    struct ipa_user_get_state *state;
+    struct ipa_get_subdom_acct *state;
+    struct tevent_req *subreq;
     int ret;
 
-    req = tevent_req_create(memctx, &state, struct ipa_user_get_state);
+    req = tevent_req_create(memctx, &state, struct ipa_get_subdom_acct);
     if (!req) return NULL;
 
     state->ev = ev;
@@ -112,10 +113,11 @@ struct tevent_req *ipa_get_subdomain_account_info_send(TALLOC_CTX
*memctx,
     }
     if (ret != EOK) goto fail;
 
-    ret = ipa_get_subdomain_account_info_retry(req);
-    if (ret != EOK) {
+    subreq = sdap_id_op_connect_send(state->op, state, &ret);
+    if (!subreq) {
         goto fail;
     }
+    tevent_req_set_callback(subreq, ipa_get_subdom_acct_connected, req);
 
     return req;
 
@@ -125,29 +127,12 @@ fail:
     return req;
 }
 
-static int ipa_get_subdomain_account_info_retry(struct tevent_req *req)
-{
-    struct ipa_user_get_state *state = tevent_req_data(req,
-                                                    struct ipa_user_get_state);
-    struct tevent_req *subreq;
-    int ret = EOK;
-
-    subreq = sdap_id_op_connect_send(state->op, state, &ret);
-    if (!subreq) {
-        return ret;
-    }
-
-    tevent_req_set_callback(subreq, ipa_get_subdomain_account_info_connect_done,
-                            req);
-    return EOK;
-}
-
-static void ipa_get_subdomain_account_info_connect_done(struct tevent_req *subreq)
+static void ipa_get_subdom_acct_connected(struct tevent_req *subreq)
 {
     struct tevent_req *req = tevent_req_callback_data(subreq,
-                                                      struct tevent_req);
-    struct ipa_user_get_state *state = tevent_req_data(req,
-                                                     struct ipa_user_get_state);
+                                                struct tevent_req);
+    struct ipa_get_subdom_acct *state = tevent_req_data(req,
+                                                struct ipa_get_subdom_acct);
     int dp_error = DP_ERR_FATAL;
     int ret;
     const char *name;
@@ -194,17 +179,17 @@ static void ipa_get_subdomain_account_info_connect_done(struct
tevent_req *subre
         tevent_req_error(req, ENOMEM);
         return;
     }
-    tevent_req_set_callback(subreq, ipa_get_subdomain_account_info_done, req);
+    tevent_req_set_callback(subreq, ipa_get_subdom_acct_done, req);
 
     return;
 }
 
-static void ipa_get_subdomain_account_info_done(struct tevent_req *subreq)
+static void ipa_get_subdom_acct_done(struct tevent_req *subreq)
 {
     struct tevent_req *req = tevent_req_callback_data(subreq,
-                                                      struct tevent_req);
-    struct ipa_user_get_state *state = tevent_req_data(req,
-                                                     struct ipa_user_get_state);
+                                                struct tevent_req);
+    struct ipa_get_subdom_acct *state = tevent_req_data(req,
+                                                struct ipa_get_subdom_acct);
     int dp_error = DP_ERR_FATAL;
     int ret;
 
@@ -214,12 +199,12 @@ static void ipa_get_subdomain_account_info_done(struct tevent_req
*subreq)
     ret = sdap_id_op_done(state->op, ret, &dp_error);
     if (dp_error == DP_ERR_OK && ret != EOK) {
         /* retry */
-        ret = ipa_get_subdomain_account_info_retry(req);
-        if (ret != EOK) {
+        subreq = sdap_id_op_connect_send(state->op, state, &ret);
+        if (!subreq) {
             tevent_req_error(req, ret);
             return;
         }
-
+        tevent_req_set_callback(subreq, ipa_get_subdom_acct_connected, req);
         return;
     }
 
@@ -235,10 +220,10 @@ static void ipa_get_subdomain_account_info_done(struct tevent_req
*subreq)
     tevent_req_done(req);
 }
 
-int ipa_user_get_recv(struct tevent_req *req, int *dp_error_out)
+int ipa_get_subdom_acct_recv(struct tevent_req *req, int *dp_error_out)
 {
-    struct ipa_user_get_state *state = tevent_req_data(req,
-                                                    struct ipa_user_get_state);
+    struct ipa_get_subdom_acct *state = tevent_req_data(req,
+                                                struct ipa_get_subdom_acct);
 
     if (dp_error_out) {
         *dp_error_out = state->dp_error;
-- 
1.7.1

[PATCH 2/5] Fix ipa_subdomain_id names and tevent_req style

[Pavel Březina]

Ack.

[PATCH 3/5] Fix tevent_req style for get_netgroup in ipa_id

[Simo Sorce]

Also do not intermix two tevent_req sequences
---
 src/providers/ipa/ipa_id.c |  151 +++++++++++++++++++++-----------------------
 1 files changed, 71 insertions(+), 80 deletions(-)

diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
index 58bb1c11608acd4a7b341c80623eb37df17dec77..953e089b7093bb3304ef8caf0c8145d67901a638
100644
--- a/src/providers/ipa/ipa_id.c
+++ b/src/providers/ipa/ipa_id.c
@@ -30,32 +30,12 @@
 #include "providers/ldap/sdap_async.h"
 #include "providers/ipa/ipa_id.h"
 
-struct ipa_netgroup_get_state {
-    struct tevent_context *ev;
-    struct ipa_id_ctx *ctx;
-    struct sdap_id_op *op;
-    struct sysdb_ctx *sysdb;
-    struct sss_domain_info *domain;
+static struct tevent_req *ipa_id_get_netgroup_send(TALLOC_CTX *memctx,
+                                                   struct tevent_context *ev,
+                                                   struct ipa_id_ctx *ipa_ctx,
+                                                   const char *name);
+static int ipa_id_get_netgroup_recv(struct tevent_req *req, int *dp_error);
 
-    const char *name;
-    int timeout;
-
-    char *filter;
-    const char **attrs;
-
-    size_t count;
-    struct sysdb_attrs **netgroups;
-
-    int dp_error;
-};
-
-struct tevent_req *ipa_netgroup_get_send(TALLOC_CTX *memctx,
-                                     struct tevent_context *ev,
-                                     struct ipa_id_ctx *ctx,
-                                     const char *name);
-static int ipa_netgroup_get_retry(struct tevent_req *req);
-static void ipa_netgroup_get_connect_done(struct tevent_req *subreq);
-static void ipa_netgroup_get_done(struct tevent_req *subreq);
 static void ipa_account_info_netgroups_done(struct tevent_req *req);
 static void ipa_account_info_users_done(struct tevent_req *req);
 
@@ -102,7 +82,7 @@ void ipa_account_info_handler(struct be_req *breq)
             break;
         }
 
-        req = ipa_netgroup_get_send(breq, breq->be_ctx->ev, ipa_ctx,
ar->filter_value);
+        req = ipa_id_get_netgroup_send(breq, breq->be_ctx->ev, ipa_ctx,
ar->filter_value);
         if (!req) {
             return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, "Out of
memory");
         }
@@ -153,23 +133,58 @@ static void ipa_account_info_users_done(struct tevent_req *req)
     ipa_account_info_complete(breq, dp_error, ret, "User lookup failed");
 }
 
+static void ipa_account_info_netgroups_done(struct tevent_req *req)
+{
+    struct be_req *breq = tevent_req_callback_data(req, struct be_req);
+    const char *error_text;
+    int ret, dp_error;
+
+    ret = ipa_id_get_netgroup_recv(req, &dp_error);
+    talloc_zfree(req);
+
+    ipa_account_info_complete(breq, dp_error, ret, "Netgroup lookup failed");
+}
+
 /* Request for netgroups
  * - first start here and then go to ipa_netgroups.c
  */
-struct tevent_req *ipa_netgroup_get_send(TALLOC_CTX *memctx,
-                                     struct tevent_context *ev,
-                                     struct ipa_id_ctx *ipa_ctx,
-                                     const char *name)
+struct ipa_id_get_netgroup_state {
+    struct tevent_context *ev;
+    struct ipa_id_ctx *ctx;
+    struct sdap_id_op *op;
+    struct sysdb_ctx *sysdb;
+    struct sss_domain_info *domain;
+
+    const char *name;
+    int timeout;
+
+    char *filter;
+    const char **attrs;
+
+    size_t count;
+    struct sysdb_attrs **netgroups;
+
+    int dp_error;
+};
+
+static void ipa_id_get_netgroup_connected(struct tevent_req *subreq);
+static void ipa_id_get_netgroup_done(struct tevent_req *subreq);
+
+static struct tevent_req *ipa_id_get_netgroup_send(TALLOC_CTX *memctx,
+                                                   struct tevent_context *ev,
+                                                   struct ipa_id_ctx *ipa_ctx,
+                                                   const char *name)
 {
     struct tevent_req *req;
-    struct ipa_netgroup_get_state *state;
+    struct ipa_id_get_netgroup_state *state;
+    struct tevent_req *subreq;
     struct sdap_id_ctx *ctx;
     char *clean_name;
     int ret;
 
     ctx = ipa_ctx->sdap_id_ctx;
 
-    req = tevent_req_create(memctx, &state, struct ipa_netgroup_get_state);
+    req = tevent_req_create(memctx, &state, struct ipa_id_get_netgroup_state);
     if (!req) return NULL;
 
     state->ev = ev;
@@ -209,10 +224,11 @@ struct tevent_req *ipa_netgroup_get_send(TALLOC_CTX *memctx,
                                &state->attrs, NULL);
     if (ret != EOK) goto fail;
 
-    ret = ipa_netgroup_get_retry(req);
-    if (ret != EOK) {
+    subreq = sdap_id_op_connect_send(state->op, state, &ret);
+    if (!subreq) {
         goto fail;
     }
+    tevent_req_set_callback(subreq, ipa_id_get_netgroup_connected, req);
 
     return req;
 
@@ -222,28 +238,12 @@ fail:
     return req;
 }
 
-static int ipa_netgroup_get_retry(struct tevent_req *req)
+static void ipa_id_get_netgroup_connected(struct tevent_req *subreq)
 {
-    struct ipa_netgroup_get_state *state = tevent_req_data(req,
-                                                    struct ipa_netgroup_get_state);
-    struct tevent_req *subreq;
-    int ret = EOK;
-
-    subreq = sdap_id_op_connect_send(state->op, state, &ret);
-    if (!subreq) {
-        return ret;
-    }
-
-    tevent_req_set_callback(subreq, ipa_netgroup_get_connect_done, req);
-    return EOK;
-}
-
-static void ipa_netgroup_get_connect_done(struct tevent_req *subreq)
-{
-    struct tevent_req *req = tevent_req_callback_data(subreq,
-                                                      struct tevent_req);
-    struct ipa_netgroup_get_state *state = tevent_req_data(req,
-                                                    struct ipa_netgroup_get_state);
+    struct tevent_req *req =
+                    tevent_req_callback_data(subreq, struct tevent_req);
+    struct ipa_id_get_netgroup_state *state =
+                    tevent_req_data(req, struct ipa_id_get_netgroup_state);
     int dp_error = DP_ERR_FATAL;
     int ret;
     struct sdap_id_ctx *sdap_ctx = state->ctx->sdap_id_ctx;
@@ -267,32 +267,33 @@ static void ipa_netgroup_get_connect_done(struct tevent_req
*subreq)
         tevent_req_error(req, ENOMEM);
         return;
     }
-    tevent_req_set_callback(subreq, ipa_netgroup_get_done, req);
+    tevent_req_set_callback(subreq, ipa_id_get_netgroup_done, req);
 
     return;
 }
 
-static void ipa_netgroup_get_done(struct tevent_req *subreq)
+static void ipa_id_get_netgroup_done(struct tevent_req *subreq)
 {
-    struct tevent_req *req = tevent_req_callback_data(subreq,
-                                                      struct tevent_req);
-    struct ipa_netgroup_get_state *state = tevent_req_data(req,
-                                                    struct ipa_netgroup_get_state);
+    struct tevent_req *req =
+                    tevent_req_callback_data(subreq, struct tevent_req);
+    struct ipa_id_get_netgroup_state *state =
+                    tevent_req_data(req, struct ipa_id_get_netgroup_state);
     int dp_error = DP_ERR_FATAL;
     int ret;
 
-    ret = ipa_get_netgroups_recv(subreq, state, &state->count,
&state->netgroups);
+    ret = ipa_get_netgroups_recv(subreq, state,
+                                 &state->count, &state->netgroups);
     talloc_zfree(subreq);
     ret = sdap_id_op_done(state->op, ret, &dp_error);
 
     if (dp_error == DP_ERR_OK && ret != EOK) {
         /* retry */
-        ret = ipa_netgroup_get_retry(req);
-        if (ret != EOK) {
+        subreq = sdap_id_op_connect_send(state->op, state, &ret);
+        if (!subreq) {
             tevent_req_error(req, ret);
             return;
         }
-
+        tevent_req_set_callback(subreq, ipa_id_get_netgroup_connected, req);
         return;
     }
 
@@ -322,13 +323,13 @@ static void ipa_netgroup_get_done(struct tevent_req *subreq)
     return;
 }
 
-int ipa_netgroup_get_recv(struct tevent_req *req, int *dp_error_out)
+static int ipa_id_get_netgroup_recv(struct tevent_req *req, int *dp_error)
 {
-    struct ipa_netgroup_get_state *state = tevent_req_data(req,
-                                                    struct ipa_netgroup_get_state);
+    struct ipa_id_get_netgroup_state *state =
+                    tevent_req_data(req, struct ipa_id_get_netgroup_state);
 
-    if (dp_error_out) {
-        *dp_error_out = state->dp_error;
+    if (dp_error) {
+        *dp_error = state->dp_error;
     }
 
     TEVENT_REQ_RETURN_ON_ERROR(req);
@@ -336,16 +337,6 @@ int ipa_netgroup_get_recv(struct tevent_req *req, int *dp_error_out)
     return EOK;
 }
 
-static void ipa_account_info_netgroups_done(struct tevent_req *req)
-{
-    struct be_req *breq = tevent_req_callback_data(req, struct be_req);
-    int ret, dp_error;
-
-    ret = ipa_netgroup_get_recv(req, &dp_error);
-    talloc_zfree(req);
-
-    ipa_account_info_complete(breq, dp_error, ret, "Netgroup lookup failed");
-}
 
 void ipa_check_online(struct be_req *be_req)
 {
-- 
1.7.1

[PATCH 3/5] Fix tevent_req style for get_netgroup in ipa_id

[Pavel Březina]

Ack.

[PATCH 4/5] Streamline ipa_account_info handler

[Simo Sorce]

In particular note that we merge ipa_account_info_netgroups_done()
and ipa_account_info_users_done() into a single fucntion called
ipa_account_info_done() that handles both cases

We also remove the auxiliary function ipa_account_info_complete() that
unnecessarily violates the tevent_req style and instead use a new function
named ipa_account_info_error_text() to generate error text.
---
 src/providers/ipa/ipa_id.c |  128 ++++++++++++++++++-------------------------
 1 files changed, 54 insertions(+), 74 deletions(-)

diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
index 953e089b7093bb3304ef8caf0c8145d67901a638..8e4309f865daec3a574ffdb486c2dda7225cc120
100644
--- a/src/providers/ipa/ipa_id.c
+++ b/src/providers/ipa/ipa_id.c
@@ -30,23 +30,45 @@
 #include "providers/ldap/sdap_async.h"
 #include "providers/ipa/ipa_id.h"
 
+static const char *ipa_account_info_error_text(int ret, int *dp_error,
+                                               const char *default_text)
+{
+    switch (*dp_error) {
+    case DP_ERR_OK:
+        if (ret == EOK) {
+            return NULL;
+        }
+        DEBUG(SSSDBG_CRIT_FAILURE, ("Bug: dp_error is OK on failed request"));
+        *dp_error = DP_ERR_FATAL;
+        break;
+    case DP_ERR_OFFLINE:
+        return "Offline";
+    case DP_ERR_FATAL:
+        if (ret == ENOMEM) {
+            return "Out of memory";
+        }
+        break;
+    default:
+        break;
+    }
+
+    return default_text;
+}
+
 static struct tevent_req *ipa_id_get_netgroup_send(TALLOC_CTX *memctx,
                                                    struct tevent_context *ev,
                                                    struct ipa_id_ctx *ipa_ctx,
                                                    const char *name);
 static int ipa_id_get_netgroup_recv(struct tevent_req *req, int *dp_error);
 
-static void ipa_account_info_netgroups_done(struct tevent_req *req);
-static void ipa_account_info_users_done(struct tevent_req *req);
+static void ipa_account_info_done(struct tevent_req *req);
 
 void ipa_account_info_handler(struct be_req *breq)
 {
     struct ipa_id_ctx *ipa_ctx;
     struct sdap_id_ctx *ctx;
     struct be_acct_req *ar;
-    struct tevent_req *req;
-    const char *err = "Unknown Error";
-    int ret = EOK;
+    struct tevent_req *req = NULL;
 
     ipa_ctx = talloc_get_type(breq->be_ctx->bet_info[BET_ID].pvt_bet_data, struct
ipa_id_ctx);
     ctx = ipa_ctx->sdap_id_ctx;
@@ -58,93 +80,51 @@ void ipa_account_info_handler(struct be_req *breq)
     ar = talloc_get_type(breq->req_data, struct be_acct_req);
 
     if (strcasecmp(ar->domain, breq->be_ctx->domain->name) != 0) {
+        /* if domain names do not match, this is a subdomain case */
         req = ipa_get_subdom_acct_send(breq, breq->be_ctx->ev, ctx, ar);
-        if (!req) {
-            return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, "Out of
memory");
-        }
 
-        tevent_req_set_callback(req, ipa_account_info_users_done, breq);
-
-        return;
-    }
-
-    switch (ar->entry_type & 0xFFF) {
-    case BE_REQ_USER: /* user */
-    case BE_REQ_GROUP: /* group */
-    case BE_REQ_INITGROUPS: /* init groups for user */
-    case BE_REQ_SERVICES: /* Services. Not natively supported by IPA */
-        return sdap_handle_account_info(breq, ctx);
-
-    case BE_REQ_NETGROUP:
+    } else if ((ar->entry_type & 0xFFF) == BE_REQ_NETGROUP) {
+        /* netgroups are handled by a separate request function */
         if (ar->filter_type != BE_FILTER_NAME) {
-            ret = EINVAL;
-            err = "Invalid filter type";
-            break;
+            return sdap_handler_done(breq, DP_ERR_FATAL,
+                                     EINVAL, "Invalid filter type");
         }
-
-        req = ipa_id_get_netgroup_send(breq, breq->be_ctx->ev, ipa_ctx,
ar->filter_value);
-        if (!req) {
-            return sdap_handler_done(breq, DP_ERR_FATAL, ENOMEM, "Out of
memory");
-        }
-
-        tevent_req_set_callback(req, ipa_account_info_netgroups_done, breq);
-        break;
-
-    default: /*fail*/
-        ret = EINVAL;
-        err = "Invalid request type";
-    }
-
-    if (ret != EOK) return sdap_handler_done(breq, DP_ERR_FATAL, ret, err);
-}
-
-static void ipa_account_info_complete(struct be_req *breq, int dp_error,
-                                       int ret, const char *default_error_text)
-{
-    const char* error_text;
-
-    if (dp_error == DP_ERR_OK) {
-        if (ret == EOK) {
-            error_text = NULL;
-        } else {
-            DEBUG(1, ("Bug: dp_error is OK on failed request"));
-            dp_error = DP_ERR_FATAL;
-            error_text = default_error_text;
-        }
-    } else if (dp_error == DP_ERR_OFFLINE) {
-        error_text = "Offline";
-    } else if (dp_error == DP_ERR_FATAL && ret == ENOMEM) {
-        error_text = "Out of memory";
+        req = ipa_id_get_netgroup_send(breq, breq->be_ctx->ev,
+                                       ipa_ctx, ar->filter_value);
     } else {
-        error_text = default_error_text;
+        /* any account request is handled by sdap,
+         * any invalid request is caught there. */
+        return sdap_handle_account_info(breq, ctx);
     }
 
-    sdap_handler_done(breq, dp_error, ret, error_text);
-}
-
-static void ipa_account_info_users_done(struct tevent_req *req)
-{
-    struct be_req *breq = tevent_req_callback_data(req, struct be_req);
-    int ret, dp_error;
-
-    ret = ipa_get_subdom_acct_recv(req, &dp_error);
-    talloc_zfree(req);
-
-    ipa_account_info_complete(breq, dp_error, ret, "User lookup failed");
+    if (!req) {
+        return sdap_handler_done(breq, DP_ERR_FATAL,
+                                 ENOMEM, "Out of memory");
+    }
+    tevent_req_set_callback(req, ipa_account_info_done, breq);
 }
 
-static void ipa_account_info_netgroups_done(struct tevent_req *req)
+static void ipa_account_info_done(struct tevent_req *req)
 {
     struct be_req *breq = tevent_req_callback_data(req, struct be_req);
+    struct be_acct_req *ar = talloc_get_type(breq->req_data,
+                                             struct be_acct_req);
     const char *error_text;
     int ret, dp_error;
 
-    ret = ipa_id_get_netgroup_recv(req, &dp_error);
+    if ((ar->entry_type & 0xFFF) == BE_REQ_NETGROUP) {
+        ret = ipa_id_get_netgroup_recv(req, &dp_error);
+    } else {
+        ret = ipa_get_subdom_acct_recv(req, &dp_error);
+    }
     talloc_zfree(req);
 
-    ipa_account_info_complete(breq, dp_error, ret, "Netgroup lookup failed");
+    error_text = ipa_account_info_error_text(ret, &dp_error,
+                                             "Account info lookup failed");
+    sdap_handler_done(breq, dp_error, ret, error_text);
 }
 
+
 /* Request for netgroups
  * - first start here and then go to ipa_netgroups.c
  */
-- 
1.7.1

[PATCH 4/5] Streamline ipa_account_info handler

[Pavel Březina]

On 11/28/2012 05:24 AM, Simo Sorce wrote:
>  In particular note that we merge ipa_account_info_netgroups_done()
>  and ipa_account_info_users_done() into a single fucntion called
>  ipa_account_info_done() that handles both cases
>
>  We also remove the auxiliary function ipa_account_info_complete() that
>  unnecessarily violates the tevent_req style and instead use a new function
>  named ipa_account_info_error_text() to generate error text.
>  ---
>    src/providers/ipa/ipa_id.c |  128 ++++++++++++++++++-------------------------
>    1 files changed, 54 insertions(+), 74 deletions(-)
>
>  diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
>  index 953e089b7093bb3304ef8caf0c8145d67901a638..8e4309f865daec3a574ffdb486c2dda7225cc120
> 100644
>  --- a/src/providers/ipa/ipa_id.c
>  +++ b/src/providers/ipa/ipa_id.c
>  @@ -30,23 +30,45 @@
>    #include "providers/ldap/sdap_async.h"
>    #include "providers/ipa/ipa_id.h"
>
>  +static const char *ipa_account_info_error_text(int ret, int *dp_error,
>  +                                               const char *default_text)
>  +{
>  +    switch (*dp_error) {
>  +    case DP_ERR_OK:
>  +        if (ret == EOK) {
>  +            return NULL;
>  +        }
>  +        DEBUG(SSSDBG_CRIT_FAILURE, ("Bug: dp_error is OK on failed request"));
Missing \n.

>  +        *dp_error = DP_ERR_FATAL;
>  +        break;
>  +    case DP_ERR_OFFLINE:
>  +        return "Offline";
>  +    case DP_ERR_FATAL:
>  +        if (ret == ENOMEM) {
>  +            return "Out of memory";
>  +        }
>  +        break;
>  +    default:
>  +        break;
>  +    }
>  +
>  +    return default_text;
>  +} 
Otherwise it's good. I like ipa_account_info_handler() as well :)

[PATCH 4/5] Streamline ipa_account_info handler

[Simo Sorce]

On Thu, 2012-11-29 at 11:48 +0100, Pavel Březina wrote:
>  On 11/28/2012 05:24 AM, Simo Sorce wrote:
>  > In particular note that we merge ipa_account_info_netgroups_done()
>  > and ipa_account_info_users_done() into a single fucntion called
>  > ipa_account_info_done() that handles both cases
>  >
>  > We also remove the auxiliary function ipa_account_info_complete() that
>  > unnecessarily violates the tevent_req style and instead use a new function
>  > named ipa_account_info_error_text() to generate error text.
>  > ---
>  >   src/providers/ipa/ipa_id.c |  128 ++++++++++++++++++-------------------------
>  >   1 files changed, 54 insertions(+), 74 deletions(-)
>  >
>  > diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
>  > index
> 953e089b7093bb3304ef8caf0c8145d67901a638..8e4309f865daec3a574ffdb486c2dda7225cc120 100644
>  > --- a/src/providers/ipa/ipa_id.c
>  > +++ b/src/providers/ipa/ipa_id.c
>  > @@ -30,23 +30,45 @@
>  >   #include "providers/ldap/sdap_async.h"
>  >   #include "providers/ipa/ipa_id.h"
>  >
>  > +static const char *ipa_account_info_error_text(int ret, int *dp_error,
>  > +                                               const char *default_text)
>  > +{
>  > +    switch (*dp_error) {
>  > +    case DP_ERR_OK:
>  > +        if (ret == EOK) {
>  > +            return NULL;
>  > +        }
>  > +        DEBUG(SSSDBG_CRIT_FAILURE, ("Bug: dp_error is OK on failed
> request"));
>  
>  Missing \n.
>  
>  > +        *dp_error = DP_ERR_FATAL;
>  > +        break;
>  > +    case DP_ERR_OFFLINE:
>  > +        return "Offline";
>  > +    case DP_ERR_FATAL:
>  > +        if (ret == ENOMEM) {
>  > +            return "Out of memory";
>  > +        }
>  > +        break;
>  > +    default:
>  > +        break;
>  > +    }
>  > +
>  > +    return default_text;
>  > +}
>  
>  Otherwise it's good. I like ipa_account_info_handler() as well :) 
Attached just this patch with the requested fix.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[PATCH 4/5] Streamline ipa_account_info handler

[Pavel Březina]

On 12/02/2012 06:05 AM, Simo Sorce wrote:
>  On Thu, 2012-11-29 at 11:48 +0100, Pavel Březina wrote:
> > On 11/28/2012 05:24 AM, Simo Sorce wrote:
> >> In particular note that we merge ipa_account_info_netgroups_done()
> >> and ipa_account_info_users_done() into a single fucntion called
> >> ipa_account_info_done() that handles both cases
> >>
> >> We also remove the auxiliary function ipa_account_info_complete() that
> >> unnecessarily violates the tevent_req style and instead use a new function
> >> named ipa_account_info_error_text() to generate error text.
> >> ---
> >>    src/providers/ipa/ipa_id.c |  128 ++++++++++++++++++-------------------------
> >>    1 files changed, 54 insertions(+), 74 deletions(-)
> >>
> >> diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
> >> index
> 953e089b7093bb3304ef8caf0c8145d67901a638..8e4309f865daec3a574ffdb486c2dda7225cc120 100644
> >> --- a/src/providers/ipa/ipa_id.c
> >> +++ b/src/providers/ipa/ipa_id.c
> >> @@ -30,23 +30,45 @@
> >>    #include "providers/ldap/sdap_async.h"
> >>    #include "providers/ipa/ipa_id.h"
> >>
> >> +static const char *ipa_account_info_error_text(int ret, int *dp_error,
> >> +                                               const char *default_text)
> >> +{
> >> +    switch (*dp_error) {
> >> +    case DP_ERR_OK:
> >> +        if (ret == EOK) {
> >> +            return NULL;
> >> +        }
> >> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Bug: dp_error is OK on failed
> request"));
> >
> > Missing \n.
> >
> >> +        *dp_error = DP_ERR_FATAL;
> >> +        break;
> >> +    case DP_ERR_OFFLINE:
> >> +        return "Offline";
> >> +    case DP_ERR_FATAL:
> >> +        if (ret == ENOMEM) {
> >> +            return "Out of memory";
> >> +        }
> >> +        break;
> >> +    default:
> >> +        break;
> >> +    }
> >> +
> >> +    return default_text;
> >> +}
> >
> > Otherwise it's good. I like ipa_account_info_handler() as well :)
>
>  Attached just this patch with the requested fix.
>
>  Simo. 
I guess you forgot to commit the fix? :-)

[PATCH 4/5] Streamline ipa_account_info handler

[Simo Sorce]

On Mon, 2012-12-03 at 13:42 +0100, Pavel Březina wrote:
>  On 12/02/2012 06:05 AM, Simo Sorce wrote:
>  > On Thu, 2012-11-29 at 11:48 +0100, Pavel Březina wrote:
>  >> On 11/28/2012 05:24 AM, Simo Sorce wrote:
>  >>> In particular note that we merge ipa_account_info_netgroups_done()
>  >>> and ipa_account_info_users_done() into a single fucntion called
>  >>> ipa_account_info_done() that handles both cases
>  >>>
>  >>> We also remove the auxiliary function ipa_account_info_complete() that
>  >>> unnecessarily violates the tevent_req style and instead use a new function
>  >>> named ipa_account_info_error_text() to generate error text.
>  >>> ---
>  >>>    src/providers/ipa/ipa_id.c |  128
> ++++++++++++++++++-------------------------
>  >>>    1 files changed, 54 insertions(+), 74 deletions(-)
>  >>>
>  >>> diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
>  >>> index
> 953e089b7093bb3304ef8caf0c8145d67901a638..8e4309f865daec3a574ffdb486c2dda7225cc120 100644
>  >>> --- a/src/providers/ipa/ipa_id.c
>  >>> +++ b/src/providers/ipa/ipa_id.c
>  >>> @@ -30,23 +30,45 @@
>  >>>    #include "providers/ldap/sdap_async.h"
>  >>>    #include "providers/ipa/ipa_id.h"
>  >>>
>  >>> +static const char *ipa_account_info_error_text(int ret, int *dp_error,
>  >>> +                                               const char *default_text)
>  >>> +{
>  >>> +    switch (*dp_error) {
>  >>> +    case DP_ERR_OK:
>  >>> +        if (ret == EOK) {
>  >>> +            return NULL;
>  >>> +        }
>  >>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Bug: dp_error is OK on failed
> request"));
>  >>
>  >> Missing \n.
>  >>
>  >>> +        *dp_error = DP_ERR_FATAL;
>  >>> +        break;
>  >>> +    case DP_ERR_OFFLINE:
>  >>> +        return "Offline";
>  >>> +    case DP_ERR_FATAL:
>  >>> +        if (ret == ENOMEM) {
>  >>> +            return "Out of memory";
>  >>> +        }
>  >>> +        break;
>  >>> +    default:
>  >>> +        break;
>  >>> +    }
>  >>> +
>  >>> +    return default_text;
>  >>> +}
>  >>
>  >> Otherwise it's good. I like ipa_account_info_handler() as well :)
>  >
>  > Attached just this patch with the requested fix.
>  >
>  > Simo.
>  
>  I guess you forgot to commit the fix? :-) 
Sigh :-/
No I had an older git log and c&p the wrong commit id :-(

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[PATCH 4/5] Streamline ipa_account_info handler

[Pavel Březina]

On 12/03/2012 08:52 PM, Simo Sorce wrote:
>  On Mon, 2012-12-03 at 13:42 +0100, Pavel Březina wrote:
> > On 12/02/2012 06:05 AM, Simo Sorce wrote:
> >> On Thu, 2012-11-29 at 11:48 +0100, Pavel Březina wrote:
> >>> On 11/28/2012 05:24 AM, Simo Sorce wrote:
> >>>> In particular note that we merge ipa_account_info_netgroups_done()
> >>>> and ipa_account_info_users_done() into a single fucntion called
> >>>> ipa_account_info_done() that handles both cases
> >>>>
> >>>> We also remove the auxiliary function ipa_account_info_complete() that
> >>>> unnecessarily violates the tevent_req style and instead use a new
> function
> >>>> named ipa_account_info_error_text() to generate error text.
> >>>> ---
> >>>>     src/providers/ipa/ipa_id.c |  128
> ++++++++++++++++++-------------------------
> >>>>     1 files changed, 54 insertions(+), 74 deletions(-)
> >>>>
> >>>> diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
> >>>> index
> 953e089b7093bb3304ef8caf0c8145d67901a638..8e4309f865daec3a574ffdb486c2dda7225cc120 100644
> >>>> --- a/src/providers/ipa/ipa_id.c
> >>>> +++ b/src/providers/ipa/ipa_id.c
> >>>> @@ -30,23 +30,45 @@
> >>>>     #include "providers/ldap/sdap_async.h"
> >>>>     #include "providers/ipa/ipa_id.h"
> >>>>
> >>>> +static const char *ipa_account_info_error_text(int ret, int *dp_error,
> >>>> +                                               const char
> *default_text)
> >>>> +{
> >>>> +    switch (*dp_error) {
> >>>> +    case DP_ERR_OK:
> >>>> +        if (ret == EOK) {
> >>>> +            return NULL;
> >>>> +        }
> >>>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Bug: dp_error is OK on failed
> request"));
> >>>
> >>> Missing \n.
> >>>
> >>>> +        *dp_error = DP_ERR_FATAL;
> >>>> +        break;
> >>>> +    case DP_ERR_OFFLINE:
> >>>> +        return "Offline";
> >>>> +    case DP_ERR_FATAL:
> >>>> +        if (ret == ENOMEM) {
> >>>> +            return "Out of memory";
> >>>> +        }
> >>>> +        break;
> >>>> +    default:
> >>>> +        break;
> >>>> +    }
> >>>> +
> >>>> +    return default_text;
> >>>> +}
> >>>
> >>> Otherwise it's good. I like ipa_account_info_handler() as well :)
> >>
> >> Attached just this patch with the requested fix.
> >>
> >> Simo.
> >
> > I guess you forgot to commit the fix? :-)
>
>  Sigh :-/
>  No I had an older git log and c&p the wrong commit id :-(
>
>  Simo. 
Ack.

[PATCH 4/5] Streamline ipa_account_info handler

[Jakub Hrozek]

On Mon, Dec 03, 2012 at 02:52:46PM -0500, Simo Sorce wrote:
>  On Mon, 2012-12-03 at 13:42 +0100, Pavel Březina wrote:
>  > On 12/02/2012 06:05 AM, Simo Sorce wrote:
>  > > On Thu, 2012-11-29 at 11:48 +0100, Pavel Březina wrote:
>  > >> On 11/28/2012 05:24 AM, Simo Sorce wrote:
>  > >>> In particular note that we merge ipa_account_info_netgroups_done()
>  > >>> and ipa_account_info_users_done() into a single fucntion called
>  > >>> ipa_account_info_done() that handles both cases
>  > >>>
>  > >>> We also remove the auxiliary function ipa_account_info_complete() that
>  > >>> unnecessarily violates the tevent_req style and instead use a new
> function
>  > >>> named ipa_account_info_error_text() to generate error text.
>  > >>> ---
>  > >>>    src/providers/ipa/ipa_id.c |  128
> ++++++++++++++++++-------------------------
>  > >>>    1 files changed, 54 insertions(+), 74 deletions(-)
>  > >>>
>  > >>> diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
>  > >>> index
> 953e089b7093bb3304ef8caf0c8145d67901a638..8e4309f865daec3a574ffdb486c2dda7225cc120 100644
>  > >>> --- a/src/providers/ipa/ipa_id.c
>  > >>> +++ b/src/providers/ipa/ipa_id.c
>  > >>> @@ -30,23 +30,45 @@
>  > >>>    #include "providers/ldap/sdap_async.h"
>  > >>>    #include "providers/ipa/ipa_id.h"
>  > >>>
>  > >>> +static const char *ipa_account_info_error_text(int ret, int
> *dp_error,
>  > >>> +                                               const char
> *default_text)
>  > >>> +{
>  > >>> +    switch (*dp_error) {
>  > >>> +    case DP_ERR_OK:
>  > >>> +        if (ret == EOK) {
>  > >>> +            return NULL;
>  > >>> +        }
>  > >>> +        DEBUG(SSSDBG_CRIT_FAILURE, ("Bug: dp_error is OK on
> failed request"));
>  > >>
>  > >> Missing \n.
>  > >>
>  > >>> +        *dp_error = DP_ERR_FATAL;
>  > >>> +        break;
>  > >>> +    case DP_ERR_OFFLINE:
>  > >>> +        return "Offline";
>  > >>> +    case DP_ERR_FATAL:
>  > >>> +        if (ret == ENOMEM) {
>  > >>> +            return "Out of memory";
>  > >>> +        }
>  > >>> +        break;
>  > >>> +    default:
>  > >>> +        break;
>  > >>> +    }
>  > >>> +
>  > >>> +    return default_text;
>  > >>> +}
>  > >>
>  > >> Otherwise it's good. I like ipa_account_info_handler() as well :)
>  > >
>  > > Attached just this patch with the requested fix.
>  > >
>  > > Simo.
>  > 
>  > I guess you forgot to commit the fix? :-)
>   
For some reason this reply broke patchwork and this whole ^^^ reply got
included in the commit message of this patch in patchwork.

I fixed the commit message before pushing.

>  Sigh :-/
>  No I had an older git log and c&p the wrong commit id :-(
>  
>  Simo.

[PATCH 5/5] Use an entry type mask macro to filter entry types

[Simo Sorce]

Avoids hardcoding magic numbers everywhere and self documents why a
mask is being applied.
---
 src/providers/data_provider.h         |    1 +
 src/providers/ipa/ipa_id.c            |    4 ++--
 src/providers/ipa/ipa_subdomains_id.c |    2 +-
 src/providers/ldap/ldap_id.c          |    2 +-
 src/providers/proxy/proxy_id.c        |    2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h
index d49fcd524ef37e4755263399903d2804aaf29737..bb944509da9f1dc89216266cf62c57fb4127fd57
100644
--- a/src/providers/data_provider.h
+++ b/src/providers/data_provider.h
@@ -142,6 +142,7 @@
 #define BE_REQ_SUDO_RULES    0x0007
 #define BE_REQ_AUTOFS        0x0009
 #define BE_REQ_HOST          0x0010
+#define BE_REQ_TYPE_MASK     0x00FF
 #define BE_REQ_FAST          0x1000
 
 /* AUTH related common data and functions */
diff --git a/src/providers/ipa/ipa_id.c b/src/providers/ipa/ipa_id.c
index 8e4309f865daec3a574ffdb486c2dda7225cc120..5cdd780f0b2fe899c0079f1219f455e22537aeca
100644
--- a/src/providers/ipa/ipa_id.c
+++ b/src/providers/ipa/ipa_id.c
@@ -83,7 +83,7 @@ void ipa_account_info_handler(struct be_req *breq)
         /* if domain names do not match, this is a subdomain case */
         req = ipa_get_subdom_acct_send(breq, breq->be_ctx->ev, ctx, ar);
 
-    } else if ((ar->entry_type & 0xFFF) == BE_REQ_NETGROUP) {
+    } else if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_NETGROUP) {
         /* netgroups are handled by a separate request function */
         if (ar->filter_type != BE_FILTER_NAME) {
             return sdap_handler_done(breq, DP_ERR_FATAL,
@@ -112,7 +112,7 @@ static void ipa_account_info_done(struct tevent_req *req)
     const char *error_text;
     int ret, dp_error;
 
-    if ((ar->entry_type & 0xFFF) == BE_REQ_NETGROUP) {
+    if ((ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_NETGROUP) {
         ret = ipa_id_get_netgroup_recv(req, &dp_error);
     } else {
         ret = ipa_get_subdom_acct_recv(req, &dp_error);
diff --git a/src/providers/ipa/ipa_subdomains_id.c
b/src/providers/ipa/ipa_subdomains_id.c
index 518ff85d7b2caa78e1ff779d9cbfb332dd447b1f..481b998065eec29a52b9c39fba4017ff10098481
100644
--- a/src/providers/ipa/ipa_subdomains_id.c
+++ b/src/providers/ipa/ipa_subdomains_id.c
@@ -85,7 +85,7 @@ struct tevent_req *ipa_get_subdom_acct_send(TALLOC_CTX *memctx,
     }
     state->sysdb = state->domain->sysdb;
 
-    state->entry_type = (ar->entry_type & 0xFFF);
+    state->entry_type = (ar->entry_type & BE_REQ_TYPE_MASK);
     state->filter = ar->filter_value;
     state->filter_type = ar->filter_type;
 
diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
index 13a8212bc2cb0055a0e200527e4c7da7d66c4c45..a258e827b4788c0bea65021e286d6628e952c4ea
100644
--- a/src/providers/ldap/ldap_id.c
+++ b/src/providers/ldap/ldap_id.c
@@ -929,7 +929,7 @@ void sdap_handle_account_info(struct be_req *breq, struct sdap_id_ctx
*ctx)
 
     ar = talloc_get_type(breq->req_data, struct be_acct_req);
 
-    switch (ar->entry_type & 0xFFF) {
+    switch (ar->entry_type & BE_REQ_TYPE_MASK) {
     case BE_REQ_USER: /* user */
 
         /* skip enumerations on demand */
diff --git a/src/providers/proxy/proxy_id.c b/src/providers/proxy/proxy_id.c
index ce66fa128e29eea880f28733570a7420bd6d445e..87eb91b1ee47fce61757aa58efc13d84ecc2acd2
100644
--- a/src/providers/proxy/proxy_id.c
+++ b/src/providers/proxy/proxy_id.c
@@ -1347,7 +1347,7 @@ void proxy_get_account_info(struct be_req *breq)
         return proxy_reply(breq, DP_ERR_FATAL, EINVAL, "Invalid attr type");
     }
 
-    switch (ar->entry_type & 0xFFF) {
+    switch (ar->entry_type & BE_REQ_TYPE_MASK) {
     case BE_REQ_USER: /* user */
         switch (ar->filter_type) {
         case BE_FILTER_ENUM:
-- 
1.7.1

[PATCH 5/5] Use an entry type mask macro to filter entry types

[Pavel Březina]

Ack, but I'd like to consider using macro for:

(ar->entry_type & BE_REQ_TYPE_MASK)

Something like:

BE_GET_REQ_TYPE(request) (request & BE_REQ_TYPE_MASK)

[PATCH 5/5] Use an entry type mask macro to filter entry types

[Simo Sorce]

On Thu, 2012-11-29 at 11:48 +0100, Pavel Březina wrote:
>  Ack, but I'd like to consider using macro for:
>  
>  (ar->entry_type & BE_REQ_TYPE_MASK)
>  
>  Something like:
>  
>  BE_GET_REQ_TYPE(request) (request & BE_REQ_TYPE_MASK) 
Good idea, I'll add that.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

On Tue, Nov 27, 2012 at 11:24:53PM -0500, Simo Sorce wrote:
>  This revision should address all concerns raised by Pavel for code
> that I
>  actually changed from the original. For patch 1 a calrification I can't
>  make was asked. I 'fixed' the code as I realized I copied a debug statement
>  wrapped in an if statement that wasn't in the original code I move around,
>  so I simply dropped it (the statement is still present but later on in another
>  part of the code, where it remains from the original code).
>  
>  For patch for I completely changed approach and simplified the code even more by
>  removing the switch statement completely, Thanks Pavel, that code looks even
>  better to me now :)
>  
>  Simo Sorce (5):
>    Fix tevent_req style for krb5_auth
>    Fix ipa_subdomain_id names and tevent_req style
>    Fix tevent_req style for get_netgroup in ipa_id
>    Streamline ipa_account_info handler
>    Use an entry type mask macro to filter entry types 
The latest revisions of all five patches has been pushed to master.

Nice work!