Improve error handling and replies in the InfoPipe

When detecting an internal error in the InfoPipe, make a best-
attempt at sending an error message back to the calling program,
instead of simply leaving the client to wait for the timeout.

3 files changed, 62 insertions, 48 deletions

diff --git a/server/infopipe/infopipe.c b/server/infopipe/infopipe.c
index 4ec971caa..8d4bfeefa 100644
--- a/server/infopipe/infopipe.c
+++ b/server/infopipe/infopipe.c
@@ -144,11 +144,15 @@ static int infp_monitor_init(struct infp_ctx *infp_ctx)
 /* Helper function to return an immediate error message in the event
  * of internal error in the InfoPipe to avoid forcing the clients to
  * time out waiting for a reply.
+ * This function will make a best effort to send a reply, but if it
+ * fails, clients will simply have to handle the timeout.
  */
 void infp_return_failure(struct infp_req_ctx *infp_req, const char *message)
 {
     DBusMessage *reply;
 
+    if(infp_req == NULL) return;
+
     reply = dbus_message_new_error(infp_req->req_message,
                                    DBUS_ERROR_FAILED,
                                    message);
@@ -172,6 +176,8 @@ void infp_return_success(struct infp_req_ctx *infp_req)
 {
     DBusMessage *reply;
 
+    if (infp_req == NULL) return;
+
     reply = dbus_message_new_method_return(infp_req->req_message);
     /* If the reply was NULL, we ran out of memory, so we won't
      * bother trying to queue the message to send. In this case,
diff --git a/server/infopipe/infopipe_groups.c b/server/infopipe/infopipe_groups.c
index 9aaecf2f2..4b2c881c2 100644
--- a/server/infopipe/infopipe_groups.c
+++ b/server/infopipe/infopipe_groups.c
@@ -59,9 +59,12 @@ static void infp_do_group_create_callback(void *pvt, int status,
             reply = dbus_message_new_error(grcreate_req->infp_req->req_message,
                                            DBUS_ERROR_FILE_EXISTS,
                                            error_msg);
+            if (reply)
+                sbus_conn_send_reply(grcreate_req->infp_req->sconn, reply);
+        }
+        else {
+            infp_return_failure(grcreate_req->infp_req, NULL);
         }
-        if (reply)
-            sbus_conn_send_reply(grcreate_req->infp_req->sconn, reply);
         talloc_free(grcreate_req);
         return;
     }
@@ -75,9 +78,8 @@ static void infp_do_group_create_callback(void *pvt, int status,
 
     /* We have no more usernames to add, so commit the transaction */
     sysdb_transaction_done(grcreate_req->sysdb_req, status);
-    reply =
-        dbus_message_new_method_return(grcreate_req->infp_req->req_message);
-    if (reply) sbus_conn_send_reply(grcreate_req->infp_req->sconn, reply);
+
+    infp_return_success(grcreate_req->infp_req);
     talloc_free(grcreate_req);
     return;
 }
@@ -97,6 +99,7 @@ static void infp_do_group_create(struct sysdb_req *req, void *pvt)
     if (ret != EOK) {
         DEBUG(0, ("Could not invoke sysdb_add_group\n"));
         sysdb_transaction_done(grcreate_req->sysdb_req, ret);
+        infp_return_failure(grcreate_req->infp_req, NULL);
         talloc_free(grcreate_req);
         return;
     }
@@ -221,6 +224,7 @@ einval:
 
 error:
     if (arg_grnames) dbus_free_string_array(arg_grnames);
+    if(grcreate_req) infp_return_failure(grcreate_req->infp_req, NULL);
     talloc_free(grcreate_req);
     return ret;
 }
