From 243002f2828fca5996a720e7785e07cf482a6a46 Mon Sep 17 00:00:00 2001
From: Jef Roosens <roosensjef@gmail.com>
Date: Sat, 14 Jan 2023 18:44:33 +0100
Subject: [PATCH 1/2] fix(cron): ensure valid day values; some other stuff

---
 Makefile                   |   2 +-
 src/cron/c/expression.c    |   7 +-
 src/cron/c/expression.h    |   8 ++-
 src/cron/c/parse.c         | 130 +++++++++++++++++++++++++++----------
 src/cron/expression.v      |  11 ++--
 src/cron/expression_test.v |   2 +-
 6 files changed, 112 insertions(+), 48 deletions(-)

diff --git a/Makefile b/Makefile
index c71ff1f..3571d30 100644
--- a/Makefile
+++ b/Makefile
@@ -1,6 +1,6 @@
 # =====CONFIG=====
 SRC_DIR := src
-SOURCES != find '$(SRC_DIR)' -iname '*.v'
+SOURCES != find '$(SRC_DIR)' -\( -iname '*.v' -or -iname '*.h' -or -iname '*.c' -\)
 
 V_PATH ?= v
 V := $(V_PATH) -showcc -gc boehm -W -d use_openssl -skip-unused
diff --git a/src/cron/c/expression.c b/src/cron/c/expression.c
index ed0306f..0051e49 100644
--- a/src/cron/c/expression.c
+++ b/src/cron/c/expression.c
@@ -84,11 +84,6 @@ int ce_next(cron_simple_time *out, cron_expression *ce, cron_simple_time *ref) {
 
 		while (out->day > month_days[ce->months[month_index % ce->month_count] - 1]) {
 			month_index++;
-
-			// TODO find out if this can happen
-			if (month_index == 2 * ce->month_count) {
-				return 1;
-			}
 		}
 	}
 
