dbfp: improve error handling

Get rid of the complicated dbfp_status structure and use plain old
return codes.  This leads to a cleaner interface and fixes the problem
with unsufficient error handling introduced with 1648e81.  The drawback
is that the details of curl and expat errors are hidden from the user,
but this can be adressed later.

3 files changed, 140 insertions, 130 deletions

diff --git a/dbfp.c b/dbfp.c
index d822f20..8aed661 100644
--- a/dbfp.c
+++ b/dbfp.c
@@ -18,23 +18,26 @@
 #define DBFP_URL_LEN 8000
 
 struct parse_data {
-	struct dbfp_status *status;
+	int error;
 	void *data;
 };
 
-struct loc_data {
+struct location_data {
 	size_t n;
-	struct dbfp_location *locs;
+	struct dbfp_location *locations;
 };
 
 static int dbfp_parse(void *contents, size_t len, size_t n, void *data);
-static void dbfp_request(struct dbfp *dbfp, char *service, char *query,
-		XML_Parser parser, struct dbfp_status *status);
+static int dbfp_request(struct dbfp *dbfp, char *service, char *query,
+		XML_Parser parser);
 static int dbfp_url(struct dbfp *dbfp, char *service, char *query, char **url);
 
-static void handle_loc(struct loc_data *ld, const char **atts);
-static void loc_ele_start(void *data, const char *name, const char **atts);
-static void loc_ele_end(void *data, const char *name);
+static int check_ele_error(const char *name, const char **atts);
+
+static int add_location(struct location_data *ld, const char **atts,
+		size_t idx_name, size_t idx_id);
+static void location_start(void *data, const char *name, const char **atts);
+static void location_end(void *data, const char *name);
 
 int dbfp_init(struct dbfp *dbfp, char *key)
 {
@@ -60,66 +63,55 @@ void dbfp_close(struct dbfp *dbfp)
 	free(dbfp->key);
 }
 
-struct dbfp_status dbfp_query_location_name(struct dbfp *dbfp, char *input,
-		size_t *n, struct dbfp_location **out)
+int dbfp_query_location_name(struct dbfp *dbfp, char *input,
+		size_t *n, struct dbfp_location **locations)
 {
-	struct dbfp_status status = { 0, 0, 0, 0, 0 };
+	int err;
 	char *query = NULL;
-	int len;
-	struct parse_data pd;
-	struct loc_data ld;
 	XML_Parser parser = NULL;
+	int len;
+	struct location_data ld = { .n = 0, .locations = NULL };
+	struct parse_data pd = { .error = 0, .data = &ld };
 
-	if (!dbfp || !input) {
-		status.error = 1;
-		status.run_error = EINVAL;
-		goto cleanup;
-	}
+	if (!dbfp || !input || !n || !locations)
+		return EINVAL;
+
+	*n = 0;
+	*locations = NULL;
 
 	/*
 	 * TODO(robin.krahl):  improve query building and implement HTML
 	 * encoding.
 	 */
 	query = malloc(DBFP_URL_LEN);
-	if (!query) {
-		status.error = 1;
-		status.run_error = ENOMEM;
-		goto cleanup;
-	}
+	if (!query)
+		return ENOMEM;
 	len = snprintf(query, DBFP_URL_LEN, "input=%s", input);
 	if (len < 0 || len >= DBFP_URL_LEN) {
-		status.error = 1;
-		status.run_error = -1;
+		err = DBFP_ERROR_FORMAT;
 		goto cleanup;
 	}
 
-	pd.status = &status;
-	pd.data = &ld;
-	ld.n = 0;
-	ld.locs = NULL;
-
 	parser = XML_ParserCreateNS(NULL, '\0');
 	XML_SetUserData(parser, &pd);
-	XML_SetElementHandler(parser, loc_ele_start, loc_ele_end);
-
-	dbfp_request(dbfp, "location.name", query, parser, &status);
-
-	if (!status.error) {
-		if (n)
-			*n = ld.n;
-		if (out)
-			*out = ld.locs;
-		else
-			free(ld.locs);
-	} else {
-		free(ld.locs);
-	}
+	XML_SetElementHandler(parser, location_start, location_end);
+
+	err = dbfp_request(dbfp, "location.name", query, parser);
+	if (pd.error)
+		err = pd.error;
+	if (err)
+		goto cleanup;
+
+	*n = ld.n;
+	*locations = ld.locations;
 
 cleanup:
+	if (err)
+		free(ld.locations);
 	XML_ParserFree(parser);
 	free(query);
 
-	return status;
+	return err;
 }
 
 static int dbfp_parse(void *contents, size_t len, size_t n, void *data)
@@ -130,28 +122,26 @@ static int dbfp_parse(void *contents, size_t len, size_t n, void *data)
 
 	pd = (struct parse_data *)XML_GetUserData(parser);
 
-	if (!pd->status->parse_error && !XML_Parse(parser, contents, size, 0)) {
-		pd->status->error = 1;
-		pd->status->parse_error = XML_GetErrorCode(parser);
-	}
+	if (!pd->error && !XML_Parse(parser, contents, size, 0))
+		pd->error = DBFP_ERROR_PARSE;
 
 	return size;
 }
 