@@ -234,7 +238,6 @@ struct infp_deletegroup_ctx {
 static void infp_do_group_delete_callback(void *pvt, int status,
                                           struct ldb_result *res)
 {
-    DBusMessage *reply = NULL;
     struct infp_deletegroup_ctx *grdel_req =
         talloc_get_type(pvt, struct infp_deletegroup_ctx);
 
@@ -244,15 +247,12 @@ static void infp_do_group_delete_callback(void *pvt, int status,
     if (status != EOK) {
         DEBUG(0, ("Failed to delete group from sysdb. Error code %d\n",
                   status));
+        infp_return_failure(grdel_req->infp_req, NULL);
         talloc_free(grdel_req);
         return;
     }
 
-    reply = dbus_message_new_method_return(grdel_req->infp_req->req_message);
-    if (reply) {
-        sbus_conn_send_reply(grdel_req->infp_req->sconn, reply);
-        dbus_message_unref(reply);
-    }
+    infp_return_success(grdel_req->infp_req);
     talloc_free(grdel_req);
 }
 
@@ -270,6 +270,7 @@ static void infp_do_group_delete(struct sysdb_req *req, void *pvt)
                              grdel_req);
     if (ret != EOK) {
         DEBUG(0, ("Could not delete group entry\n"));
+        infp_return_failure(grdel_req->infp_req, NULL);
         talloc_free(grdel_req);
         return;
     }
@@ -381,6 +382,7 @@ einval:
     return EOK;
 
 error:
+    if (grdel_req) infp_return_failure(grdel_req->infp_req, NULL);
     talloc_free(grdel_req);
     return ret;
 }
@@ -437,11 +439,7 @@ static void infp_do_member_callback(void *pvt, int status,
 fail:
 sysdb_transaction_done(grmod_req->sysdb_req, status);
     fail_msg = talloc_asprintf(grmod_req, "Could not modify group");
-    reply = dbus_message_new_error(grmod_req->infp_req->req_message,
-                                   DBUS_ERROR_FAILED,
-                                   fail_msg);
-    sbus_conn_send_reply(grmod_req->infp_req->sconn, reply);
-    dbus_message_unref(reply);
+    infp_return_failure(grmod_req->infp_req, fail_msg);
     talloc_free(grmod_req);
     return;
 }
@@ -492,6 +490,7 @@ static void infp_do_member(struct sysdb_req *req, void *pvt)
     return;
 
 error:
+    infp_return_failure(grmod_req->infp_req, NULL);
     talloc_free(grmod_req);
     return;
 }
@@ -722,6 +721,7 @@ static void infp_do_gid(struct sysdb_req *req, void *pvt)
                                            error_msg);
             if (reply) sbus_conn_send_reply(grmod_req->infp_req->sconn, reply);
         }
+        infp_return_failure(grmod_req->infp_req, NULL);
         talloc_free(grmod_req);
         return;
     }
@@ -831,8 +831,7 @@ einval:
     return EOK;
 
 error:
+    if(grmod_req) infp_return_failure(grmod_req->infp_req, NULL);
     talloc_free(grmod_req);
     return ret;
-
-
 }
diff --git a/server/infopipe/infopipe_users.c b/server/infopipe/infopipe_users.c
index 119c66fcc..196080905 100644
--- a/server/infopipe/infopipe_users.c
+++ b/server/infopipe/infopipe_users.c
@@ -52,6 +52,7 @@ static void infp_users_get_cached_callback(void *ptr,
 
     if (status != LDB_SUCCESS) {
         DEBUG(0, ("Failed to enumerate users in the cache db.\n"));
+        infp_return_failure(infp_getcached_req->infp_req, NULL);
         talloc_free(infp_getcached_req);
         return;
     }
@@ -59,6 +60,7 @@ static void infp_users_get_cached_callback(void *ptr,
     /* Construct a reply */
     reply = dbus_message_new_method_return(infp_getcached_req->infp_req->req_message);
     if(reply == NULL) {
+        infp_return_failure(infp_getcached_req->infp_req, NULL);
         talloc_free(infp_getcached_req);
         return;
     }
@@ -93,6 +95,7 @@ error:
           ("Critical error constructing reply message for %s\n",
             INFP_USERS_GET_CACHED));
     dbus_message_unref(reply);
+    infp_return_failure(infp_getcached_req->infp_req, NULL);
     talloc_free(infp_getcached_req);
     return;
 }