@@ -108,7 +103,7 @@ int ce_next_from_now(cron_simple_time *out, cron_expression *ce) {
 	struct tm gm;
 	gmtime_r(&t, &gm);
 
-	struct cron_simple_time ref = {
+	cron_simple_time ref = {
 		.year = gm.tm_year,
 		// tm_mon goes from 0 to 11
 		.month = gm.tm_mon + 1,
diff --git a/src/cron/c/expression.h b/src/cron/c/expression.h
index c9441f6..1c86436 100644
--- a/src/cron/c/expression.h
+++ b/src/cron/c/expression.h
@@ -1,3 +1,6 @@
+#ifndef VIETER_CRON
+#define VIETER_CRON
+
 #include <time.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -7,7 +10,8 @@ typedef enum cron_parse_error {
 	cron_parse_ok = 0,
 	cron_parse_invalid_expression = 1,
 	cron_parse_invalid_number = 2,
-	cron_parse_out_of_range = 3
+	cron_parse_out_of_range = 3,
+	cron_parse_too_many_parts = 4
 } cron_parse_error;
 
 typedef struct cron_expression {
@@ -38,3 +42,5 @@ int cron_ce_next(cron_simple_time *out, cron_expression *ce, cron_simple_time *r
 int cron_ce_next_from_now(cron_simple_time *out, cron_expression *ce);
 
 enum cron_parse_error cron_ce_parse_expression(cron_expression *out, char *s);
+
+#endif
diff --git a/src/cron/c/parse.c b/src/cron/c/parse.c
index a577c6d..f54b818 100644
--- a/src/cron/c/parse.c
+++ b/src/cron/c/parse.c
@@ -1,21 +1,28 @@
 #include "expression.h"
 
+const uint8_t month_days[] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
+
 // Allowed value ranges for the minute, hour, day and month field
 const uint8_t min[4] = {0, 0, 1, 1};
 const uint8_t max[4] = {59, 23, 31, 12};
 
-// Convert a string a uint8_t value by parsing it using atoi and checking
+const uint8_t min_parts = 2;
+const uint8_t max_parts = 4;
+
+// Convert a string into a uint8_t value by parsing it using atoi and checking
 // whether it's contained within the given range
 #define SAFE_ATOI(v,s,min,max) \
 	int _##v = atoi(s); \
 	if ((_##v) == 0 && strcmp((s), "0") != 0) { \
 		return cron_parse_invalid_number; \
 	} \
-	if (v < (min) || v > (max)) { \
+	if (((_##v) < (min)) || ((_##v) > (max))) { \
 		return cron_parse_out_of_range; \
 	} \
 	v = (uint8_t) (_##v);
 
+#define MAX(x, y) (((x) > (y)) ? (x) : (y))
+
 /**
  * Given a range expression, produce a bit field defining what numbers in the
  * min-max range the expression represents. Bit 0 (starting from the
@@ -32,7 +39,7 @@ const uint8_t max[4] = {59, 23, 31, 12};
  * - a/c
  * - a-b/c
  */
-enum cron_parse_error ce_parse_range(uint64_t *out, char *s, uint8_t min, uint8_t max) {
+cron_parse_error ce_parse_range(uint64_t *out, char *s, uint8_t min, uint8_t max) {
 	// The * expression means "every possible value"
 	if (s[0] == '*') {
 		// A '*' is only valid on its own
@@ -98,7 +105,7 @@ enum cron_parse_error ce_parse_range(uint64_t *out, char *s, uint8_t min, uint8_
  * min-max range the part represents. A part consists of one or more range
  * expressions, separated by commas.
  */
-enum cron_parse_error ce_parse_part(uint64_t *out, char *s, uint8_t min, uint8_t max) {
+cron_parse_error ce_parse_part(uint64_t *out, char *s, uint8_t min, uint8_t max) {
 	*out = 0;
 
 	char *next;
@@ -175,28 +182,30 @@ enum cron_parse_error ce_parse_expression(cron_expression *out, char *s) {
 	s = strdup(s);
 	char *orig_s = s;
 
-	uint8_t part_count = 0;
-
-	char *next;
 	enum cron_parse_error res = cron_parse_ok;
-	uint64_t bfs[4];
+	uint64_t bfs[max_parts];
+
+	// First we divide the input string into its parts, divided by spaces.
+	// Each part is delimited by a NULL byte.
+	uint8_t part_count = 0;
+	char *parts[max_parts];
+	char *next;
 
 	// Skip leading spaces
-	while (s[0] == ' ') {
-		s++;
+	size_t offset = 0;
+
+	while (s[offset] == ' ') {
+		offset++;
 	}
 
-	while (part_count < 4 && ((next = strchr(s, ' ')) != NULL)) {
+	s += offset;
+
+	while (part_count < max_parts && ((next = strchr(s, ' ')) != NULL)) {
 		next[0] = '\0';
-		res = ce_parse_part(&bfs[part_count], s, min[part_count], max[part_count]);
-
-		if (res != cron_parse_ok) {
-			goto end;
-		}
-
-		size_t offset = 1;
+		parts[part_count] = s;
 
 		// Skip multiple spaces
+		offset = 1;
 		while (next[offset] == ' ') {
 			offset++;
 		}
@@ -205,34 +214,87 @@ enum cron_parse_error ce_parse_expression(cron_expression *out, char *s) {
 		part_count++;
 	}
 
-	// Parse final trailing part
-	if (part_count < 4 && s[0] != '\0') {
-		res = ce_parse_part(&bfs[part_count], s, min[part_count], max[part_count]);
+	// The loop exited because we already have 4 parts, yet there's still at
+	// least one more part that follows.
+	if (next != NULL) {
+		res = cron_parse_too_many_parts;
+	} else if (s[0] != '\0') {
+		// There's one more excessive trailing part
+		if (part_count == max_parts) {
+			res = cron_parse_too_many_parts;
+			goto end;
+		}
+
+		parts[part_count] = s;
+		part_count++;
+	}
+
+
+	// We now parse the parts in reverse. This is because the month part
+	// determines the maximum value of the day part.
+	
+	uint64_t bit_field = 0;
+	
+	// Months
+	if (part_count >= 4) {
+		res = ce_parse_part(&bit_field, parts[3], min[3], max[3]);
 
 		if (res != cron_parse_ok) {
 			goto end;
 		}
 
-		part_count++;
+		out->month_count = bf_to_nums(&out->months, bit_field, min[3], max[3]);
+	}
+	// If months aren't provided, they're replaced with a *
+	else {
+		out->month_count = bf_to_nums(&out->months, ~0, min[3], max[3]);
 	}
 
-	// At least two parts need to be provided
-	if (part_count < 2) {
-		res = cron_parse_invalid_expression;
+	// Determine what the largest allowed day value is, given the months
+	uint8_t max_day_value = 0;
+
+	for (uint8_t i = 0; i < out->month_count; i++) {
+		max_day_value = MAX(max_day_value, month_days[out->months[i] - 1]);
+	}
+	
+	// Days
+	if (part_count >= 3) {
+		bit_field = 0;
+
+		res = ce_parse_part(&bit_field, parts[2], min[2], max_day_value);
+
+		if (res != cron_parse_ok) {
+			goto end;
+		}
+
+		out->day_count = bf_to_nums(&out->days, bit_field, min[2], max_day_value);
+	}
+	// If days aren't provided, they're replaced with a *
+	else {
+		out->day_count = bf_to_nums(&out->days, ~0, min[2], max_day_value);
+	}
+
+	// Hours
+	bit_field = 0;
+
+	res = ce_parse_part(&bit_field, parts[1], min[1], max[1]);
+
+	if (res != cron_parse_ok) {
 		goto end;
 	}
 
-	// Ensure there's always 4 parts, as expressions can have between 2 and 4 parts
-	while (part_count < 4) {
-		// Expression is augmented with '*' expressions
-		bfs[part_count] = ~0;
-		part_count++;
+	out->hour_count = bf_to_nums(&out->hours, bit_field, min[1], max[1]);
+
+	// Minutes
+	bit_field = 0;
+
+	res = ce_parse_part(&bit_field, parts[0], min[0], max[0]);
+
+	if (res != cron_parse_ok) {
+		goto end;
 	}
 
-	out->minute_count = bf_to_nums(&out->minutes, bfs[0], min[0], max[0]);
-	out->hour_count = bf_to_nums(&out->hours, bfs[1], min[1], max[1]);
-	out->day_count = bf_to_nums(&out->days, bfs[2], min[2], max[2]);
-	out->month_count = bf_to_nums(&out->months, bfs[3], min[3], max[3]);
+	out->minute_count = bf_to_nums(&out->minutes, bit_field, min[0], max[0]);
 
 end:
 	// s is cloned
diff --git a/src/cron/expression.v b/src/cron/expression.v
index 0429b93..23e1354 100644
--- a/src/cron/expression.v
+++ b/src/cron/expression.v
@@ -2,21 +2,22 @@ module cron
 
 import time
 
+[unsafe]
+pub fn (ce &Expression) free() {
+	C.ce_free(ce)
+}
+
 pub fn parse_expression(exp string) !&Expression {
 	out := C.ce_init()
 	res := C.ce_parse_expression(out, exp.str)
 
 	if res != 0 {
-		return error('yuhh')
+		return error(res.str())
 	}
 
 	return out
 }
 
-pub fn (ce &Expression) free() {
-	C.ce_free(ce)
-}
-
 pub fn (ce &Expression) next(ref time.Time) !time.Time {
 	st := SimpleTime{
 		year: ref.year
diff --git a/src/cron/expression_test.v b/src/cron/expression_test.v
index e1a9849..d6ec002 100644
--- a/src/cron/expression_test.v
+++ b/src/cron/expression_test.v
@@ -22,7 +22,7 @@ fn test_next_simple() ! {
 	// util_test_time('0 3', '2002-01-01 00:00:00', '2002-01-01 03:00:00')!
 
 	// Overlap to next day
-	mut exp := '0    3        '
+	mut exp := '0    3         '
 	util_test_time(exp, '2002-01-01 03:00:00', '2002-01-02 03:00:00')!
 	util_test_time(exp, '2002-01-01 04:00:00', '2002-01-02 03:00:00')!
 

From 8c0315dea679fe52daa23d429310a7db645fbde8 Mon Sep 17 00:00:00 2001
From: Jef Roosens <roosensjef@gmail.com>
Date: Sat, 14 Jan 2023 18:51:01 +0100
Subject: [PATCH 2/2] refactor(cron): make next function infallible

---
 src/build/queue.v               | 14 +++++---------
 src/console/schedule/schedule.v |  2 +-
 src/cron/c/expression.c         |  8 +++-----
 src/cron/c/expression.h         |  4 ++--
 src/cron/expression.c.v         |  4 ++--
 src/cron/expression.v           | 22 +++++++---------------
 src/cron/expression_test.v      |  2 +-
 src/server/log_removal.v        | 11 +----------
 8 files changed, 22 insertions(+), 45 deletions(-)

diff --git a/src/build/queue.v b/src/build/queue.v
index 122180e..abd4ec6 100644
--- a/src/build/queue.v
+++ b/src/build/queue.v
@@ -92,7 +92,7 @@ pub fn (mut q BuildJobQueue) insert(input InsertConfig) ! {
 				q.default_schedule
 			}
 
-			job.timestamp = ce.next_from_now()!
+			job.timestamp = ce.next_from_now()
 			job.ce = ce
 		} else {
 			job.timestamp = time.now()
@@ -105,8 +105,8 @@ pub fn (mut q BuildJobQueue) insert(input InsertConfig) ! {
 // reschedule the given job by calculating the next timestamp and re-adding it
 // to its respective queue. This function is called by the pop functions
 // *after* having pop'ed the job.
-fn (mut q BuildJobQueue) reschedule(job BuildJob, arch string) ! {
-	new_timestamp := job.ce.next_from_now()!
+fn (mut q BuildJobQueue) reschedule(job BuildJob, arch string) {
+	new_timestamp := job.ce.next_from_now()
 
 	new_job := BuildJob{
 		...job
@@ -168,10 +168,7 @@ pub fn (mut q BuildJobQueue) pop(arch string) ?BuildJob {
 			job = q.queues[arch].pop()?
 
 			if !job.single {
-				// TODO how do we handle this properly? Is it even possible for a
-				// cron expression to not return a next time if it's already been
-				// used before?
-				q.reschedule(job, arch) or {}
+				q.reschedule(job, arch)
 			}
 
 			return job
@@ -198,8 +195,7 @@ pub fn (mut q BuildJobQueue) pop_n(arch string, n int) []BuildJob {
 				job = q.queues[arch].pop() or { break }
 
 				if !job.single {
-					// TODO idem
-					q.reschedule(job, arch) or {}
+					q.reschedule(job, arch)
 				}
 
 				out << job
diff --git a/src/console/schedule/schedule.v b/src/console/schedule/schedule.v
index 40b300f..ceabf24 100644
--- a/src/console/schedule/schedule.v
+++ b/src/console/schedule/schedule.v
@@ -22,7 +22,7 @@ pub fn cmd() cli.Command {
 			ce := cron.parse_expression(cmd.args.join(' '))!
 			count := cmd.flags.get_int('count')!
 
-			for t in ce.next_n(time.now(), count)! {
+			for t in ce.next_n(time.now(), count) {
 				println(t)
 			}
 		}
diff --git a/src/cron/c/expression.c b/src/cron/c/expression.c
index 0051e49..59d1bf0 100644
--- a/src/cron/c/expression.c
+++ b/src/cron/c/expression.c
@@ -15,7 +15,7 @@ void ce_free(cron_expression *ce) {
 	free(ce);
 }
 
-int ce_next(cron_simple_time *out, cron_expression *ce, cron_simple_time *ref) {
+void ce_next(cron_simple_time *out, cron_expression *ce, cron_simple_time *ref) {
 	// For all of these values, the rule is the following: if their value is
 	// the length of their respective array in the CronExpression object, that
 	// means we've looped back around. This means that the "bigger" value has
@@ -94,11 +94,9 @@ int ce_next(cron_simple_time *out, cron_expression *ce, cron_simple_time *ref) {
 	} else {
 		out->year = ref->year;
 	}
-
-	return 0;
 }
 
-int ce_next_from_now(cron_simple_time *out, cron_expression *ce) {
+void ce_next_from_now(cron_simple_time *out, cron_expression *ce) {
 	time_t t = time(NULL);
 	struct tm gm;
 	gmtime_r(&t, &gm);
@@ -112,5 +110,5 @@ int ce_next_from_now(cron_simple_time *out, cron_expression *ce) {
 		.minute = gm.tm_min
 	};
 
-	return ce_next(out, ce, &ref);
+	ce_next(out, ce, &ref);
 }
diff --git a/src/cron/c/expression.h b/src/cron/c/expression.h
index 1c86436..c599e4e 100644
--- a/src/cron/c/expression.h
+++ b/src/cron/c/expression.h
@@ -37,9 +37,9 @@ cron_expression *ce_init();
 
 void cron_ce_free(cron_expression *ce);
 
-int cron_ce_next(cron_simple_time *out, cron_expression *ce, cron_simple_time *ref);
+void cron_ce_next(cron_simple_time *out, cron_expression *ce, cron_simple_time *ref);
 
-int cron_ce_next_from_now(cron_simple_time *out, cron_expression *ce);
+void cron_ce_next_from_now(cron_simple_time *out, cron_expression *ce);
 
 enum cron_parse_error cron_ce_parse_expression(cron_expression *out, char *s);
 
diff --git a/src/cron/expression.c.v b/src/cron/expression.c.v
index 08af25c..7551e6f 100644
--- a/src/cron/expression.c.v
+++ b/src/cron/expression.c.v
@@ -32,8 +32,8 @@ fn C.ce_init() &C.cron_expression
 
 fn C.ce_free(ce &C.cron_expression)
 
-fn C.ce_next(out &C.cron_simple_time, ce &C.cron_expression, ref &C.cron_simple_time) int
+fn C.ce_next(out &C.cron_simple_time, ce &C.cron_expression, ref &C.cron_simple_time)
 
-fn C.ce_next_from_now(out &C.cron_simple_time, ce &C.cron_expression) int
+fn C.ce_next_from_now(out &C.cron_simple_time, ce &C.cron_expression)
 
 fn C.ce_parse_expression(out &C.cron_expression, s &char) int
diff --git a/src/cron/expression.v b/src/cron/expression.v
index 23e1354..c0cab8d 100644
--- a/src/cron/expression.v
+++ b/src/cron/expression.v
@@ -18,7 +18,7 @@ pub fn parse_expression(exp string) !&Expression {
 	return out
 }
 
-pub fn (ce &Expression) next(ref time.Time) !time.Time {
+pub fn (ce &Expression) next(ref time.Time) time.Time {
 	st := SimpleTime{
 		year: ref.year
 		month: ref.month
@@ -28,11 +28,7 @@ pub fn (ce &Expression) next(ref time.Time) !time.Time {
 	}
 
 	out := SimpleTime{}
-	res := C.ce_next(&out, ce, &st)
-
-	if res != 0 {
-		return error('yuhh')
-	}
+	C.ce_next(&out, ce, &st)
 
 	return time.new_time(time.Time{
 		year: out.year
@@ -43,13 +39,9 @@ pub fn (ce &Expression) next(ref time.Time) !time.Time {
 	})
 }
 
-pub fn (ce &Expression) next_from_now() !time.Time {
+pub fn (ce &Expression) next_from_now() time.Time {
 	out := SimpleTime{}
-	res := C.ce_next_from_now(&out, ce)
-
-	if res != 0 {
-		return error('yuhh')
-	}
+	C.ce_next_from_now(&out, ce)
 
 	return time.new_time(time.Time{
 		year: out.year
@@ -62,13 +54,13 @@ pub fn (ce &Expression) next_from_now() !time.Time {
 
 // next_n returns the n next occurences of the expression, given a starting
 // time.
-pub fn (ce &Expression) next_n(ref time.Time, n int) ![]time.Time {
+pub fn (ce &Expression) next_n(ref time.Time, n int) []time.Time {
 	mut times := []time.Time{cap: n}
 
-	times << ce.next(ref)!
+	times << ce.next(ref)
 
 	for i in 1 .. n {
-		times << ce.next(times[i - 1])!
+		times << ce.next(times[i - 1])
 	}
 
 	return times
diff --git a/src/cron/expression_test.v b/src/cron/expression_test.v
index d6ec002..7d1516d 100644
--- a/src/cron/expression_test.v
+++ b/src/cron/expression_test.v
@@ -8,7 +8,7 @@ fn util_test_time(exp string, t1_str string, t2_str string) ! {
 	t1 := parse(t1_str)!
 	t2 := parse(t2_str)!
 
-	t3 := ce.next(t1)!
+	t3 := ce.next(t1)
 
 	assert t2.year == t3.year
 	assert t2.month == t3.month
diff --git a/src/server/log_removal.v b/src/server/log_removal.v
index 98cba93..7f1cfb5 100644
--- a/src/server/log_removal.v
+++ b/src/server/log_removal.v
@@ -9,11 +9,7 @@ const fallback_log_removal_frequency = 24 * time.hour
 
 // log_removal_daemon removes old build logs every `log_removal_frequency`.
 fn (mut app App) log_removal_daemon(schedule cron.Expression) {
-	mut start_time := time.Time{}
-
 	for {
-		start_time = time.now()
-
 		mut too_old_timestamp := time.now().add_days(-app.conf.max_log_age)
 
 		app.linfo('Cleaning logs before $too_old_timestamp')
@@ -51,12 +47,7 @@ fn (mut app App) log_removal_daemon(schedule cron.Expression) {
 		app.linfo('Cleaned $counter logs ($failed failed)')
 
 		// Sleep until the next cycle
-		next_time := schedule.next_from_now() or {
-			app.lerror("Log removal daemon couldn't calculate next time: $err.msg(); fallback to $server.fallback_log_removal_frequency")
-
-			start_time.add(server.fallback_log_removal_frequency)
-		}
-
+		next_time := schedule.next_from_now()
 		time.sleep(next_time - time.now())
 	}
 }