[SSSD] [PATCH] DP: Refactor responder_dp_req so it's reusable by other responders

Jakub Hrozek jhrozek at redhat.com
Mon Jan 23 17:31:23 UTC 2012


On Mon, Jan 23, 2012 at 04:54:10PM +0100, Pavel Březina wrote:
> Dne 23.1.2012 11:29, Jakub Hrozek napsal(a):
> >On Sun, Jan 22, 2012 at 08:14:21PM -0500, Stephen Gallagher wrote:
> >>On Sun, 2012-01-22 at 22:08 +0100, Jakub Hrozek wrote:
> >>>A number of new responders is currently being developed in addition to
> >>>the existing NSS and PAM responders that only deal with accounts. The
> >>>new responders often deal with different data such as automounter maps
> >>>or sudoer rules. It is prudent to let these responders communicate with
> >>>back ends using their own protocol.
> >>>
> >>>Attached is a patch that refactors the requests in Data Provider so that
> >>>they are more generic. The commits message is slightly more verbose than
> >>>usual this time to explain all the changes.
> >>>
> >>>I'm going to include a sudo request shortly as an example of a new
> >>>non-account request.
> >>>
> >>>[PATCH] DP: Refactor responder_dp_req so it's reusable by other responders:
> >>>* the internal requst is now more generic and is decoupled from
> >>>   account-specific data. There is a new sss_dp_issue_request() wrapper
> >>>   that issues a BE request or registers a callback
> >>>* the public requests all use struct sss_dp_req_state as the tevent_req
> >>>   state data. This allows to report back data from the internal request
> >>>   even if the caller is just a callback notifier
> >>>* each specific request now uses an _info structure that contains all
> >>>   the data necessary to construct a DBusMessage passed to provider
> >>>* each specific request now defines a sss_dp_get_$data_msg callback that
> >>>   is called from the sss_dp_issue_request() common wraper. The purpose
> >>>   of the wrapper is to construct a DBusMessage and bind it to a DBus
> >>>   method so the message can be just sent over to back end
> >>>
> >>>The miscellanous changes include:
> >>>* change SSS_DP_ constants to an enum. This way, a switch() would error
> >>>   if a value is not handled.
> >>>* rename sss_dp_get_account_int_send() to sss_dp_internal_get_send()
> >>>   request because the internal request is going to handle more than just
> >>>   account data
> >>>
> >>>The patch must be applied on top of all Stephen's patches that are
> >>>currently on the list, including the services support.
> >>
> >>Nack.
> >>
> >>Just to remain consistent with the old approach, please start the enum
> >>explicitly at 1.
> >>
> >
> >Done.
> 
> Nack.
> 
> If you want the compiler to check that all enum are handled in
> switch() you have to provide a properly typed variable.
> 
> info->type has to be of type 'enum sss_dp_type'
> 
> And we have to split this enum to two - one for accounts and one for sudo.
> 

That's a great point.

New patch is attached.