-static void dbfp_request(struct dbfp *dbfp, char *service, char *query,
-		XML_Parser parser, struct dbfp_status *status)
+static int dbfp_request(struct dbfp *dbfp, char *service, char *query,
+		XML_Parser parser)
 {
-	char *url;
+	int err = 0;
+	char *url = NULL;
 	CURL *curl = NULL;
 
-	status->run_error = dbfp_url(dbfp, service, query, &url);
-	if (status->run_error)
+	err = dbfp_url(dbfp, service, query, &url);
+	if (err)
 		goto cleanup;
 
 	curl = curl_easy_init();
 	if (!curl) {
-		status->error = 1;
-		status->run_error = -1;
+		err = DBFP_ERROR_CURL;
 		goto cleanup;
 	}
 
@@ -159,16 +149,15 @@ static void dbfp_request(struct dbfp *dbfp, char *service, char *query,
 	curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, dbfp_parse);
 	curl_easy_setopt(curl, CURLOPT_WRITEDATA, (void *)parser);
 
-	status->curl_error = curl_easy_perform(curl);
-
-	if (status->parse_error)
-		fprintf(stderr, "Parse error %d: %s\n", status->parse_error,
-				XML_ErrorString(status->parse_error));
+	if (curl_easy_perform(curl) != CURLE_OK)
+		err = DBFP_ERROR_CURL;
 
 cleanup:
 	if (curl)
 		curl_easy_cleanup(curl);
 	free(url);
+
+	return err;
 }
 
 static int dbfp_url(struct dbfp *dbfp, char *service, char *query, char **url)
@@ -184,61 +173,90 @@ static int dbfp_url(struct dbfp *dbfp, char *service, char *query, char **url)
 	if (len < 0 || len >= DBFP_URL_LEN) {
 		free(*url);
 		*url = NULL;
-		return -1;
+		return DBFP_ERROR_FORMAT;
 	}
 
 	return 0;
 }
 
-static void handle_loc(struct loc_data *ld, const char **atts)
+static int check_ele_error(const char *name, const char **atts)
 {
-	char *name = NULL;
-	char *id = NULL;
-	size_t i;
+	if (strcmp(name, "Error") == 0) {
+		return DBFP_ERROR_API;
+	}
 
-	/*
-	 * TODO(robin.krahl):  fix error handling for the cases that a)
-	 * name or id is not set and b) realloc fails.
-	 */
+	return 0;
+}
 
