From: pcarana <pc.moreno2099@gmail.com>
Date: Fri, 22 Mar 2019 16:08:42 +0000 (-0600)
Subject: Remove some TODOs and add some fixes.
X-Git-Tag: v0.0.2~52^2~14
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=34d620e0f1508ee4a20ceed12e05b981b923aece;p=thirdparty%2FFORT-validator.git

Remove some TODOs and add some fixes.

-Deltas creation is responsibility of vrps.c, so remove it from csv.c
-Remove unnecessary storage from heap (vrp structs, base DB an delta DB) and avoid some mem leaks.
-Change 'delta_resume' to 'delta_summary'.
-Handle error codes that were ignored.
---

diff --git a/src/csv.c b/src/csv.c
index e3fe7c6a..e464476b 100644
--- a/src/csv.c
+++ b/src/csv.c
@@ -8,11 +8,14 @@
 #include <time.h>
 #include <sys/stat.h>
 
+#include "array_list.h"
 #include "configuration.h"
 #include "address.h"
 #include "line_file.h"
 #include "vrps.h"
 
+ARRAY_LIST(vrplist, struct vrp)
+
 static int
 parse_asn(char *text, unsigned int *value)
 {
@@ -70,11 +73,11 @@ parse_prefix_length(char *text, unsigned int *value, int max_value)
 }
 
 static int