> >
> >>In sss_dp_get_account_send(), you set 'ret = ENOMEM' if the key isn't
> >>allocated, but you don't 'goto error'. (On further review, it looks like
> >>that bug existed previously. Please fix it anyway).
> >>
> >
> >Done.
> >
> >>There's no purpose to the existence of 'tvcb' in sss_dp_issue_request().
> >>There should never be a case where we'd do other than just free the
> >>sidereq in the _done function. Therefore, I think it's better to just
> >>make that internal, not public.
> >
> >I was trying to retain the _send/_done/_recv structure. The attached
> >patch removes the _done function in favor of a hardcoded one that just
> >frees the sidereq.
> >
> >>Also, please put back the comment about
> >>how we're freeing it because the callbacks have already been invoked.
> >>
> >
> >Done.
> >
> >>If you're switching to using an enum for sss_dp_type, please remove the
> >>default: section from the switch statement in sss_dp_get_account_msg().
> >>I'd prefer that to be a compile-time warning (since most of us built
> >>with -Werror, that would make it easy to catch).
> >
> >Yes, that was the reason I switched it to enum but forgot to actually
> >remove the default :-)
> >
> >>
> >>In sss_dp_issue_request(), when you get HASH_SUCCESS, you're just
> >>returning EOK immediately. You need to add this caller to the list of
> >>callbacks. Presumably you intended to break here instead, like in the
> >>original code.
> >>
> >
> >Thanks, fixed
> >
> >>There's a flaw in your hash key algorithm. It's theoretically possible
> >>for two different wrappers to call sss_dp_issue_request() with the same
> >>key, but a different msg_create argument. This could result in the hash
> >>comparison falsely believing that an existing request of the same type
> >>is in progress, whereas it is actually looking up other data. I realize
> >>that our current code isn't going to be able to hit this, but we need to
> >>account for the possibility. I'd suggest that we might want to build the
> >>hash key internal to sss_dp_issue_request() and concatenate it to the
> >>pointer address of the msg_create function plus a string key argument
> >>(instead of a hash_key_t). This way, we know for certain that it is two
> >>requests with the same arguments that are expecting the same reply. So
> >>hash keys would look like: "0xdeadbeef:1:jhrozek at mydomain" (for an
> >>SSS_DP_USER request against all domains with no 'extra' argument).
> >>
> >
> >Fixed.
> >
> >>Typo: "DEBUG(SSSDBG_CRIT_FAILURE, ("The request has disappearead?\n"));"
> >>Should be: "DEBUG(SSSDBG_CRIT_FAILURE, ("The request has
> >>disappeared?\n"));
> >>
> >
> >Fixed.
> >
> >>While we're making such sweeping changes, could we consider renaming the
> >>return values err_maj and err_min to "dp_err" and "errno", respectively?
> >>(This isn't mandatory, but it might not be a bad time to do so.). It
> >>would also be helpful if those labels were used in error messages where
> >>the DP response is printed. (Seeing [1][22][(null)] in the debug logs
> >>means a trip to the source to decode).
> >>
> >
> >I changed the names to dp_err and dp_ret to avoid name-clash with errno
> >from errno.h
> >
> >>In general, I think this is a great idea and a terrific refactor. We
> >>just need to clean up the above issues.
> >>
> >
> >I also exported sss_dp_issue_request() in responder.h and made it
> >non-static as requested in the other mail.
> >
> >Thank you for the review. A new patch is attached.
> >
> >
-------------- next part --------------
From 4fc5bd57f04eaeae47fdb9675ad86e70202f9b94 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Fri, 20 Jan 2012 13:53:16 +0100
Subject: [PATCH] DP: Refactor responder_dp_req so it's reusable by other
 responders

* the internal requst is now more generic and is decoupled from
  account-specific data. There is a new sss_dp_issue_request() wrapper
  that issues a BE request or registers a callback
* the public requests all use struct sss_dp_req_state as the tevent_req
  state data. This allows to report back data from the internal request
  even if the caller is just a callback notifier
* each specific request now uses an _info structure that contains all
  the data necessary to construct a DBusMessage passed to provider
* each specific request now defines a sss_dp_get_$data_msg callback that
  is called from the sss_dp_issue_request() common wraper. The purpose
  of the wrapper is to construct a DBusMessage and bind it to a DBus
  method so the message can be just sent over to back end

The miscellanous changes include:
* change SSS_DP_ constants to an enum. This way, a switch() would error
  if a value is not handled.
* rename sss_dp_get_account_int_send() to sss_dp_internal_get_send()
  request because the internal request is going to handle more than just
  account data
* the DBus return values were renamed from err_maj, err_min to dp_err
  and dp_ret respectively
---
 src/responder/common/responder.h    |   56 ++++-
 src/responder/common/responder_dp.c |  540 ++++++++++++++++++++---------------
 src/responder/nss/nsssrv_services.c |    3 +-
 3 files changed, 354 insertions(+), 245 deletions(-)

diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 2944aa5c47ffd33df53933f50d47dc770db100c7..235b1d97bc6425ad71bccbc8d7572d8239856932 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -157,12 +157,6 @@ void sss_cmd_done(struct cli_ctx *cctx, void *freectx);
 int sss_cmd_get_version(struct cli_ctx *cctx);
 struct cli_protocol_version *register_cli_protocol_version(void);
 
-#define SSS_DP_USER 1
-#define SSS_DP_GROUP 2
-#define SSS_DP_INITGROUPS 3
-#define SSS_DP_NETGR 4
-#define SSS_DP_SERVICES 5
-
 typedef void (*sss_dp_callback_t)(uint16_t err_maj, uint32_t err_min,
                                   const char *err_msg, void *ptr);
 
@@ -179,6 +173,45 @@ void handle_requests_after_reconnect(void);
 int responder_logrotate(DBusMessage *message,
                         struct sbus_connection *conn);
 
+/* Each responder-specific request must create a constructor
+ * function that creates a DBus Message that would be sent to
+ * the back end
+ */
+typedef DBusMessage * (dbus_msg_constructor)(void *);
+
+/*
+ * This function is indended for consumption by responders to create
+ * responder-specific requests such as sss_dp_get_account_send for
+ * downloading account data.
+ *
+ * Issues a new back end request based on strkey if not already running
+ * or registers a callback that is called when an existing request finishes.
+ */
+errno_t
+sss_dp_issue_request(TALLOC_CTX *mem_ctx, struct resp_ctx *rctx,
+                     const char *strkey, struct sss_domain_info *dom,
+                     dbus_msg_constructor msg_create, void *pvt,
+                     struct tevent_req *nreq);
+
+/* Every provider specific request uses this structure as the tevent_req
+ * "state" structure.
+ */
+struct sss_dp_req_state {
+    dbus_uint16_t dp_err;
+    dbus_uint32_t dp_ret;
+    char *err_msg;
+};
+
+/* The _recv functions of provider specific requests usually need to
+ * only call sss_dp_req_recv() to get return codes from back end
+ */
+errno_t
+sss_dp_req_recv(TALLOC_CTX *mem_ctx,
+                struct tevent_req *sidereq,
+                dbus_uint16_t *dp_err,
+                dbus_uint32_t *dp_ret,
+                char **err_msg);
+
 /* Send a request to the data provider
  * Once this function is called, the communication
  * with the data provider will always run to
@@ -186,12 +219,21 @@ int responder_logrotate(DBusMessage *message,
  * cancel the notification of completion, but not
  * the data provider action.
  */
+
+enum sss_dp_acct_type {
+    SSS_DP_USER = 1,
+    SSS_DP_GROUP,
+    SSS_DP_INITGROUPS,
+    SSS_DP_NETGR,
+    SSS_DP_SERVICES
+};
+
 struct tevent_req *
 sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
                         struct resp_ctx *rctx,
                         struct sss_domain_info *dom,
                         bool fast_reply,
-                        int type,
+                        enum sss_dp_acct_type type,
                         const char *opt_name,
                         uint32_t opt_id,
                         const char *extra);