-	for (i = 0; atts[i]; i += 2) {
-		if (strcmp(atts[i], "name") == 0)
-			name = strdup(atts[i + 1]);
-		else if (strcmp(atts[i], "id") == 0)
-			id = strdup(atts[i + 1]);
+static int add_location(struct location_data *ld, const char **atts,
+		size_t idx_name, size_t idx_id)
+{
+	int err = 0;
+	char *val_name = NULL;
+	char *val_id = NULL;
+
+	val_name = strdup(atts[idx_name]);
+	val_id = strdup(atts[idx_id]);
+	if (!val_name || !val_id) {
+		err = ENOMEM;
+		goto cleanup;
 	}
 
-	if (name && id) {
-		ld->locs = realloc(ld->locs, (ld->n + 1) * sizeof(*ld->locs));
-		if (ld->locs) {
-			ld->n++;
-			ld->locs[ld->n - 1].name = name;
-			ld->locs[ld->n - 1].id = id;
-		}
-	} else {
-		free(name);
-		free(id);
+	ld->locations = realloc(ld->locations,
+			(ld->n + 1) * sizeof(*ld->locations));
+	if (!ld->locations) {
+		err = ENOMEM;
+		goto cleanup;
 	}
+
+	ld->n++;
+	ld->locations[ld->n - 1].name = val_name;
+	ld->locations[ld->n - 1].id = val_id;
+
+cleanup:
+	if (err) {
+		free(val_name);
+		free(val_id);
+	}
+
+	return err;
 }
 
-static void loc_ele_start(void *data, const char *name, const char **atts)
+static void location_start(void *data, const char *name, const char **atts)
 {
+	int err = 0;
 	struct parse_data *pd = (struct parse_data *)data;
-	struct loc_data *ld = (struct loc_data *)pd->data;
+	struct location_data *ld = (struct location_data *)pd->data;
+	size_t idx_name = 0;
+	size_t idx_id = 0;
 	size_t i;
 
-	if (strcmp(name, "Error") == 0) {
-		for (i = 0; atts[i]; i += 2) {
-			if (strcmp(atts[i], "code")) {
-				fprintf(stderr, "err code %s\n", atts[i + 1]);
-				break;
-			}
-		}
-	} else if (strcmp(name, "StopLocation") == 0) {
-		handle_loc(ld, atts);
+	err = check_ele_error(name, atts);
+	if (err)
+		goto cleanup;
+
+	if (strcmp(name, "StopLocation") != 0)
+		goto cleanup;
+
+	for (i = 0; atts[i]; i += 2) {
+		if (strcmp(atts[i], "name") == 0)
+			idx_name = i + 1;
+		else if (strcmp(atts[i], "id") == 0)
+			idx_id = i + 1;
 	}
+
+	if (!idx_name || !idx_id) {
+		err = DBFP_ERROR_STRUCTURE;
+		goto cleanup;
+	}
+
+	err = add_location(ld, atts, idx_name, idx_id);
+
+cleanup:
+	if (err)
+		pd->error = err;
 }
 
-static void loc_ele_end(void *data, const char *name)
+static void location_end(void *data, const char *name)
 {
 }
diff --git a/dbfp.h b/dbfp.h
index 7c786c7..ca92ed1 100644
--- a/dbfp.h
+++ b/dbfp.h
@@ -9,16 +9,16 @@
 
 #include <stddef.h>
 
-struct dbfp {
-	char *key;
+enum dbfp_error {
+	DBFP_ERROR_CURL = -1,
+	DBFP_ERROR_FORMAT = -2,
+	DBFP_ERROR_STRUCTURE = -3,
+	DBFP_ERROR_API = -4,
+	DBFP_ERROR_PARSE = -5
 };
 
-struct dbfp_status {
-	int error;
-	int run_error;
-	int parse_error;
-	int curl_error;
-	int api_error;
+struct dbfp {
+	char *key;
 };
 
 struct dbfp_location {
@@ -29,7 +29,7 @@ struct dbfp_location {
 int dbfp_init(struct dbfp *dbfp, char *key);
 void dbfp_close(struct dbfp *dbfp);
 
-struct dbfp_status dbfp_query_location_name(struct dbfp *dbfp, char *input,
-		size_t *n, struct dbfp_location **out);
+int dbfp_query_location_name(struct dbfp *dbfp, char *input,
+		size_t *n, struct dbfp_location **locations);
 
 #endif  /* DBFP_H_ */
diff --git a/dbfp_check.c b/dbfp_check.c
index 067918f..2c048d8 100644
--- a/dbfp_check.c
+++ b/dbfp_check.c
@@ -26,50 +26,42 @@ END_TEST
 
 START_TEST(test_dbfp_access)
 {
-	struct dbfp dbfp;
-	struct dbfp_status status;
 	int err;
+	struct dbfp dbfp;
+	struct dbfp_location *locations = NULL;
+	size_t n;
 
 	err = dbfp_init(&dbfp, api_key);
 	ck_assert_int_eq(err, 0);
 
-	status = dbfp_query_location_name(&dbfp, "", NULL, NULL);
-	ck_assert_int_eq(status.error, 0);
-	ck_assert_int_eq(status.run_error, 0);
-	ck_assert_int_eq(status.parse_error, 0);
-	ck_assert_int_eq(status.curl_error, 0);
-	ck_assert_int_eq(status.api_error, 0);
+	err = dbfp_query_location_name(&dbfp, "", &n, &locations);
+	ck_assert_int_eq(err, 0);
 
+	free(locations);
 	dbfp_close(&dbfp);
 }
 END_TEST
 
 START_TEST(test_dbfp_location_simple)
 {
-	struct dbfp dbfp;
-	struct dbfp_status status;
 	int err;
+	struct dbfp dbfp;
+	struct dbfp_location *locations = NULL;
 	size_t n;
-	struct dbfp_location *locs = NULL;
 
 	err = dbfp_init(&dbfp, api_key);
 	ck_assert_int_eq(err, 0);
 
-	status = dbfp_query_location_name(&dbfp, "freiburg", &n, &locs);
-	ck_assert_int_eq(status.error, 0);
-	ck_assert_int_eq(status.run_error, 0);
-	ck_assert_int_eq(status.parse_error, 0);
-	ck_assert_int_eq(status.curl_error, 0);
-	ck_assert_int_eq(status.api_error, 0);
+	err = dbfp_query_location_name(&dbfp, "freiburg", &n, &locations);
+	ck_assert_int_eq(err, 0);
 
 	ck_assert_int_gt(n, 0);
-	ck_assert_ptr_ne(locs, NULL);
-
-	ck_assert_str_eq(locs[0].name, "Freiburg(Breisgau) Hbf");
-	ck_assert_str_eq(locs[0].id, "008000107");
+	ck_assert_ptr_ne(locations, NULL);
 
-	free(locs);
+	ck_assert_str_eq(locations[0].name, "Freiburg(Breisgau) Hbf");
+	ck_assert_str_eq(locations[0].id, "008000107");
 
+	free(locations);
 	dbfp_close(&dbfp);
 }
 END_TEST