r10305: start implementing better error handling

changed the prioivate modules API
error string are now not spread over all
modules but are kept in  a single place.

This allows a better control of memory
and error reporting.
(This used to be commit 3fc676ac1d6f59d08bedbbd9377986154cf84ce4)

5 files changed, 62 insertions, 157 deletions

diff --git a/source4/lib/ldb/modules/ldb_map.c b/source4/lib/ldb/modules/ldb_map.c
index 93ae13ffc2..69e021b4ee 100644
--- a/source4/lib/ldb/modules/ldb_map.c
+++ b/source4/lib/ldb/modules/ldb_map.c
@@ -97,7 +97,6 @@ static const struct ldb_map_objectclass *map_find_objectclass_remote(struct ldb_
 
 struct map_private {
 	struct ldb_map_context context;
-	const char *last_err_string;
 };
 
 static struct ldb_map_context *map_get_privdat(struct ldb_module *module)
@@ -807,8 +806,7 @@ static int map_search_bytree_mp(struct ldb_module *module, const struct ldb_dn *
 	talloc_free(newattrs);
 
 	if (mpret == -1) {
-		struct map_private *map_private = module->private_data;
-		map_private->last_err_string = ldb_errstring(privdat->mapped_ldb);
+		ldb_set_errstring(module, talloc_strdup(module, ldb_errstring(privdat->mapped_ldb)));
 		return -1;
 	}
 
@@ -910,13 +908,12 @@ static int map_search(struct ldb_module *module, const struct ldb_dn *base,
 		       enum ldb_scope scope, const char *expression,
 		       const char * const *attrs, struct ldb_message ***res)
 {
-	struct map_private *map = module->private_data;
 	struct ldb_parse_tree *tree;
 	int ret;
 
 	tree = ldb_parse_tree(NULL, expression);
 	if (tree == NULL) {
-		map->last_err_string = "expression parse failed";
+		ldb_set_errstring(module, talloc_strdup(module, "expression parse failed"));
 		return -1;
 	}
 
@@ -1121,7 +1118,7 @@ static int map_add(struct ldb_module *module, const struct ldb_message *msg)
 	ldb_msg_add_string(module->ldb, fb, "isMapped", "TRUE");
 	ret = ldb_next_add_record(module, fb);
 	if (ret == -1) {
-		ldb_debug(module->ldb, LDB_DEBUG_WARNING, "Adding fallback record failed: %s", ldb_next_errstring(module));
+		ldb_debug(module->ldb, LDB_DEBUG_WARNING, "Adding fallback record failed: %s", ldb_errstring(module->ldb));
 		return -1;
 	}
 
@@ -1266,19 +1263,6 @@ static int map_end_trans(struct ldb_module *module, int status)
 	return ldb_next_end_trans(module, status);
 }
 
-/*
-  return extended error information
-*/
-static const char *map_errstring(struct ldb_module *module)
-{
-	struct map_private *map = module->private_data;
-	
-	if (map->last_err_string)
-		return map->last_err_string;
-
-	return ldb_next_errstring(module);
-}
-
 static const struct ldb_module_ops map_ops = {
 	.name              = "map",
 	.search            = map_search,
@@ -1288,8 +1272,7 @@ static const struct ldb_module_ops map_ops = {
 	.delete_record     = map_delete,
 	.rename_record     = map_rename,
 	.start_transaction = map_start_trans,
-	.end_transaction   = map_end_trans,
-	.errstring         = map_errstring
+	.end_transaction   = map_end_trans
 };
 
 static char *map_find_url(struct ldb_context *ldb, const char *name)
@@ -1354,8 +1337,6 @@ struct ldb_module *ldb_map_init(struct ldb_context *ldb, const struct ldb_map_at
 
 	talloc_free(url);
 
-	data->last_err_string = NULL;
-
 	/* Get list of attribute maps */
 	j = 0;
 	data->context.attribute_maps = NULL;
diff --git a/source4/lib/ldb/modules/rdn_name.c b/source4/lib/ldb/modules/rdn_name.c
index ed5400176c..c1a0c0852a 100644
--- a/source4/lib/ldb/modules/rdn_name.c
+++ b/source4/lib/ldb/modules/rdn_name.c
@@ -37,10 +37,6 @@
 #include "ldb/include/ldb_private.h"
 #include <time.h>
 
-struct private_data {
-	const char *error_string;
-};
-
 static int rdn_name_search(struct ldb_module *module, const struct ldb_dn *base,
 				  enum ldb_scope scope, const char *expression,
 				  const char * const *attrs, struct ldb_message ***res)
@@ -73,8 +69,6 @@ static struct ldb_message_element *rdn_name_find_attribute(const struct ldb_mess
 /* add_record: add crateTimestamp/modifyTimestamp attributes */
 static int rdn_name_add_record(struct ldb_module *module, const struct ldb_message *msg)
 {
-	struct private_data *data = (struct private_data *)module->private_data;
-
 	struct ldb_message *msg2;
 	struct ldb_message_element *attribute;
 	struct ldb_dn_component *rdn;
@@ -134,8 +128,11 @@ static int rdn_name_add_record(struct ldb_module *module, const struct ldb_messa
 			}
 		}
 		if (i == attribute->num_values) {
-			data->error_string = talloc_asprintf(data, "RDN mismatch on %s: %s", ldb_dn_linearize(msg2, msg2->dn), rdn->name);
-			ldb_debug(module->ldb, LDB_DEBUG_FATAL, "%s\n", data->error_string);
+			char *error_string = talloc_asprintf(module, "RDN mismatch on %s: %s", ldb_dn_linearize(msg2, msg2->dn), rdn->name);
+			if (error_string) {
+				ldb_set_errstring(module, error_string);
+				ldb_debug(module->ldb, LDB_DEBUG_FATAL, "%s\n", error_string);
+			}
 			talloc_free(msg2);
 			return -1;
 		}
@@ -229,23 +226,6 @@ static int rdn_end_trans(struct ldb_module *module, int status)
 	return ldb_next_end_trans(module, status);
 }
 
-/* return extended error information */
-static const char *rdn_name_errstring(struct ldb_module *module)
-{
-	struct private_data *data = (struct private_data *)module->private_data;
-
-	ldb_debug(module->ldb, LDB_DEBUG_TRACE, "rdn_name_errstring\n");
-	if (data->error_string) {
-		const char *error;
-
-		error = data->error_string;
-		data->error_string = NULL;
-		return error;
-	}
-
-	return ldb_next_errstring(module);
-}
-
 static int rdn_name_destructor(void *module_ctx)
 {
 	/* struct ldb_module *ctx = module_ctx; */
@@ -262,8 +242,7 @@ static const struct ldb_module_ops rdn_name_ops = {
 	.delete_record     = rdn_name_delete_record,
 	.rename_record     = rdn_name_rename_record,
 	.start_transaction = rdn_start_trans,
-	.end_transaction   = rdn_end_trans,
-	.errstring         = rdn_name_errstring
+	.end_transaction   = rdn_end_trans
 };
 
 
@@ -275,20 +254,12 @@ struct ldb_module *rdn_name_module_init(struct ldb_context *ldb, const char *opt
 #endif
 {
 	struct ldb_module *ctx;
-	struct private_data *data;
 
 	ctx = talloc(ldb, struct ldb_module);
 	if (!ctx)
 		return NULL;
 
-	data = talloc(ctx, struct private_data);
-	if (!data) {
-		talloc_free(ctx);
-		return NULL;
-	}
-
-	data->error_string = NULL;
-	ctx->private_data = data;
+	ctx->private_data = NULL;
 	ctx->ldb = ldb;
 	ctx->prev = ctx->next = NULL;
 	ctx->ops = &rdn_name_ops;
diff --git a/source4/lib/ldb/modules/schema.c b/source4/lib/ldb/modules/schema.c
index 9406d54ce7..1a2c2b8f97 100644
--- a/source4/lib/ldb/modules/schema.c
+++ b/source4/lib/ldb/modules/schema.c
@@ -34,6 +34,7 @@
 
 #include "includes.h"
 #include "ldb/include/ldb.h"
+#include "ldb/include/ldb_errors.h"
 #include "ldb/include/ldb_private.h"
 
 #define SCHEMA_FLAG_RESET	0
@@ -51,10 +52,6 @@
 	 check there's only one structrual class (or a chain of structural classes)
 */
 
-struct private_data {
-	const char *error_string;
-};
-
 struct schema_attribute {
 	int flags;
 	char *name;
@@ -189,8 +186,8 @@ static int add_attribute_uniq(void *mem_ctx, struct schema_attribute_list *list,
    recursively get parent objectlasses attributes */
 static int get_attr_list_recursive(struct ldb_module *module, struct schema_structures *schema_struct)
 {
-	struct private_data *data = (struct private_data *)module->private_data;
 	struct ldb_message **srch;
+	char *error_string;
 	int i, j;
 	int ret;
 
@@ -213,14 +210,20 @@ static int get_attr_list_recursive(struct ldb_module *module, struct schema_stru
 
 		if (ret <= 0) {
 			/* Schema DB Error: Error occurred retrieving Object Class Description */
-			ldb_debug(module->ldb, LDB_DEBUG_ERROR, "Error retrieving Objectclass %s.\n", schema_struct->objectclasses.attr[i].name);
-			data->error_string = "Internal error. Error retrieving schema objectclass";
+			error_string = talloc_asprintf(module, "Error retrieving Objectclass %s.\n", schema_struct->objectclasses.attr[i].name);
+			if (error_string) {
+				ldb_set_errstring(module, error_string);
+				ldb_debug(module->ldb, LDB_DEBUG_ERROR, error_string);
+			}
 			return -1;
 		}
 		if (ret > 1) {
 			/* Schema DB Error: Too Many Records */
-			ldb_debug(module->ldb, LDB_DEBUG_ERROR, "Too many records found retrieving Objectclass %s.\n", schema_struct->objectclasses.attr[i].name);
-			data->error_string = "Internal error. Too many records searching for schema objectclass";
+			error_string = talloc_asprintf(module, "Too many records found retrieving Objectclass %s.\n", schema_struct->objectclasses.attr[i].name);
+			if (error_string) {
+				ldb_set_errstring(module, error_string);
+				ldb_debug(module->ldb, LDB_DEBUG_ERROR, error_string);
+			}
 			return -1;
 		}
 
@@ -297,8 +300,8 @@ static int schema_search_bytree(struct ldb_module *module, const struct ldb_dn *
 /* add_record */
 static int schema_add_record(struct ldb_module *module, const struct ldb_message *msg)
 {
-	struct private_data *data = (struct private_data *)module->private_data;
 	struct schema_structures *entry_structs;
+	char *error_string;
 	unsigned int i;
 	int ret;
 
@@ -342,13 +345,15 @@ static int schema_add_record(struct ldb_module *module, const struct ldb_message
 					     entry_structs->required_attrs.attr[i].name);
 
 		if (attr == NULL) { /* not found */
-			ldb_debug(module->ldb, LDB_DEBUG_ERROR,
+			error_string = talloc_asprintf(module,
 				  "The required_attrs attribute %s is missing.\n",
 				  entry_structs->required_attrs.attr[i].name);
-
-			data->error_string = "Objectclass violation, a required attribute is missing";
+			if (error_string) {
+				ldb_set_errstring(module, error_string);
+				ldb_debug(module->ldb, LDB_DEBUG_ERROR, error_string);
+			}
 			talloc_free(entry_structs);
-			return -1;
+			return LDB_ERR_OBJECT_CLASS_VIOLATION;
 		}
 
 		/* mark the attribute as checked */
@@ -365,13 +370,15 @@ static int schema_add_record(struct ldb_module *module, const struct ldb_message
 						     entry_structs->entry_attrs.attr[i].name);
 
 			if (attr == NULL) { /* not found */
-				ldb_debug(module->ldb, LDB_DEBUG_ERROR,
+				error_string = talloc_asprintf(module,
 					  "The attribute %s is not referenced by any objectclass.\n",
 					  entry_structs->entry_attrs.attr[i].name);
-
-				data->error_string = "Objectclass violation, an invalid attribute name was found";
+				if (error_string) {
+					ldb_set_errstring(module, error_string);
+					ldb_debug(module->ldb, LDB_DEBUG_ERROR, error_string);
+				}
 				talloc_free(entry_structs);
-				return -1;
+				return LDB_ERR_OBJECT_CLASS_VIOLATION;
 			}
 		}
 	}
@@ -384,8 +391,8 @@ static int schema_add_record(struct ldb_module *module, const struct ldb_message
 /* modify_record */
 static int schema_modify_record(struct ldb_module *module, const struct ldb_message *msg)
 {
-	struct private_data *data = (struct private_data *)module->private_data;
 	struct schema_structures *entry_structs;
+	char *error_string;
 	unsigned int i;
 	int ret;
 
@@ -437,25 +444,30 @@ static int schema_modify_record(struct ldb_module *module, const struct ldb_mess
 					     entry_structs->required_attrs.attr[i].name);
 
 		if (attr == NULL) { /* not found */
-			ldb_debug(module->ldb, LDB_DEBUG_ERROR,
+			error_string = talloc_asprintf(module,
 				  "The required_attrs attribute %s is missing.\n",
 				  entry_structs->required_attrs.attr[i].name);
-
-			data->error_string = "Objectclass violation, a required attribute is missing";
+			if (error_string) {
+				ldb_set_errstring(module, error_string);
+				ldb_debug(module->ldb, LDB_DEBUG_ERROR, error_string);
+			}
 			talloc_free(entry_structs);
-			return -1;
+			return LDB_ERR_OBJECT_CLASS_VIOLATION;
 		}
 
 		/* check we are not trying to delete a required attribute */
 		/* TODO: consider multivalued attrs */
 		if ((attr->flags & SCHEMA_FLAG_MOD_DELETE) != 0) {
-			ldb_debug(module->ldb, LDB_DEBUG_ERROR,
+			error_string = talloc_asprintf(module,
 				  "Trying to delete the required attribute %s.\n",
 				  attr->name);
+			if (error_string) {
+				ldb_set_errstring(module, error_string);
+				ldb_debug(module->ldb, LDB_DEBUG_ERROR, error_string);
+			}
 
-			data->error_string = "Objectclass violation, a required attribute cannot be removed";
 			talloc_free(entry_structs);
-			return -1;
+			return LDB_ERR_OBJECT_CLASS_VIOLATION;
 		}
 
 		/* mark the attribute as checked */
@@ -472,13 +484,16 @@ static int schema_modify_record(struct ldb_module *module, const struct ldb_mess
 						     entry_structs->entry_attrs.attr[i].name);
 
 			if (attr == NULL) { /* not found */
-				ldb_debug(module->ldb, LDB_DEBUG_ERROR,
+				error_string = talloc_asprintf(module,
 					  "The attribute %s is not referenced by any objectclass.\n",
 					  entry_structs->entry_attrs.attr[i].name);
+				if (error_string) {
+					ldb_set_errstring(module, error_string);
+					ldb_debug(module->ldb, LDB_DEBUG_ERROR, error_string);
+				}
 
-				data->error_string = "Objectclass violation, an invalid attribute name was found";
 				talloc_free(entry_structs);
-				return -1;
+				return LDB_ERR_OBJECT_CLASS_VIOLATION;
 			}
 		}
 	}
@@ -509,22 +524,6 @@ static int schema_end_trans(struct ldb_module *module, int status) {
 	return ldb_next_end_trans(module, status);
 }
 
-/* return extended error information */
-static const char *schema_errstring(struct ldb_module *module)
-{
-	struct private_data *data = (struct private_data *)module->private_data;
-
-	if (data->error_string) {
-		const char *error;
-
-		error = data->error_string;
-		data->error_string = NULL;
-		return error;
-	}
-
-	return ldb_next_errstring(module);
-}
-
 static int schema_destructor(void *module_ctx)
 {
 /* 	struct ldb_module *ctx = module_ctx; */
@@ -541,8 +540,7 @@ static const struct ldb_module_ops schema_ops = {
 	.delete_record     = schema_delete_record,
 	.rename_record     = schema_rename_record,
 	.start_transaction = schema_start_trans,
-	.end_transaction   = schema_end_trans,
-	.errstring         = schema_errstring,
+	.end_transaction   = schema_end_trans
 };
 
 #ifdef HAVE_DLOPEN_DISABLED
@@ -552,21 +550,13 @@ struct ldb_module *schema_module_init(struct ldb_context *ldb, const char *optio
 #endif
 {
 	struct ldb_module *ctx;
-	struct private_data *data;
 
 	ctx = talloc(ldb, struct ldb_module);
 	if (!ctx) {
 		return NULL;
 	}
 
-	data = talloc(ctx, struct private_data);
-	if (data == NULL) {
-		talloc_free(ctx);
-		return NULL;
-	}
-
-	data->error_string = NULL;
-	ctx->private_data = data;
+	ctx->private_data = NULL;
 	ctx->ldb = ldb;
 	ctx->prev = ctx->next = NULL;
 	ctx->ops = &schema_ops;
diff --git a/source4/lib/ldb/modules/skel.c b/source4/lib/ldb/modules/skel.c
index 37c0417c64..701128cd27 100644
--- a/source4/lib/ldb/modules/skel.c
+++ b/source4/lib/ldb/modules/skel.c
@@ -38,7 +38,7 @@
 
 struct private_data {
 
-	char *errstring;
+	char *some_private_data;
 };
 
 /* search */
@@ -85,18 +85,12 @@ static int skel_end_trans(struct ldb_module *module, int status)
 	return ldb_next_end_trans(module, status);
 }
 
-/* return extended error information */
-static const char *skel_errstring(struct ldb_module *module)
-{
-	return ldb_next_errstring(module);
-}
-
 static int skel_destructor(void *module_ctx)
 {
 	struct ldb_module *ctx = talloc_get_type(module_ctx, struct ldb_module);
 	struct private_data *data = talloc_get_type(ctx->private_data, struct private_data);
 	/* put your clean-up functions here */
-	if (data->errstring) talloc_free(data->errstring);
+	if (data->some_private_data) talloc_free(data->some_private_data);
 	return 0;
 }
 
@@ -110,7 +104,6 @@ static const struct ldb_module_ops skel_ops = {
 	.rename_record	   = skel_rename_record,
 	.start_transaction = skel_start_trans,
 	.end_transaction   = skel_end_trans,
-	.errstring	   = skel_errstring
 };
 
 #ifdef HAVE_DLOPEN_DISABLED
@@ -132,7 +125,7 @@ struct ldb_module *skel_module_init(struct ldb_context *ldb, const char *options
 		return NULL;
 	}
 
-	data->errstring = NULL;
+	data->some_private_data = NULL;
 	ctx->private_data = data;
 
 	ctx->ldb = ldb;
diff --git a/source4/lib/ldb/modules/timestamps.c b/source4/lib/ldb/modules/timestamps.c
index 6687b1929d..dc91937f85 100644
--- a/source4/lib/ldb/modules/timestamps.c
+++ b/source4/lib/ldb/modules/timestamps.c
@@ -37,10 +37,6 @@
 #include "ldb/include/ldb_private.h"
 #include <time.h>
 
-struct private_data {
-	const char *error_string;
-};
-
 static int timestamps_search(struct ldb_module *module, const struct ldb_dn *base,
 				  enum ldb_scope scope, const char *expression,
 				  const char * const *attrs, struct ldb_message ***res)
@@ -227,23 +223,6 @@ static int timestamps_end_trans(struct ldb_module *module, int status)
 	return ldb_next_end_trans(module, status);
 }
 
-/* return extended error information */
-static const char *timestamps_errstring(struct ldb_module *module)
-{
-	struct private_data *data = (struct private_data *)module->private_data;
-
-	ldb_debug(module->ldb, LDB_DEBUG_TRACE, "timestamps_errstring\n");
-	if (data->error_string) {
-		const char *error;
-
-		error = data->error_string;
-		data->error_string = NULL;
-		return error;
-	}
-
-	return ldb_next_errstring(module);
-}
-
 static int timestamps_destructor(void *module_ctx)
 {
 	/* struct ldb_module *ctx = module_ctx; */
@@ -260,8 +239,7 @@ static const struct ldb_module_ops timestamps_ops = {
 	.delete_record     = timestamps_delete_record,
 	.rename_record     = timestamps_rename_record,
 	.start_transaction = timestamps_start_trans,
-	.end_transaction   = timestamps_end_trans,
-	.errstring         = timestamps_errstring
+	.end_transaction   = timestamps_end_trans
 };
 
 
@@ -273,20 +251,12 @@ struct ldb_module *timestamps_module_init(struct ldb_context *ldb, const char *o
 #endif
 {
 	struct ldb_module *ctx;
-	struct private_data *data;
 
 	ctx = talloc(ldb, struct ldb_module);
 	if (!ctx)
 		return NULL;
 
-	data = talloc(ctx, struct private_data);
-	if (!data) {
-		talloc_free(ctx);
-		return NULL;
-	}
-
-	data->error_string = NULL;
-	ctx->private_data = data;
+	ctx->private_data = NULL;
 	ctx->ldb = ldb;
 	ctx->prev = ctx->next = NULL;
 	ctx->ops = &timestamps_ops;