rtr_server_SOURCES += address.c address.h
rtr_server_SOURCES += array_list.h
rtr_server_SOURCES += clients.c clients.h
-rtr_server_SOURCES += common.c common.h
+rtr_server_SOURCES += common.h
rtr_server_SOURCES += configuration.c configuration.h
rtr_server_SOURCES += csv.c csv.h
rtr_server_SOURCES += line_file.c line_file.h
{
int error;
+ /*
+ * TODO (review) I see this pattern often: You call `err()`, and then
+ * throw a pretty exception.
+ *
+ * `err()` does not return, so any cleanup that follows it is pointless.
+ *
+ * More importantly, returnless functions are bad practice,
+ * because they destroy the recovery potential of calling code.
+ * Use `warn()`/`warnx()` instead (and continue throwing cleanly).
+ */
if (str == NULL) {
err(-EINVAL, "Null string received, can't decode IPv6 prefix");
return -EINVAL;
{
unsigned long len;
+ /*
+ * TODO (review) You probably meant to use `errx()`, not `err()`.
+ *
+ * `err()` depends on a thread variable called `errno`. It makes no
+ * sense to call `err` when you know `errno` has not been set properly
+ * by some system function.
+ *
+ * If you haven't done it, see `man 3 err` and `man 3 errno`.
+ */
if (text == NULL) {
err(-EINVAL, "Null string received, can't decode prefix length");
return -EINVAL;
ARRAY_LIST(clientsdb, struct client)
+/* TODO (review) Does not need to live in the heap */
struct clientsdb *clients_db;
int
{
struct client *client;
+ /* TODO (review) `client` should be in the stack. */
client = malloc(sizeof(struct client));
if (client == NULL) {
err(-ENOMEM, "Couldn't allocate client");
if (client == NULL)
return create_client(fd, addr, rtr_version);
+ /*
+ * TODO (review) Your margin seems to be misconfigured; you're violating
+ * the line width often.
+ *
+ * Line width is 80 characters and tabs are 8.
+ *
+ * `Window > Preferences > C/C++ > Code Style > Formatter > New... > Ok`
+ * `Indentation > Tab size`
+ * `Line Wrapping > Maximum line width`
+ */
/*
* Isn't ready to handle distinct version on clients reconnection, but for
* now there's no problem since only 1 RTR version is supported (RVR v0).
+++ /dev/null
-#include "common.h"
-
-#include <stdlib.h>
-#include <string.h>
-
-char *
-str_clone(char const *original)
-{
- char *result;
- result = malloc(strlen(original) + 1);
- if (result != NULL)
- strcpy(result, original);
- return result;
-}
* does not.
*/
-char *str_clone(char const *);
-
#endif /* _SRC_COMMON_H_ */
DEFAULT_VRPS_LOCATION, &vrps_location);
if (error)
return error;
- config.vrps_location = str_clone(vrps_location);
+ /* TODO (review) check NULL */
+ config.vrps_location = strdup(vrps_location);
/*
* RFC 6810 and 8210:
return error;
}
- config.port = str_clone(service);
+ /* TODO (review) check NULL */
+ config.port = strdup(service);
return 0;
}
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);
int current_line;
int error;
+ /*
+ * TODO (review) Skipping the first line should not be hardcoded; if
+ * there's no header, you will ignore important information.
+ *
+ * Try checking whether you need to handle the line by way of the "AS"
+ * prefix instead.
+ */
/* First line is expected to be the header, ignore it */
current_line = 1;
error = lfile_read(lfile, &line);
return error;
}
/* Start the initial delta */
+ /* TODO (review) check NULL */
delta = create_delta();
do {
++current_line;
continue;
}
+ /* TODO (review) line is leaking. */
error = add_vrp(line, delta);
if (error) {
delta_destroy(&delta);
}
} while (true);
persist:
+ /* TODO (review) delta is leaking. */
error = deltas_db_add_delta(delta);
if (error)
err(error, "VRPs Delta couldn't be persisted");
int error;
location = config_get_vrps_location();
+
+ /*
+ * TODO (review) This is a needless bother; remove please.
+ *
+ * (Having a csv extension does not guarantee the file is CSV and
+ * vice versa.)
+ */
if (!location_has_extension(location, ".csv")) {
warn("%s isn't a CSV file", location);
error = -EINVAL;
goto end1;
}
+ /*
+ * TODO (review) This is an extremely heavy operation, and should not
+ * happen before you've confirmed that you need to read the file.
+ */
error = lfile_open(location, &lfile);
if (error)
goto end1; /* Error msg already printed. */
}
last_update = attr.st_mtim.tv_sec;
+ /*
+ * TODO (review) Time comparisons are particularly vulnerable to integer
+ * overflow. (Think about `time_t` being defined as a small integer,
+ * and `get_vrps_last_modified_date()` is the max value.)
+ *
+ * Usa `difftime()` instead. (`man 3 difftime`)
+ */
if (check_update && last_update <= get_vrps_last_modified_date())
goto end2;
return error;
}
+/* TODO (review) Should be `csv_parse_vrps_file(void)`. */
int
csv_parse_vrps_file()
{
}
}
+ /* TODO (review) I don't understand this comment; please ellaborate. */
/* TODO This will be overriden when reading from config file */
if (json_file == NULL) {
fprintf(stderr, "Missing flag '-f <file name>'\n");
goto end;
for (i = 0; i < len; i++) {
+ /*
+ * TODO (review) Why are you indexing `vrps`? You did not
+ * allocate an array.
+ */
if (vrps[i].in_addr_len == INET_ADDRSTRLEN)
error = send_ipv4_prefix_pdu(common, &vrps[i]);
else if (vrps[i].in_addr_len == INET6_ADDRSTRLEN)
struct thread_param *arg;
pthread_t thread;
+ /* TODO (review) 5 should be configurable. */
listen(server_fd, 5);
sizeof_client_addr = sizeof(client_addr);
#include "vrps.h"
+/* TODO (review) Can't find the meaning of "VRP" and "VRPS". Please explain. */
+
#include <stdbool.h>
#include <string.h>
#include "array_list.h"
struct vrps vrps;
};
+/* TODO (review) why pointers? */
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;
+ /** ROA changes over time */
struct deltasdb *deltas_db;
u_int32_t current_serial;
u_int16_t v0_session_id;
err(error, "Deltas DB couldn't be initialized");
return error;
}
+
+ /*
+ * TODO (review) This looks dangerous.
+ *
+ * 1. RTR server starts with serial 0
+ * 2. Router wants serial
+ * 3. RTR Server provides serial 0
+ * 4. RTR Server dies
+ * 5. RTR Server starts with serial 0, but the repository has changed
+ * by this point
+ *
+ * Can they realize that they are out of sync?
+ *
+ * Can't you use the current date as serial?
+ */
state.current_serial = START_SERIAL;
/* The downcast takes the LSBs */
+ /*
+ * TODO (review) The result of `time()` is unlikely to fit in a 16-bit
+ * integer.
+ *
+ * (Also: Integer overflow yields undefined behavior.)
+ */
state.v0_session_id = time(NULL);
/* Minus 1 to prevent same ID */
state.v1_session_id = state.v0_session_id - 1;
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)
{
struct vrps *base, *search_list;
struct vrp *cursor;
+ /*
+ * Note: Don't fix this function yet.
+ * I realize why you implemented it this way, and I'm trying to come up
+ * with a more efficient algorithm.
+ */
+
+ /* TODO (review) check NULL */
resume_delta = create_delta();
resume_delta->serial = delta->serial;
/* First check for announcements */
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);
}
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);
}
if (state.base_db->vrps.len == 0)
return NO_DATA_AVAILABLE;
+ /*
+ * TODO (review) `//`-style comments are somewhat controversial in that
+ * they weren't added until some late standard. From my reading of it,
+ * the OpenBSD style does not approve of them either.
+ *
+ * If you're using Eclipse, go to `Window > Preferences > C/C++ > Code
+ * Analysis`, and set `Coding Style > Line Comments` to `Error`.
+ *
+ * Then patch all the emerging red underlines.
+ */
// No serial to match, and there's data at DB
if (serial == NULL)
return DIFF_AVAILABLE;
projects / thirdparty / FORT-validator.git / commitdiff
