From d982bd5323b2f0dfe4cef6b592fa4a1e3cf7ed7c Mon Sep 17 00:00:00 2001 From: Will Thompson Date: Fri, 19 Apr 2024 17:36:55 +0100 Subject: [PATCH] aggregate-tally: Propagate errors while iterating Previously emer_aggregate_tally_iter() and emer_aggregate_tally_iter_before() had no return value and would simply call g_warning() in the error case. This makes it hard to test the error paths without aborting the test program. Make each function return an error and move the g_warning() out into the calling functions. https://phabricator.endlessm.com/T35331 --- daemon/emer-aggregate-tally.c | 70 ++++++++++++------------ daemon/emer-aggregate-tally.h | 28 +++++----- daemon/emer-daemon.c | 71 +++++++++++++++--------- tests/daemon/test-aggregate-tally.c | 85 ++++++++++++++++++----------- tools/print-aggregates.c | 23 ++++++-- 5 files changed, 168 insertions(+), 109 deletions(-) diff --git a/daemon/emer-aggregate-tally.c b/daemon/emer-aggregate-tally.c index d79f9f5..14b9440 100644 --- a/daemon/emer-aggregate-tally.c +++ b/daemon/emer-aggregate-tally.c @@ -659,18 +659,23 @@ emer_aggregate_tally_iter_internal (EmerAggregateTally *self, * @flags: Whether to delete events while iterating * @func: (scope call): Callback to call for each matching event * @user_data: (closure func): Data to pass to @func + * @error: Location to store an error if one occurs while iterating or + * deleting events. * * Iterate all events in the tally for the given frequency, either on a * specific date/month or (if @datetime is %NULL) on all dates at that * frequency. + * + * Returns: %TRUE on success, or %FALSE with @error set otherwise. */ -void -emer_aggregate_tally_iter (EmerAggregateTally *self, - EmerTallyType tally_type, - GDateTime *datetime, - EmerTallyIterFlags flags, - EmerTallyIterFunc func, - gpointer user_data) +gboolean +emer_aggregate_tally_iter (EmerAggregateTally *self, + EmerTallyType tally_type, + GDateTime *datetime, + EmerTallyIterFlags flags, + EmerTallyIterFunc func, + gpointer user_data, + GError **error) { const char *SELECT_SQL = "SELECT id, event_id, unix_user_id, " @@ -683,43 +688,40 @@ emer_aggregate_tally_iter (EmerAggregateTally *self, "FROM tally " "WHERE length(date) = ?"; const char *query = (datetime != NULL) ? SELECT_SQL : SELECT_ALL_SQL; - g_autoptr(GError) error = NULL; - if (!emer_aggregate_tally_iter_internal (self, - query, - tally_type, - datetime, - flags, - func, - user_data, - &error)) - g_critical ("%s: %s", G_STRFUNC, error->message); + return emer_aggregate_tally_iter_internal (self, + query, + tally_type, + datetime, + flags, + func, + user_data, + error); } -void -emer_aggregate_tally_iter_before (EmerAggregateTally *self, - EmerTallyType tally_type, - GDateTime *datetime, - EmerTallyIterFlags flags, - EmerTallyIterFunc func, - gpointer user_data) +gboolean +emer_aggregate_tally_iter_before (EmerAggregateTally *self, + EmerTallyType tally_type, + GDateTime *datetime, + EmerTallyIterFlags flags, + EmerTallyIterFunc func, + gpointer user_data, + GError **error) { const char *SELECT_SQL = "SELECT id, event_id, unix_user_id, " " payload, counter, date " "FROM tally " "WHERE length(date) = length(?1) AND date < ?1;"; - g_autoptr(GError) error = NULL; - if (!emer_aggregate_tally_iter_internal (self, - SELECT_SQL, - tally_type, - datetime, - flags, - func, - user_data, - &error)) - g_critical ("%s: %s", G_STRFUNC, error->message); + return emer_aggregate_tally_iter_internal (self, + SELECT_SQL, + tally_type, + datetime, + flags, + func, + user_data, + error); } gboolean diff --git a/daemon/emer-aggregate-tally.h b/daemon/emer-aggregate-tally.h index 1eb5e02..1cbbdcc 100644 --- a/daemon/emer-aggregate-tally.h +++ b/daemon/emer-aggregate-tally.h @@ -68,19 +68,21 @@ gboolean emer_aggregate_tally_store_event (EmerAggregateTally *self, GDateTime *datetime, GError **error); -void emer_aggregate_tally_iter (EmerAggregateTally *self, - EmerTallyType tally_type, - GDateTime *datetime, - EmerTallyIterFlags flags, - EmerTallyIterFunc func, - gpointer user_data); - -void emer_aggregate_tally_iter_before (EmerAggregateTally *self, - EmerTallyType tally_type, - GDateTime *datetime, - EmerTallyIterFlags flags, - EmerTallyIterFunc func, - gpointer user_data); +gboolean emer_aggregate_tally_iter (EmerAggregateTally *self, + EmerTallyType tally_type, + GDateTime *datetime, + EmerTallyIterFlags flags, + EmerTallyIterFunc func, + gpointer user_data, + GError **error); + +gboolean emer_aggregate_tally_iter_before (EmerAggregateTally *self, + EmerTallyType tally_type, + GDateTime *datetime, + EmerTallyIterFlags flags, + EmerTallyIterFunc func, + gpointer user_data, + GError **error); gboolean emer_aggregate_tally_clear (EmerAggregateTally *self, GError **error); diff --git a/daemon/emer-daemon.c b/daemon/emer-daemon.c index 50617ab..c8cc8f4 100644 --- a/daemon/emer-daemon.c +++ b/daemon/emer-daemon.c @@ -1152,6 +1152,7 @@ clock_ticked_midnight_cb (gpointer user_data) g_autoptr (GDateTime) now = NULL; g_autofree char *date = NULL; gint64 now_monotonic_us; + g_autoptr(GError) error = NULL; now_monotonic_us = g_get_monotonic_time (); @@ -1167,12 +1168,17 @@ clock_ticked_midnight_cb (gpointer user_data) self->current_aggregate_tally_date, now_monotonic_us); - emer_aggregate_tally_iter (self->aggregate_tally, - EMER_TALLY_DAILY_EVENTS, - self->current_aggregate_tally_date, - EMER_TALLY_ITER_FLAG_DELETE, - buffer_aggregate_event_to_queue, - self); + if (!emer_aggregate_tally_iter (self->aggregate_tally, + EMER_TALLY_DAILY_EVENTS, + self->current_aggregate_tally_date, + EMER_TALLY_ITER_FLAG_DELETE, + buffer_aggregate_event_to_queue, + self, + &error)) + { + g_warning ("Failed to buffer daily events for %s: %s", date, error->message); + g_clear_error (&error); + } /* Monthly events */ now = g_date_time_new_now_local (); @@ -1191,12 +1197,17 @@ clock_ticked_midnight_cb (gpointer user_data) self->current_aggregate_tally_date, now_monotonic_us); - emer_aggregate_tally_iter (self->aggregate_tally, - EMER_TALLY_MONTHLY_EVENTS, - self->current_aggregate_tally_date, - EMER_TALLY_ITER_FLAG_DELETE, - buffer_aggregate_event_to_queue, - self); + if (!emer_aggregate_tally_iter (self->aggregate_tally, + EMER_TALLY_MONTHLY_EVENTS, + self->current_aggregate_tally_date, + EMER_TALLY_ITER_FLAG_DELETE, + buffer_aggregate_event_to_queue, + self, + &error)) + { + g_warning ("Failed to buffer monthly events for %s: %s", month_date, error->message); + g_clear_error (&error); + } } split_aggregate_timers (self, now_monotonic_us); @@ -1318,21 +1329,31 @@ static void buffer_past_aggregate_events (EmerDaemon *self) { g_autoptr(GDateTime) now = g_date_time_new_now_local (); + g_autoptr(GError) error = NULL; - emer_aggregate_tally_iter_before (self->aggregate_tally, - EMER_TALLY_DAILY_EVENTS, - now, - EMER_TALLY_ITER_FLAG_DELETE, - buffer_aggregate_event_to_queue, - self); - - emer_aggregate_tally_iter_before (self->aggregate_tally, - EMER_TALLY_MONTHLY_EVENTS, - now, - EMER_TALLY_ITER_FLAG_DELETE, - buffer_aggregate_event_to_queue, - self); + if (!emer_aggregate_tally_iter_before (self->aggregate_tally, + EMER_TALLY_DAILY_EVENTS, + now, + EMER_TALLY_ITER_FLAG_DELETE, + buffer_aggregate_event_to_queue, + self, + &error)) + { + g_warning ("Failed to buffer past daily events: %s", error->message); + g_clear_error (&error); + } + if (!emer_aggregate_tally_iter_before (self->aggregate_tally, + EMER_TALLY_MONTHLY_EVENTS, + now, + EMER_TALLY_ITER_FLAG_DELETE, + buffer_aggregate_event_to_queue, + self, + &error)) + { + g_warning ("Failed to buffer past monthly events: %s", error->message); + g_clear_error (&error); + } } static void diff --git a/tests/daemon/test-aggregate-tally.c b/tests/daemon/test-aggregate-tally.c index bcfddcf..7b1887f 100644 --- a/tests/daemon/test-aggregate-tally.c +++ b/tests/daemon/test-aggregate-tally.c @@ -199,14 +199,13 @@ test_aggregate_tally_iter (struct Fixture *fixture, g_autoptr(GVariant) payload = v_str (payload_str); g_autoptr(GPtrArray) events = g_ptr_array_new_with_free_func (aggregate_event_free); int i; + g_autoptr(GError) error = NULL; // Add the same aggregate event multiple times. It must // result in a single aggregate event with the sum of // counters for (i = 0; i < 10; i++) { - g_autoptr(GError) error = NULL; - emer_aggregate_tally_store_event (fixture->tally, EMER_TALLY_DAILY_EVENTS, 1001, @@ -224,7 +223,9 @@ test_aggregate_tally_iter (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 1); AggregateEvent *e = g_ptr_array_index (events, 0); @@ -243,7 +244,9 @@ test_aggregate_tally_iter (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 0); } @@ -288,8 +291,9 @@ test_aggregate_tally_permutations (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, n_unix_uids * G_N_ELEMENTS (uuids) * G_N_ELEMENTS (payloads)); for (size_t i = 0; i < events->len; i++) { @@ -307,7 +311,9 @@ test_aggregate_tally_permutations (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 0); } @@ -337,7 +343,9 @@ test_aggregate_tally_large_counter_single (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 1); AggregateEvent *e = g_ptr_array_index (events, 0); @@ -381,8 +389,9 @@ test_aggregate_tally_large_counter_add (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 1); AggregateEvent *e = g_ptr_array_index (events, 0); g_assert_cmpuint (e->counter, ==, ((guint32) G_MAXINT32) + 1); @@ -419,8 +428,9 @@ test_aggregate_tally_large_counter_upper_bound (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 1); AggregateEvent *e = g_ptr_array_index (events, 0); g_assert_cmpuint (e->counter, ==, G_MAXUINT32); @@ -475,8 +485,9 @@ test_aggregate_tally_iter_before_daily (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DEFAULT, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 25); for (i = 0; i < events->len; i++) { @@ -494,8 +505,9 @@ test_aggregate_tally_iter_before_daily (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 25); for (i = 0; i < events->len; i++) { @@ -511,8 +523,9 @@ test_aggregate_tally_iter_before_daily (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DEFAULT, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 0); } @@ -565,8 +578,9 @@ test_aggregate_tally_iter_before_monthly (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DEFAULT, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 12); for (i = 0; i < events->len; i++) { @@ -584,8 +598,9 @@ test_aggregate_tally_iter_before_monthly (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 12); for (i = 0; i < events->len; i++) { @@ -601,8 +616,9 @@ test_aggregate_tally_iter_before_monthly (struct Fixture *fixture, datetime, EMER_TALLY_ITER_FLAG_DEFAULT, tally_iter_func, - events); - + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 0); } @@ -656,7 +672,9 @@ test_aggregate_tally_iter_all (struct Fixture *fixture, NULL, EMER_TALLY_ITER_FLAG_DEFAULT, tally_iter_func, - events); + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 50); g_ptr_array_set_size (events, 0); @@ -666,7 +684,9 @@ test_aggregate_tally_iter_all (struct Fixture *fixture, NULL, EMER_TALLY_ITER_FLAG_DEFAULT, tally_iter_func, - events); + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 1); } @@ -721,21 +741,24 @@ test_aggregate_tally_read_only (struct Fixture *fixture, NULL, EMER_TALLY_ITER_FLAG_DEFAULT, tally_iter_func, - events); + events, + &error); + g_assert_no_error (error); g_assert_cmpuint (events->len, ==, 1); /* Try to delete – this should fail */ - /* TODO: this currently just prints a CRITICAL (and crashes the test) */ -#if 0 g_ptr_array_set_size (events, 0); emer_aggregate_tally_iter (tally_ro, EMER_TALLY_DAILY_EVENTS, NULL, EMER_TALLY_ITER_FLAG_DELETE, tally_iter_func, - events); + events, + &error); + /* No particular assertion about the error */ + g_assert_nonnull (error); g_assert_cmpuint (events->len, ==, 1); -#endif + g_clear_error (&error); /* Try to insert via the read-only connection: this should fail. */ gboolean ret = emer_aggregate_tally_store_event (tally_ro, EMER_TALLY_DAILY_EVENTS, 0, uuids[1], NULL, 2, datetime, &error); diff --git a/tools/print-aggregates.c b/tools/print-aggregates.c index 2f1610c..0e2bf8f 100644 --- a/tools/print-aggregates.c +++ b/tools/print-aggregates.c @@ -63,11 +63,18 @@ print_event (guint32 unix_user_id, return EMER_TALLY_ITER_CONTINUE; } -static void -print_events (EmerAggregateTally *tally, - EmerTallyType tally_type) +static gboolean +print_events (EmerAggregateTally *tally, + EmerTallyType tally_type, + GError **error) { - emer_aggregate_tally_iter (tally, tally_type, NULL, EMER_TALLY_ITER_FLAG_DEFAULT, print_event, NULL); + return emer_aggregate_tally_iter (tally, + tally_type, + NULL, + EMER_TALLY_ITER_FLAG_DEFAULT, + print_event, + NULL, + error); } gint @@ -118,8 +125,12 @@ main (gint argc, } g_print ("%s", HEADER); - print_events (tally, EMER_TALLY_DAILY_EVENTS); - print_events (tally, EMER_TALLY_MONTHLY_EVENTS); + if (!print_events (tally, EMER_TALLY_DAILY_EVENTS, &error) || + !print_events (tally, EMER_TALLY_MONTHLY_EVENTS, &error)) + { + g_printerr ("Error while listing events: %s\n", error->message); + return EXIT_FAILURE; + } return 0; }