diff --git a/src/responder/common/responder_dp.c b/src/responder/common/responder_dp.c
index 5174bd3e9fe71c01a6e4885f3e969709b58d9564..aa439a6bdd49764b421dc8276b849d07b26b3f75 100644
--- a/src/responder/common/responder_dp.c
+++ b/src/responder/common/responder_dp.c
@@ -32,18 +32,6 @@ hash_table_t *dp_requests = NULL;
 
 struct sss_dp_req;
 
-struct dp_get_account_state {
-    struct resp_ctx *rctx;
-    struct sss_domain_info *dom;
-    const char *opt_name;
-    uint32_t opt_id;
-    hash_key_t *key;
-
-    dbus_uint16_t err_maj;
-    dbus_uint32_t err_min;
-    char *err_msg;
-};
-
 struct sss_dp_callback {
     struct sss_dp_callback *prev;
     struct sss_dp_callback *next;
@@ -62,8 +50,8 @@ struct sss_dp_req {
     struct tevent_timer *tev;
     struct sss_dp_callback *cb_list;
 
-    dbus_uint16_t err_maj;
-    dbus_uint32_t err_min;
+    dbus_uint16_t dp_err;
+    dbus_uint32_t dp_ret;
     char *err_msg;
 };
 
@@ -81,7 +69,7 @@ static int sss_dp_req_destructor(void *ptr)
 {
     struct sss_dp_callback *cb;
     struct sss_dp_req *sdp_req = talloc_get_type(ptr, struct sss_dp_req);
-    struct dp_get_account_state *state;
+    struct sss_dp_req_state *state;
 
     /* Cancel Dbus pending reply if still pending */
     if (sdp_req->pending_reply) {
@@ -93,9 +81,9 @@ static int sss_dp_req_destructor(void *ptr)
      * an error now.
      */
     while((cb = sdp_req->cb_list) != NULL) {
-        state = tevent_req_data(cb->req, struct dp_get_account_state);
-        state->err_maj = DP_ERR_FATAL;
-        state->err_min = EIO;
+        state = tevent_req_data(cb->req, struct sss_dp_req_state);
+        state->dp_err = DP_ERR_FATAL;
+        state->dp_ret = EIO;
 
         /* tevent_req_done/error will free cb */
         tevent_req_error(cb->req, EIO);
@@ -156,8 +144,8 @@ void handle_requests_after_reconnect(void)
     }
 }
 static int sss_dp_get_reply(DBusPendingCall *pending,
-                            dbus_uint16_t *err_maj,
-                            dbus_uint32_t *err_min,
+                            dbus_uint16_t *dp_err,
+                            dbus_uint32_t *dp_ret,
                             char **err_msg)
 {
     DBusMessage *reply;
@@ -187,8 +175,8 @@ static int sss_dp_get_reply(DBusPendingCall *pending,
     switch (type) {
     case DBUS_MESSAGE_TYPE_METHOD_RETURN:
         ret = dbus_message_get_args(reply, &dbus_error,
-                                    DBUS_TYPE_UINT16, err_maj,
-                                    DBUS_TYPE_UINT32, err_min,
+                                    DBUS_TYPE_UINT16, dp_err,
+                                    DBUS_TYPE_UINT32, dp_ret,
                                     DBUS_TYPE_STRING, err_msg,
                                     DBUS_TYPE_INVALID);
         if (!ret) {
@@ -198,9 +186,11 @@ static int sss_dp_get_reply(DBusPendingCall *pending,
             err = EIO;
             goto done;
         }
-        DEBUG(4, ("Got reply (%u, %u, %s) from Data Provider\n",
-                  (unsigned int)*err_maj, (unsigned int)*err_min, *err_msg));
-
+        DEBUG(SSSDBG_TRACE_LIBS,
+              ("Got reply from Data Provider - "
+               "DP error code: %u errno: %u error message: %s\n",
+              (unsigned int)*dp_err, (unsigned int)*dp_ret,
+              *err_msg ? *err_msg : "none"));
         break;
 
     case DBUS_MESSAGE_TYPE_ERROR:
@@ -233,162 +223,86 @@ done:
 }
 
 static struct tevent_req *
-sss_dp_get_account_int_send(struct resp_ctx *rctx,
-                            hash_key_t *key,
-                            struct sss_domain_info *dom,
-                            uint32_t be_type,
-                            const char *filter);
+sss_dp_internal_get_send(struct resp_ctx *rctx,
+                         hash_key_t *key,
+                         struct sss_domain_info *dom,
+                         DBusMessage *msg);
 
 static void
-sss_dp_get_account_done(struct tevent_req *subreq);
+sss_dp_req_done(struct tevent_req *sidereq);
 
-/* Send a request to the data provider
- * Once this function is called, the communication
- * with the data provider will always run to
- * completion. Freeing the returned tevent_req will
- * cancel the notification of completion, but not
- * the data provider action.
- */
-struct tevent_req *
-sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
-                        struct resp_ctx *rctx,
-                        struct sss_domain_info *dom,
-                        bool fast_reply,
-                        int type,
-                        const char *opt_name,
-                        uint32_t opt_id,
-                        const char *extra)
+errno_t
+sss_dp_issue_request(TALLOC_CTX *mem_ctx, struct resp_ctx *rctx,
+                     const char *strkey, struct sss_domain_info *dom,
+                     dbus_msg_constructor msg_create, void *pvt,
+                     struct tevent_req *nreq)
 {
-    errno_t ret;
     int hret;
-    struct tevent_req *req;
+    hash_value_t value;
+    hash_key_t *key;
     struct tevent_req *sidereq;
-    struct dp_get_account_state *state;
     struct sss_dp_req *sdp_req;
     struct sss_dp_callback *cb;
-    char *filter;
-    uint32_t be_type;
-    hash_value_t value;
+    DBusMessage *msg;
+    TALLOC_CTX *tmp_ctx = NULL;
+    errno_t ret;
 
-    req = tevent_req_create(mem_ctx, &state, struct dp_get_account_state);
-    if (!req) {
-        return NULL;
+    tmp_ctx = talloc_new(NULL);
+    if (!tmp_ctx) {
+        return ENOMEM;
     }
 
-    /* either, or, not both */
-    if (opt_name && opt_id) {
-        ret = EINVAL;
-        goto error;
-    }
-
-    if (!dom) {
-        ret = EINVAL;
-        goto error;
-    }
-
-    state->rctx = rctx;
-    state->dom = dom;
-
-    switch (type) {
-    case SSS_DP_USER:
-        be_type = BE_REQ_USER;
-        break;
-    case SSS_DP_GROUP:
-        be_type = BE_REQ_GROUP;
-        break;
-    case SSS_DP_INITGROUPS:
-        be_type = BE_REQ_INITGROUPS;
-        break;
-    case SSS_DP_NETGR:
-        be_type = BE_REQ_NETGROUP;
-        break;
-    case SSS_DP_SERVICES:
-        be_type = BE_REQ_SERVICES;
-        break;
-    default:
-        ret = EINVAL;
-        goto error;
-    }
-
-    if (fast_reply) {
-        be_type |= BE_REQ_FAST;
-    }
-
-    /* Check whether there's already an identical request in progress */
-
-    state->key = talloc(state, hash_key_t);
-    if (!state->key) {
+    key = talloc(tmp_ctx, hash_key_t);
+    if (!key) {
         ret = ENOMEM;
-        goto error;
+        goto fail;
     }
 
-    state->key->type = HASH_KEY_STRING;
-
-    if (opt_name) {
-        if (extra) {
-            filter = talloc_asprintf(state, "name=%s:%s",
-                                     opt_name, extra);
-            state->key->str = talloc_asprintf(state->key, "%d:%s:%s@%s",
-                                              type, opt_name,
-                                              extra, dom->name);
-        } else {
-            filter = talloc_asprintf(state, "name=%s", opt_name);
-            state->key->str = talloc_asprintf(state->key, "%d:%s@%s",
-                                              type, opt_name, dom->name);
-        }
-    } else if (opt_id) {
-        if (extra) {
-            filter = talloc_asprintf(state, "idnumber=%u:%s",
-                                     opt_id, extra);
-            state->key->str = talloc_asprintf(state->key, "%d:%d:%s@%s",
-                                              type, opt_id,
-                                              extra, dom->name);
-        } else {
-            filter = talloc_asprintf(state, "idnumber=%u", opt_id);
-            state->key->str = talloc_asprintf(state->key, "%d:%d@%s",
-                                              type, opt_id, dom->name);
-        }
-    } else {
-        filter = talloc_strdup(state, ENUM_INDICATOR);
-        state->key->str = talloc_asprintf(state->key, "%d:*@%s",
-                                          type, dom->name);
-    }
-    if (!filter || !state->key->str) {
+    key->type = HASH_KEY_STRING;
+    key->str = talloc_asprintf(key, "%p:%s\n", msg_create, strkey);
+    if (!key->str) {
         ret = ENOMEM;
+        goto fail;
     }
 
     /* Check the hash for existing references to this request */
-    hret = hash_lookup(rctx->dp_request_table, state->key, &value);
+    hret = hash_lookup(rctx->dp_request_table, key, &value);
     switch (hret) {
     case HASH_SUCCESS:
         /* Request already in progress */
         DEBUG(SSSDBG_TRACE_FUNC,
-              ("Identical request in progress: [%s]\n", state->key->str));
+              ("Identical request in progress: [%s]\n", key->str));
         break;
 
     case HASH_ERROR_KEY_NOT_FOUND:
         /* No such request in progress
          * Create a new request
          */
+        msg = msg_create(pvt);
+        if (!msg) {
+            DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot create D-Bus message\n"));
+            ret = EIO;
+            goto fail;
+        }
 
         value.type = HASH_VALUE_PTR;
-        sidereq = sss_dp_get_account_int_send(rctx, state->key, dom,
-                                             be_type, filter);
+        sidereq = sss_dp_internal_get_send(rctx, key, dom, msg);
+        dbus_message_unref(msg);
         if (!sidereq) {
-            ret = ENOMEM;
-            goto error;
+            DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot send D-Bus message\n"));
+            ret = EIO;
+            goto fail;
         }
-        tevent_req_set_callback(sidereq, sss_dp_get_account_done, NULL);
+        tevent_req_set_callback(sidereq, sss_dp_req_done, NULL);
 
         /* We should now be able to find the sdp_req in the hash table */
-        hret = hash_lookup(rctx->dp_request_table, state->key, &value);
+        hret = hash_lookup(rctx->dp_request_table, key, &value);
         if (hret != HASH_SUCCESS) {
             /* Something must have gone wrong with creating the request */
-            talloc_zfree(sidereq);
+            DEBUG(SSSDBG_CRIT_FAILURE, ("The request has disappeared?\n"));
             ret = EIO;
-            goto error;
+            goto fail;
         }
-
         break;
 
     default:
@@ -396,24 +310,24 @@ sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
               ("Could not query request list (%s)\n",
                hash_error_string(hret)));
         ret = EIO;
-        goto error;
+        goto fail;
     }
 
     /* Register this request for results */
     sdp_req = talloc_get_type(value.ptr, struct sss_dp_req);
     if (!sdp_req) {
-        DEBUG(0, ("Could not retrieve DP request context\n"));
+        DEBUG(SSSDBG_CRIT_FAILURE, ("Could not retrieve DP request context\n"));
         ret = EIO;
-        goto error;
+        goto fail;
     }
 
-    cb = talloc_zero(state, struct sss_dp_callback);
+    cb = talloc_zero(mem_ctx, struct sss_dp_callback);
     if (!cb) {
         ret = ENOMEM;
-        goto error;
+        goto fail;
     }
 
-    cb->req = req;
+    cb->req = nreq;
     cb->sdp_req = sdp_req;
 
     /* Add it to the list of requests to call */
@@ -422,42 +336,40 @@ sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
     talloc_set_destructor((TALLOC_CTX *)cb,
                           sss_dp_callback_destructor);
 
-    return req;
-
-error:
-    tevent_req_error(req, ret);
-    tevent_req_post(req, rctx->ev);
-    return req;
+    ret = EOK;
+fail:
+    talloc_free(tmp_ctx);
+    return ret;
 }
 
 static void
-sss_dp_get_account_done(struct tevent_req *sidereq)
+sss_dp_req_done(struct tevent_req *sidereq)
 {
     /* Nothing to do here. The callbacks have already been invoked */
     talloc_zfree(sidereq);
 }
 
 errno_t
-sss_dp_get_account_recv(TALLOC_CTX *mem_ctx,
-                        struct tevent_req *req,
-                        dbus_uint16_t *err_maj,
-                        dbus_uint32_t *err_min,
-                        char **err_msg)
+sss_dp_req_recv(TALLOC_CTX *mem_ctx,
+                struct tevent_req *sidereq,
+                dbus_uint16_t *dp_err,
+                dbus_uint32_t *dp_ret,
+                char **err_msg)
 {
-    struct dp_get_account_state *state =
-            tevent_req_data(req, struct dp_get_account_state);
+    struct sss_dp_req_state *state =
+            tevent_req_data(sidereq, struct sss_dp_req_state);
 
     enum tevent_req_state TRROEstate;
     uint64_t TRROEerr;
 
-    *err_maj = state->err_maj;
-    *err_min = state->err_min;
+    *dp_err = state->dp_err;
+    *dp_ret = state->dp_ret;
     *err_msg = talloc_steal(mem_ctx, state->err_msg);
 
-    if (tevent_req_is_error(req, &TRROEstate, &TRROEerr)) {
+    if (tevent_req_is_error(sidereq, &TRROEstate, &TRROEerr)) {
         if (TRROEstate == TEVENT_REQ_USER_ERROR) {
-            *err_maj = DP_ERR_FATAL;
-            *err_min = TRROEerr;
+            *dp_err = DP_ERR_FATAL;
+            *dp_ret = TRROEerr;
         } else {
             return EIO;
         }
@@ -466,35 +378,224 @@ sss_dp_get_account_recv(TALLOC_CTX *mem_ctx,
     return EOK;
 }
 
-struct dp_get_account_int_state {
-    struct resp_ctx *rctx;
+/* Send a request to the data provider
+ * Once this function is called, the communication
+ * with the data provider will always run to
+ * completion. Freeing the returned tevent_req will
+ * cancel the notification of completion, but not
+ * the data provider action.
+ */
+static DBusMessage *sss_dp_get_account_msg(void *pvt);
+
+struct sss_dp_account_info {
     struct sss_domain_info *dom;
-    uint32_t be_type;
-    const char *filter;
 
-    struct sss_dp_req *sdp_req;
-    DBusPendingCall *pending_reply;
+    bool fast_reply;
+    enum sss_dp_acct_type type;
+    const char *opt_name;
+    const char *extra;
+    uint32_t opt_id;
 };
 
-static void sss_dp_get_account_int_done(DBusPendingCall *pending, void *ptr);
-
-static struct tevent_req *
-sss_dp_get_account_int_send(struct resp_ctx *rctx,
-                            hash_key_t *key,
-                            struct sss_domain_info *dom,
-                            uint32_t be_type,
-                            const char *filter)
+struct tevent_req *
+sss_dp_get_account_send(TALLOC_CTX *mem_ctx,
+                        struct resp_ctx *rctx,
+                        struct sss_domain_info *dom,
+                        bool fast_reply,
+                        enum sss_dp_acct_type type,
+                        const char *opt_name,
+                        uint32_t opt_id,
+                        const char *extra)
 {
     errno_t ret;
-    int hret;
     struct tevent_req *req;
-    struct dp_get_account_int_state *state;
-    struct be_conn *be_conn;
+    struct sss_dp_account_info *info;
+    struct sss_dp_req_state *state;
+    char *key;
+
+    req = tevent_req_create(mem_ctx, &state, struct sss_dp_req_state);
+    if (!req) {
+        return NULL;
+    }
+
+    /* either, or, not both */
+    if (opt_name && opt_id) {
+        ret = EINVAL;
+        goto error;
+    }
+
+    if (!dom) {
+        ret = EINVAL;
+        goto error;
+    }
+
+    info = talloc_zero(state, struct sss_dp_account_info);
+    info->fast_reply = fast_reply;
+    info->type = type;
+    info->opt_name = opt_name;
+    info->opt_id = opt_id;
+    info->extra = extra;
+    info->dom = dom;
+
+    if (opt_name) {
+        if (extra) {
+            key = talloc_asprintf(state, "%d:%s:%s@%s",
+                                  type, opt_name, extra, dom->name);
+        } else {
+            key = talloc_asprintf(state, "%d:%s@%s",
+                                  type, opt_name, dom->name);
+        }
+    } else if (opt_id) {
+        if (extra) {
+            key = talloc_asprintf(state, "%d:%d:%s@%s",
+                                  type, opt_id, extra, dom->name);
+        } else {
+            key = talloc_asprintf(state, "%d:%d@%s", type, opt_id, dom->name);
+        }
+    } else {
+        key = talloc_asprintf(state, "%d:*@%s", type, dom->name);
+    }
+    if (!key) {
+        ret = ENOMEM;
+        goto error;
+    }
+
+    ret = sss_dp_issue_request(state, rctx, key, dom, sss_dp_get_account_msg,
+                               info, req);
+    talloc_free(key);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE,
+              ("Could not issue DP request [%d]: %s\n",
+               ret, strerror(ret)));
+        goto error;
+    }
+
+    return req;
+
+error:
+    tevent_req_error(req, ret);
+    tevent_req_post(req, rctx->ev);
+    return req;
+}
+
+static DBusMessage *
+sss_dp_get_account_msg(void *pvt)
+{
     DBusMessage *msg;
     dbus_bool_t dbret;
-    bool msg_created = false;
-    hash_value_t value;
+    struct sss_dp_account_info *info;
+    uint32_t be_type;
     uint32_t attrs = BE_ATTR_CORE;
+    char *filter;
+
+    info = talloc_get_type(pvt, struct sss_dp_account_info);
+
+    switch (info->type) {
+        case SSS_DP_USER:
+            be_type = BE_REQ_USER;
+            break;
+        case SSS_DP_GROUP:
+            be_type = BE_REQ_GROUP;
+            break;
+        case SSS_DP_INITGROUPS:
+            be_type = BE_REQ_INITGROUPS;
+            break;
+        case SSS_DP_NETGR:
+            be_type = BE_REQ_NETGROUP;
+            break;
+        case SSS_DP_SERVICES:
+            be_type = BE_REQ_SERVICES;
+            break;
+    }
+
+    if (info->fast_reply) {
+        be_type |= BE_REQ_FAST;
+    }
+
+    if (info->opt_name) {
+        if (info->extra) {
+            filter = talloc_asprintf(info, "name=%s:%s",
+                                     info->opt_name, info->extra);
+        } else {
+            filter = talloc_asprintf(info, "name=%s", info->opt_name);
+        }
+    } else if (info->opt_id) {
+        if (info->extra) {
+            filter = talloc_asprintf(info, "idnumber=%u:%s",
+                                     info->opt_id, info->extra);
+        } else {
+            filter = talloc_asprintf(info, "idnumber=%u", info->opt_id);
+        }
+    } else {
+        filter = talloc_strdup(info, ENUM_INDICATOR);
+    }
+    if (!filter) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory?!\n"));
+        return NULL;
+    }
+
+    msg = dbus_message_new_method_call(NULL,
+                                       DP_PATH,
+                                       DP_INTERFACE,
+                                       DP_METHOD_GETACCTINFO);
+    if (msg == NULL) {
+        talloc_free(filter);
+        DEBUG(SSSDBG_CRIT_FAILURE, ("Out of memory?!\n"));
+        return NULL;
+    }
+
+    /* create the message */
+    DEBUG(SSSDBG_TRACE_FUNC,
+          ("Creating request for [%s][%u][%d][%s]\n",
+           info->dom->name, be_type, attrs, filter));
+
+    dbret = dbus_message_append_args(msg,
+                                     DBUS_TYPE_UINT32, &be_type,
+                                     DBUS_TYPE_UINT32, &attrs,
+                                     DBUS_TYPE_STRING, &filter,
+                                     DBUS_TYPE_INVALID);
+    talloc_free(filter);
+    if (!dbret) {
+        DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to build message\n"));
+        dbus_message_unref(msg);
+        return NULL;
+    }
+
+    return msg;
+}
+
+errno_t
+sss_dp_get_account_recv(TALLOC_CTX *mem_ctx,
+                        struct tevent_req *req,
+                        dbus_uint16_t *dp_err,
+                        dbus_uint32_t *dp_ret,
+                        char **err_msg)
+{
+    return sss_dp_req_recv(mem_ctx, req, dp_err, dp_ret, err_msg);
+}
+
+struct dp_internal_get_state {
+    struct resp_ctx *rctx;
+    struct sss_domain_info *dom;
+
+    struct sss_dp_req *sdp_req;
+    DBusPendingCall *pending_reply;
+};
+
+static void sss_dp_internal_get_done(DBusPendingCall *pending, void *ptr);
+
+static struct tevent_req *
+sss_dp_internal_get_send(struct resp_ctx *rctx,
+                         hash_key_t *key,
+                         struct sss_domain_info *dom,
+                         DBusMessage *msg)
+{
+    errno_t ret;
+    int hret;
+    struct tevent_req *req;
+    struct dp_internal_get_state *state;
+    struct be_conn *be_conn;
+    hash_value_t value;
 
     /* Internal requests need to be allocated on the responder context
      * so that they don't go away if a client disconnects. The worst-
@@ -503,13 +604,11 @@ sss_dp_get_account_int_send(struct resp_ctx *rctx,
      */
     req = tevent_req_create(rctx,
                             &state,
-                            struct dp_get_account_int_state);
+                            struct dp_internal_get_state);
     if (!req)  return NULL;
 
     state->rctx = rctx;
     state->dom = dom;
-    state->be_type = be_type;
-    state->filter = filter;
 
     state->sdp_req = talloc_zero(state, struct sss_dp_req);
     if (!state->sdp_req) {
@@ -538,40 +637,11 @@ sss_dp_get_account_int_send(struct resp_ctx *rctx,
         goto error;
     }
 
-    /* create the message */
-    msg = dbus_message_new_method_call(NULL,
-                                       DP_PATH,
-                                       DP_INTERFACE,
-                                       DP_METHOD_GETACCTINFO);
-    if (msg == NULL) {
-        DEBUG(0,("Out of memory?!\n"));
-        ret = ENOMEM;
-        goto error;
-    }
-    msg_created = true;
-
-    DEBUG(SSSDBG_TRACE_FUNC,
-          ("Sending request for [%s][%u][%d][%s]\n",
-           dom->name, be_type, attrs, filter));
-
-    dbret = dbus_message_append_args(msg,
-                                     DBUS_TYPE_UINT32, &be_type,
-                                     DBUS_TYPE_UINT32, &attrs,
-                                     DBUS_TYPE_STRING, &filter,
-                                     DBUS_TYPE_INVALID);
-    if (!dbret) {
-        DEBUG(1,("Failed to build message\n"));
-        ret = EIO;
-        goto error;
-    }
-
     ret = sbus_conn_send(be_conn->conn, msg,
                          SSS_CLI_SOCKET_TIMEOUT / 2,
-                         sss_dp_get_account_int_done,
+                         sss_dp_internal_get_done,
                          req,
                          &state->sdp_req->pending_reply);
-    dbus_message_unref(msg);
-    msg_created = false;
     if (ret != EOK) {
         /*
          * Critical Failure
@@ -601,44 +671,40 @@ sss_dp_get_account_int_send(struct resp_ctx *rctx,
     return req;
 
 error:
-    if (msg_created) {
-        dbus_message_unref(msg);
-    }
-
     tevent_req_error(req, ret);
     tevent_req_post(req, rctx->ev);
     return req;
 }
 
-static void sss_dp_get_account_int_done(DBusPendingCall *pending, void *ptr)
+static void sss_dp_internal_get_done(DBusPendingCall *pending, void *ptr)
 {
     int ret;
     struct tevent_req *req;
     struct sss_dp_req *sdp_req;
     struct sss_dp_callback *cb;
-    struct dp_get_account_int_state *state;
-    struct dp_get_account_state *cb_state;
+    struct dp_internal_get_state *state;
+    struct sss_dp_req_state *cb_state;
 
     req = talloc_get_type(ptr, struct tevent_req);
-    state = tevent_req_data(req, struct dp_get_account_int_state);
+    state = tevent_req_data(req, struct dp_internal_get_state);
     sdp_req = state->sdp_req;
 
     /* prevent trying to cancel a reply that we already received */
     sdp_req->pending_reply = NULL;
 
     ret = sss_dp_get_reply(pending,
-                           &sdp_req->err_maj,
-                           &sdp_req->err_min,
+                           &sdp_req->dp_err,
+                           &sdp_req->dp_ret,
                            &sdp_req->err_msg);
     if (ret != EOK) {
         if (ret == ETIME) {
-            sdp_req->err_maj = DP_ERR_TIMEOUT;
-            sdp_req->err_min = ret;
+            sdp_req->dp_err = DP_ERR_TIMEOUT;
+            sdp_req->dp_ret = ret;
             sdp_req->err_msg = talloc_strdup(sdp_req, "Request timed out");
         }
         else {
-            sdp_req->err_maj = DP_ERR_FATAL;
-            sdp_req->err_min = ret;
+            sdp_req->dp_err = DP_ERR_FATAL;
+            sdp_req->dp_ret = ret;
             sdp_req->err_msg =
                 talloc_strdup(sdp_req,
                               "Failed to get reply from Data Provider");
@@ -647,9 +713,9 @@ static void sss_dp_get_account_int_done(DBusPendingCall *pending, void *ptr)
 
     /* Check whether we need to issue any callbacks */
     while ((cb = sdp_req->cb_list) != NULL) {
-        cb_state = tevent_req_data(cb->req, struct dp_get_account_state);
-        cb_state->err_maj = sdp_req->err_maj;
-        cb_state->err_min = sdp_req->err_min;
+        cb_state = tevent_req_data(cb->req, struct sss_dp_req_state);
+        cb_state->dp_err = sdp_req->dp_err;
+        cb_state->dp_ret = sdp_req->dp_ret;
         cb_state->err_msg = talloc_strdup(cb_state, sdp_req->err_msg);
         /* Don't bother checking for NULL. If it fails due to ENOMEM,
          * we can't really handle it anyway.
diff --git a/src/responder/nss/nsssrv_services.c b/src/responder/nss/nsssrv_services.c
index c651ceb586e00a38c46df9cf316f6a732e23d3b2..a0d509cfa28cae0df953011ce6a78c62882e6368 100644
--- a/src/responder/nss/nsssrv_services.c
+++ b/src/responder/nss/nsssrv_services.c
@@ -468,7 +468,8 @@ static void lookup_service_done(struct tevent_req *subreq)
               ("Unable to get information from Data Provider\n"
                "dp_error: [%u], errno: [%u], error_msg: [%s]\n"
                "Will try to return what we have in cache\n",
-               (unsigned int)err_maj, (unsigned int)err_min, err_msg));
+               (unsigned int)err_maj, (unsigned int)err_min,
+               err_msg ? err_msg : "none"));
     }
 
     /* Recheck the cache after the lookup.
-- 
1.7.7.5



More information about the sssd-devel mailing list