@@ -185,6 +188,8 @@ einval:
     return EOK;
 
 error:
+    if (infp_getcached_req)
+        infp_return_failure(infp_getcached_req->infp_req, NULL);
     talloc_free(infp_getcached_req);
     return ret;
 }
@@ -217,7 +222,7 @@ static void infp_do_user_create_callback(void *pvt,
      */
     if (status == EOK) {
         /* Return reply ack */
-        reply = dbus_message_new_method_return(infp_createuser_req->infp_req->req_message);
+        infp_return_success(infp_createuser_req->infp_req);
     }
     else if (status == EEXIST) {
         /* Return error, user already exists */
@@ -228,18 +233,17 @@ static void infp_do_user_create_callback(void *pvt,
         reply = dbus_message_new_error(infp_createuser_req->infp_req->req_message,
                                        DBUS_ERROR_FILE_EXISTS,
                                        error_msg);
+        if (reply) {
+            sbus_conn_send_reply(infp_createuser_req->infp_req->sconn, reply);
+            dbus_message_unref(reply);
+        }
     }
     else {
         /* Unknown error occurred. Print DEBUG message */
         DEBUG(0, ("Failed to create user in the sysdb. Error code %d\n", status));
-        talloc_free(infp_createuser_req);
-        return;
+        infp_return_failure(infp_createuser_req->infp_req, NULL);
     }
 
-    if (reply) {
-        sbus_conn_send_reply(infp_createuser_req->infp_req->sconn, reply);
-        dbus_message_unref(reply);
-    }
     talloc_free(infp_createuser_req);
 }
 
@@ -261,6 +265,7 @@ static void infp_do_user_create(struct sysdb_req *req, void *pvt)
     if (ret != EOK) {
         DEBUG(0, ("Could not invoke sysdb_add_user\n"));
         sysdb_transaction_done(infp_createuser_req->sysdb_req, ret);
+        infp_return_failure(infp_createuser_req->infp_req, NULL);
         talloc_free(infp_createuser_req);
         return;
     }
@@ -399,6 +404,8 @@ einval:
     return EOK;
 
 error:
+    if(infp_createuser_req)
+        infp_return_failure(infp_createuser_req->infp_req, NULL);
     talloc_free(infp_createuser_req);
     return ret;
 }
@@ -413,7 +420,6 @@ struct infp_deleteuser_ctx {
 static void infp_do_user_delete_callback(void *pvt, int status,
                                          struct ldb_result *res)
 {
-    DBusMessage *reply = NULL;
     struct infp_deleteuser_ctx *infp_deleteuser_req =
         talloc_get_type(pvt, struct infp_deleteuser_ctx);
 
@@ -422,16 +428,12 @@ static void infp_do_user_delete_callback(void *pvt, int status,
 
     if (status != EOK) {
         DEBUG(0, ("Failed to delete user from sysdb. Error code %d\n", status));
+        infp_return_failure(infp_deleteuser_req->infp_req, NULL);
         talloc_free(infp_deleteuser_req);
         return;
     }
 
-    reply = dbus_message_new_method_return(infp_deleteuser_req->infp_req->req_message);
-    if(reply) {
-        sbus_conn_send_reply(infp_deleteuser_req->infp_req->sconn,
-                             reply);
-        dbus_message_unref(reply);
-    }
+    infp_return_success(infp_deleteuser_req->infp_req);
     talloc_free(infp_deleteuser_req);
 }
 
@@ -448,6 +450,7 @@ static void infp_do_user_delete(struct sysdb_req *req, void *pvt)
                                                  infp_deleteuser_req->username);
     if(infp_deleteuser_req->user_dn == NULL) {
         DEBUG(0, ("Could not construct a user_dn for deletion.\n"));
+        infp_return_failure(infp_deleteuser_req->infp_req, NULL);
         talloc_free(infp_deleteuser_req);
         return;
     }
@@ -458,6 +461,7 @@ static void infp_do_user_delete(struct sysdb_req *req, void *pvt)
                              infp_deleteuser_req);
     if(ret != EOK) {
         DEBUG(0,("Could not delete user entry.\n"));
+        infp_return_failure(infp_deleteuser_req->infp_req, NULL);
         talloc_free(infp_deleteuser_req);
         return;
     }