-add_vrp(char *line, struct delta *delta)
+add_vrp(char *line, struct vrplist *vrplist)
 {
 	struct ipv4_prefix prefixv4;
 	struct ipv6_prefix prefixv6;
-	struct vrp *vrp;
+	struct vrp vrp;
 	unsigned int asn, prefix_length, max_prefix_length;
 	int error;
 	bool isv4;
@@ -140,7 +143,8 @@ add_vrp(char *line, struct delta *delta)
 
 	if (prefix_length > max_prefix_length) {
 		error = -EINVAL;
-		err(error, "Prefix length is greater than max prefix length at line '%s'",
+		err(error,
+		    "Prefix length is greater than max prefix length at line '%s'",
 		    line);
 		goto error;
 	}
@@ -152,47 +156,32 @@ add_vrp(char *line, struct delta *delta)
 		vrp = create_vrp6(asn, prefixv6.addr, prefixv6.len,
 		    max_prefix_length);
 
-	if (vrp == NULL) {
-		error = -ENOMEM;
-		err(error, "Couldn't allocate VRP of line '%s'", line);
-		goto error;
-	}
-
-	/*
-	 * TODO (review) You seem to be leaking `vrp` on success. (The arraylist
-	 * stores a shallow copy, not the memory block you allocated.)
-	 *
-	 * In fact, `vrp_destroy()` is empty, so you're also leaking on error.
-	 *
-	 * You can avoid all these headaches by moving `vrp` to the stack.
-	 * (ie. remove `vrp`'s asterisk and patch emerging errors.)
-	 */
-	error = delta_add_vrp(delta, vrp);
-	if (error) {
-		vrp_destroy(vrp);
-		goto error;
-	}
-
-	return 0;
+	error =  vrplist_add(vrplist, &vrp);
 
 error:
 	free(line_copy);
 	return error;
 }
 
+static void
+clean_localvrps(struct vrp *vrp)
+{
+	/* Nothing special to clean, this is just a warning silencer */
+}
+
 static int
 load_vrps(struct line_file *lfile, bool is_update)
 {
-	struct delta *delta;
+	struct vrplist localvrps;
 	char *line;
 	int current_line;
 	int error;
 
 	/* Init delta */
-	delta = create_delta();
-	if (delta == NULL) {
-		warn("Couldn't allocate new delta");
-		return -ENOMEM;
+	error = vrplist_init(&localvrps);
+	if (error ) {
+		warn("Couldn't allocate new VRPs");
+		return error;
 	}
 	current_line = 1;
 	do {
@@ -200,7 +189,6 @@ load_vrps(struct line_file *lfile, bool is_update)
 		if (error) {
 			warn("Error reading line %d, stop processing file.",
 			    current_line);
-			delta_destroy(&delta);
 			goto end;
 		}
 		if (line == NULL) {
@@ -208,7 +196,7 @@ load_vrps(struct line_file *lfile, bool is_update)
 			goto persist;
 		}
 
-		error = add_vrp(line, delta);
+		error = add_vrp(line, &localvrps);
 		if (error)
 			warnx("Ignoring content at line %d.", current_line);
 
@@ -216,13 +204,13 @@ load_vrps(struct line_file *lfile, bool is_update)
 		free(line);
 	} while (true);
 persist:
-	/* TODO (review) delta is leaking. */
-	error = deltas_db_add_delta(delta);
+	error = deltas_db_create_delta(localvrps.array, localvrps.len);
 	if (error)
 		warnx("VRPs Delta couldn't be persisted");
 end:
 	if (line != NULL)
 		free(line);
+	vrplist_cleanup(&localvrps, clean_localvrps);
 	return error;
 }
 
diff --git a/src/vrps.c b/src/vrps.c
index 39e8374e..71889406 100644
--- a/src/vrps.c
+++ b/src/vrps.c
@@ -20,46 +20,39 @@ struct delta {
 	struct vrps vrps;
 };
 
-/* TODO (review) why pointers? */
-ARRAY_LIST(deltasdb, struct delta *)
+ARRAY_LIST(deltasdb, struct delta)
 
-/*
- * TODO (review) It seems you only have one instance of this.
- *
- * Best remove those asterisks; you don't need `base_db` and `deltas_db` to live
- * in the heap.
- */
 struct state {
 	/** The current valid ROAs, freshly loaded from the file */
-	struct delta *base_db;
+	struct delta base_db;
 	/** ROA changes over time */
-	struct deltasdb *deltas_db;
+	struct deltasdb deltas_db;
 	u_int32_t current_serial;
 	u_int16_t v0_session_id;
 	u_int16_t v1_session_id;
 	time_t last_modified_date;
 } state;
 
+static int
+delta_init(struct delta *delta)
+{
+	return vrps_init(&delta->vrps);
+}
+
 int
 deltas_db_init(void)
 {
 	int error, shift;
 
-	state.base_db = create_delta();
-	if (state.base_db == NULL){
-		err(-ENOMEM, "Delta base DB couldn't be initialized");
-		return -ENOMEM;
-	}
-
-	state.deltas_db = malloc(sizeof(struct deltasdb));
-	if (state.deltas_db == NULL){
-		err(-ENOMEM, "Deltas DB couldn't be allocated");
-		return -ENOMEM;
+	error = delta_init(&state.base_db);
+	if (error){
+		warnx("Delta base DB couldn't be initialized");
+		return error;
 	}
 
-	error = deltasdb_init(state.deltas_db);
+	error = deltasdb_init(&state.deltas_db);
 	if (error) {
-		err(error, "Deltas DB couldn't be initialized");
+		warnx("Deltas DB couldn't be initialized");
 		return error;
 	}
 
@@ -79,71 +72,39 @@ deltas_db_init(void)
 	return 0;
 }
 
-struct delta *
-create_delta(void)
-{
-	struct delta *result;
-
-	result = malloc(sizeof(struct delta));
-	if (result == NULL)
-		goto fail1;
-
-	if (vrps_init(&result->vrps) != 0)
-		goto fail2;
-
-	return result;
-fail2:
-	free(result);
-fail1:
-	return NULL;
-}
-
-static struct vrp *
-create_vrp (u_int32_t asn, u_int8_t prefix_length, u_int8_t max_prefix_length)
+static void
+init_vrp (struct vrp *vrp, u_int32_t asn, u_int8_t prefix_length,
+    u_int8_t max_prefix_length)
 {
-	struct vrp *result;
-
-	result = malloc(sizeof(struct vrp));
-	if (result == NULL)
-		return NULL;
-
-	result->asn = asn;
-	result->prefix_length = prefix_length;
-	result->max_prefix_length = max_prefix_length;
+	vrp->asn = asn;
+	vrp->prefix_length = prefix_length;
+	vrp->max_prefix_length = max_prefix_length;
 	/* Set as ANNOUNCEMENT by default */
-	result->flags = FLAG_ANNOUNCEMENT;
-
-	return result;
+	vrp->flags = FLAG_ANNOUNCEMENT;
 }
 
-struct vrp *
+struct vrp
 create_vrp4(u_int32_t asn, struct in_addr ipv4_prefix, u_int8_t prefix_length,
     u_int8_t max_prefix_length)
 {
-	struct vrp *result;
-
-	result = create_vrp(asn, prefix_length, max_prefix_length);
-	if (result == NULL)
-		return NULL;
+	struct vrp result;
 
-	result->ipv4_prefix = ipv4_prefix;
-	result->in_addr_len = INET_ADDRSTRLEN;
+	init_vrp(&result, asn, prefix_length, max_prefix_length);
+	result.ipv4_prefix = ipv4_prefix;
+	result.in_addr_len = INET_ADDRSTRLEN;
 
 	return result;
 }
 
-struct vrp *
+struct vrp
 create_vrp6(u_int32_t asn, struct in6_addr ipv6_prefix, u_int8_t prefix_length,
     u_int8_t max_prefix_length)
 {
-	struct vrp *result;
+	struct vrp result;
 
-	result = create_vrp(asn, prefix_length, max_prefix_length);
-	if (result == NULL)
-		return NULL;
-
-	result->ipv6_prefix = ipv6_prefix;
-	result->in_addr_len = INET6_ADDRSTRLEN;
+	init_vrp(&result, asn, prefix_length, max_prefix_length);
+	result.ipv6_prefix = ipv6_prefix;
+	result.in_addr_len = INET6_ADDRSTRLEN;
 
 	return result;
 }
@@ -180,18 +141,19 @@ vrp_is_new(struct vrps *base, struct vrp *vrp)
 	return vrp_locate(base, vrp) == NULL;
 }
 
-/*
- * TODO (review) I don't understand the name of this function.
- *
- * I think that you meant "summarize." I've never seen "resume" used
- * an spanish "resumen."
- */
-static struct delta *
-delta_resume(struct delta *delta)
+static int
+delta_add_vrp(struct delta *delta, struct vrp *vrp)
 {
-	struct delta *resume_delta;
+	return vrps_add(&delta->vrps, vrp);
+}
+
+static int
+delta_summary(struct delta *base_delta, struct delta *result)
+{
+	struct delta summary_delta;
 	struct vrps *base, *search_list;
 	struct vrp *cursor;
+	int error;
 
 	/*
 	 * Note: Don't fix this function yet.
@@ -199,81 +161,122 @@ delta_resume(struct delta *delta)
 	 * with a more efficient algorithm.
 	 */
 
-	/* TODO (review) check NULL */
-	resume_delta = create_delta();
-	resume_delta->serial = delta->serial;
+	error = delta_init(&summary_delta);
+	if (error)
+		return error;
+
+	summary_delta.serial = base_delta->serial;
 	/* First check for announcements */
-	base = &delta->vrps;
-	search_list = &state.base_db->vrps;
+	base = &base_delta->vrps;
+	search_list = &state.base_db.vrps;
 	ARRAYLIST_FOREACH(base, cursor)
 		if (vrp_is_new(search_list, cursor)) {
 			cursor->flags = FLAG_ANNOUNCEMENT;
-			/* TODO (review) check error code */
-			delta_add_vrp(resume_delta, cursor);
+			error = delta_add_vrp(&summary_delta, cursor);
+			if (error) {
+				warnx("Couldn't add announcement to summary");
+				return error;
+			}
 		}
 
 	/* Now for withdrawals */
-	base = &state.base_db->vrps;
-	search_list = &delta->vrps;
+	base = &state.base_db.vrps;
+	search_list = &base_delta->vrps;
 	ARRAYLIST_FOREACH(base, cursor)
 		if (vrp_is_new(search_list, cursor)) {
 			cursor->flags = FLAG_WITHDRAWAL;
-			/* TODO (review) check error code */
-			delta_add_vrp(resume_delta, cursor);
+			error = delta_add_vrp(&summary_delta, cursor);
+			if (error) {
+				warnx("Couldn't add withdrawal to summary");
+				return error;
+			}
 		}
 
-	return resume_delta;
+	memcpy(result, &summary_delta, sizeof(summary_delta));
+	return 0;
 }
 
-int
-deltas_db_add_delta(struct delta *delta)
+static int
+deltas_db_add_delta(struct delta delta)
 {
-	struct delta *resume;
+	struct delta summary;
 	int result;
 
 	result = 0;
-	delta->serial = state.current_serial;
+	delta.serial = state.current_serial;
 	/* Store only updates */
-	if (delta->serial != START_SERIAL) {
-		resume = delta_resume(delta);
-		result = deltasdb_add(state.deltas_db, &resume);
+	if (delta.serial != START_SERIAL) {
+		result = delta_summary(&delta, &summary);
+		if (result != 0) {
+			warnx("Error summarizing new delta");
+			return result;
+		}
+		result = deltasdb_add(&state.deltas_db, &summary);
 	}
 	/* Don't set the base in case of error */
-	if (result != 0)
+	if (result != 0) {
+		warnx("Error persisting new delta");
 		return result;
+	}
 
-	free(state.base_db);
 	state.base_db = delta;
 	state.current_serial++;
 	return result;
 }
 
+static void
+copy_vrps(struct vrp **dst, struct vrp *src, unsigned int len)
+{
+	struct vrp *tmp;
+	tmp = realloc(*dst, len * sizeof(struct vrp));
+	if (tmp == NULL) {
+		warnx("Couldn't copy VRPs");
+		return;
+	}
+	*dst = tmp;
+	*dst = memcpy(*dst, src, len * sizeof(struct vrp));
+}
+
 int
-delta_add_vrp(struct delta *delta, struct vrp *vrp)
+deltas_db_create_delta(struct vrp *array, unsigned int len)
 {
-	return vrps_add(&delta->vrps, vrp);
+	struct delta new_delta;
+	int error;
+
+	error = delta_init(&new_delta);
+	if (error) {
+		warn("New Delta couldn't be initialized");
+		return error;
+	}
+
+	copy_vrps(&new_delta.vrps.array, array, len);
+	new_delta.vrps.len = len;
+	new_delta.vrps.capacity = len * sizeof(struct vrp);
+
+	error = deltas_db_add_delta(new_delta);
+	if (error)
+		return error;
+
+	return 0;
 }
 
-void
+static void
 vrp_destroy(struct vrp *vrp)
 {
-	/* Nothing to free yet */
+	/* Didn't allocate something, so do nothing */
 }
 
 void
-delta_destroy(struct delta **delta)
+delta_destroy(struct delta *delta)
 {
-	/* Nothing else to free yet */
-	vrps_cleanup(&(*delta)->vrps, vrp_destroy);
-	free(*delta);
+	vrps_cleanup(&delta->vrps, vrp_destroy);
 }
 
 void
 deltas_db_destroy(void)
 {
-	deltasdb_cleanup(state.deltas_db, delta_destroy);
-	free(state.deltas_db);
-	free(state.base_db);
+	deltasdb_cleanup(&state.deltas_db, delta_destroy);
+	vrps_cleanup(&state.base_db.vrps, vrp_destroy);
 }
 
 /*
@@ -292,9 +295,9 @@ deltas_db_destroy(void)
 int
 deltas_db_status(u_int32_t *serial)
 {
-	struct delta **delta;
+	struct delta *delta;
 
-	if (state.base_db->vrps.len == 0)
+	if (state.base_db.vrps.len == 0)
 		return NO_DATA_AVAILABLE;
 
 	/* No serial to match, and there's data at DB */
@@ -302,12 +305,12 @@ deltas_db_status(u_int32_t *serial)
 		return DIFF_AVAILABLE;
 
 	/* Is the last version? */
-	if (*serial == state.base_db->serial)
+	if (*serial == state.base_db.serial)
 		return NO_DIFF;
 
 	/* Get the delta corresponding to the serial */
-	ARRAYLIST_FOREACH(state.deltas_db, delta) {
-		if ((*delta)->serial == *serial)
+	ARRAYLIST_FOREACH(&state.deltas_db, delta) {
+		if (delta->serial == *serial)
 			return DIFF_AVAILABLE;
 	}
 
@@ -328,19 +331,6 @@ add_vrps_filtered(struct vrps *dst, struct vrps *src)
 			vrps_add(dst, ptr);
 }
 
-static void
-copy_vrps(struct vrp **dst, struct vrp *src, unsigned int len)
-{
-	struct vrp *tmp;
-	tmp = realloc(*dst, len * sizeof(struct vrp));
-	if (tmp == NULL) {
-		err(-ENOMEM, "Couldn't copy VRPs");
-		return;
-	}
-	*dst = tmp;
-	*dst = memcpy(*dst, src, len * sizeof(struct vrp));
-}
-
 /*
  * Get the number of updates from serial START_SERIAL to END_SERIAL, set them
  * at RESULT.
@@ -352,18 +342,18 @@ unsigned int
 get_vrps_delta(u_int32_t *start_serial, u_int32_t *end_serial,
     struct vrp **result)
 {
-	struct delta **delta1;
+	struct delta *delta1;
 	struct vrps summary;
 
 	/* No data */
-	if (state.base_db->vrps.len == 0)
+	if (state.base_db.vrps.len == 0)
 		return 0;
 
 	/* NULL start? Send the last version, there's no need to iterate DB */
 	if (start_serial == NULL) {
-		copy_vrps(result, state.base_db->vrps.array,
-		    state.base_db->vrps.len);
-		return state.base_db->vrps.len;
+		copy_vrps(result, state.base_db.vrps.array,
+		    state.base_db.vrps.len);
+		return state.base_db.vrps.len;
 	}
 
 	/* Apparently nothing to return */
@@ -372,10 +362,10 @@ get_vrps_delta(u_int32_t *start_serial, u_int32_t *end_serial,
 
 	/* Get the delta corresponding to the serials */
 	vrps_init(&summary);
-	ARRAYLIST_FOREACH(state.deltas_db, delta1) {
-		if ((*delta1)->serial > *start_serial)
-			add_vrps_filtered(&summary, &(*delta1)->vrps);
-		if ((*delta1)->serial == *end_serial)
+	ARRAYLIST_FOREACH(&state.deltas_db, delta1) {
+		if (delta1->serial > *start_serial)
+			add_vrps_filtered(&summary, &delta1->vrps);
+		if (delta1->serial == *end_serial)
 			break;
 	}
 
diff --git a/src/vrps.h b/src/vrps.h
index 386a7945..d1be5ad3 100644
--- a/src/vrps.h
+++ b/src/vrps.h
@@ -21,22 +21,16 @@ struct vrp {
 	u_int8_t	flags;
 };
 
-struct delta;
-
 int deltas_db_init(void);
 
-struct delta *create_delta(void);
-struct vrp *create_vrp4(u_int32_t, struct in_addr, u_int8_t, u_int8_t);
-struct vrp *create_vrp6(u_int32_t, struct in6_addr, u_int8_t, u_int8_t);
+struct vrp create_vrp4(u_int32_t, struct in_addr, u_int8_t, u_int8_t);
+struct vrp create_vrp6(u_int32_t, struct in6_addr, u_int8_t, u_int8_t);
 
-int delta_add_vrp(struct delta *, struct vrp *);
-int deltas_db_add_delta(struct delta *);
+int deltas_db_create_delta(struct vrp *, unsigned int);
 int deltas_db_status(u_int32_t *);
 
 unsigned int get_vrps_delta(u_int32_t *, u_int32_t *, struct vrp **);
 
-void vrp_destroy(struct vrp *);
-void delta_destroy(struct delta **);
 void deltas_db_destroy(void);
 void set_vrps_last_modified_date(time_t);