@@ -576,6 +580,8 @@ einval:
     return EOK;
 
 error:
+    if(infp_deleteuser_req)
+        infp_return_failure(infp_deleteuser_req->infp_req, NULL);
     talloc_free(infp_deleteuser_req);
     return ret;
 }
@@ -888,6 +894,7 @@ static void infp_get_attr_lookup_callback(void *ptr, int ldb_status, struct ldb_
     /* Process the current results */
     if (ldb_status != LDB_SUCCESS) {
         DEBUG(0, ("Critical error reading from sysdb.\n"));
+        infp_return_failure(infp_getattr_req->infp_req, NULL);
         goto done;
     }
 
@@ -908,6 +915,7 @@ static void infp_get_attr_lookup_callback(void *ptr, int ldb_status, struct ldb_
 
         default:
             DEBUG(0, ("GetUser call returned more than one result. This probably means the sysdb is corrupt!\n"));
+            infp_return_failure(infp_getattr_req->infp_req, NULL);
             goto done;
         }
     }
@@ -928,12 +936,14 @@ static void infp_get_attr_lookup_callback(void *ptr, int ldb_status, struct ldb_
                                         &infp_getattr_req->results[infp_getattr_req->index]);
         if (ret != EOK) {
             DEBUG(0, ("Unable to create result map!\n"));
+            infp_return_failure(infp_getattr_req->infp_req, NULL);
             goto done;
         }
         break;
     default:
         /* We received more than one result. This is bad */
         DEBUG(0, ("GetUser call returned more than one result. This probably means the sysdb is corrupt!\n"));
+        infp_return_failure(infp_getattr_req->infp_req, NULL);
         goto done;
     }
 
@@ -943,6 +953,7 @@ static void infp_get_attr_lookup_callback(void *ptr, int ldb_status, struct ldb_
         ret = infp_get_attr_lookup(infp_getattr_req);
         if (ret != EOK) {
             DEBUG(0, ("Could not read from cache database\n"));
+            infp_return_failure(infp_getattr_req->infp_req, NULL);
             goto done;
         }
         return;
@@ -951,6 +962,7 @@ static void infp_get_attr_lookup_callback(void *ptr, int ldb_status, struct ldb_
     /* No more names remain, return the result DICTs */
     reply = dbus_message_new_method_return(infp_getattr_req->infp_req->req_message);
     if (reply == NULL) {
+        infp_return_failure(infp_getattr_req->infp_req, NULL);
         goto done;
     }
 
@@ -1262,6 +1274,7 @@ end:
     dbus_free_string_array(usernames);
     dbus_free_string_array(attributes);
     if (ret != EOK) {
+        infp_return_failure(infp_getattr_req->infp_req, NULL);
         talloc_free(infp_getattr_req);
     }
     return ret;
@@ -1288,7 +1301,6 @@ struct infp_setattr_ctx {
 static void infp_do_user_set_attr(struct sysdb_req *req, void *pvt);
 static void infp_do_user_set_attr_callback(void *ptr, int ldb_status, struct ldb_result *res)
 {
-    DBusMessage *reply;
     struct infp_setattr_ctx *infp_setattr_req;
 
     infp_setattr_req = talloc_get_type(ptr, struct infp_setattr_ctx);
@@ -1298,6 +1310,7 @@ static void infp_do_user_set_attr_callback(void *ptr, int ldb_status, struct ldb
         DEBUG(0, ("Failed to store user attributes to the sysdb\n"));
         /* Cancel the transaction */
         sysdb_transaction_done(infp_setattr_req->sysdb_req, sysdb_error_to_errno(ldb_status));
+        infp_return_failure(infp_setattr_req->infp_req, NULL);
         talloc_free(infp_setattr_req);
         return;
     }
@@ -1313,13 +1326,8 @@ static void infp_do_user_set_attr_callback(void *ptr, int ldb_status, struct ldb
     sysdb_transaction_done(infp_setattr_req->sysdb_req, EOK);
 
     /* Send reply ack */
-    reply = dbus_message_new_method_return(infp_setattr_req->infp_req->req_message);
-    if(reply == NULL) {
-        talloc_free(infp_setattr_req);
-        return;
-    }
-    sbus_conn_send_reply(infp_setattr_req->infp_req->sconn, reply);
-    dbus_message_unref(reply);
+    infp_return_success(infp_setattr_req->infp_req);
+
     talloc_free(infp_setattr_req);
 }
 
@@ -1341,6 +1349,7 @@ static void infp_do_user_set_attr(struct sysdb_req *req, void *pvt)
     if(ret != EOK) {
         DEBUG(0, ("Failed to set attributes for user [%s]. Cancelling transaction\n", infp_setattr_req->usernames[infp_setattr_req->index]));
         sysdb_transaction_done(req, ret);
+        infp_return_failure(infp_setattr_req->infp_req, NULL);
         talloc_free(infp_setattr_req);
     }
 }
@@ -1653,6 +1662,8 @@ einval:
     return EOK;
 
 error:
+    if(infp_setattr_req)
+        infp_return_failure(infp_setattr_req->infp_req, NULL);
     talloc_free(infp_setattr_req);
     return ret;
 }
@@ -1666,7 +1677,6 @@ struct infp_setuid_ctx {
 
 static void infp_do_user_set_uid_callback(void *ptr, int ldb_status, struct ldb_result *res)
 {
-    DBusMessage *reply;
     struct infp_setuid_ctx *infp_setuid_req = talloc_get_type(ptr, struct infp_setuid_ctx);
 
     /* Commit or cancel the transaction, based on the ldb_status */
@@ -1675,18 +1685,14 @@ static void infp_do_user_set_uid_callback(void *ptr, int ldb_status, struct ldb_
     /* Check the LDB result */
     if (ldb_status != LDB_SUCCESS) {
         DEBUG(0, ("Failed to store user uid to the sysdb\n"));
+        infp_return_failure(infp_setuid_req->infp_req, NULL);
         talloc_free(infp_setuid_req);
         return;
     }
 
     /* Send reply ack */
-    reply = dbus_message_new_method_return(infp_setuid_req->infp_req->req_message);
-    if(reply == NULL) {
-        talloc_free(infp_setuid_req);
-        return;
-    }
-    sbus_conn_send_reply(infp_setuid_req->infp_req->sconn, reply);
-    dbus_message_unref(reply);
+    infp_return_success(infp_setuid_req->infp_req);
+
     talloc_free(infp_setuid_req);
 }
 
@@ -1708,6 +1714,7 @@ static void infp_do_user_set_uid(struct sysdb_req *req, void *pvt)
     if (ret != EOK) {
         DEBUG(0, ("Could not invoke sysdb_set_user_attr"));
         sysdb_transaction_done(infp_setuid_req->sysdb_req, ret);
+        infp_return_failure(infp_setuid_req->infp_req, NULL);
         talloc_free(infp_setuid_req);
         return;
     }
@@ -1826,6 +1833,8 @@ einval:
     return EOK;
 
 error:
+    if(infp_setuid_req)
+        infp_return_failure(infp_setuid_req->infp_req, NULL);
     talloc_free(infp_setuid_req);
     return ret;
 }