Merge lp:~ogayot/whoopsie/whoopsie into lp:whoopsie

[Olivier Gayot]

Add option for exiting right after processing existing files.

Hello,

This merge proposal is a preliminary step for moving to path-based activation of whoopsie should we decide to.

The main idea is to add to whoopsie the option to process existing reports and then exit immediately.

It consists of two different commits:
 1, stop calling process_existing_files if we know we are offline ; and therefore are not going to do any operation (rev 723)
 2. add the --no-polling switch so that whoopsie can process existing files and exit directly (rev 724)

This PR should have no incidence on the way whoopsie operates by default (i.e. without the --no-polling option).

In a subsequent merge proposal, we can move to path-based activation by changing the systemd unit files. More work might be needed in whoopsie itself if we want to improve the integration with apport though.

[lp:~ogayot/whoopsie/whoopsie]

723. By Olivier Gayot on 2022-01-31

Don't call process_existing_files if not considered online

When online_status if false, a call to process_existing_files is more or less a
no-op. We are just looping through all the report files and ignoring them (i.e.
either by calling "continue" or by reaching the end of the iteration ; without
calling either parse_and_upload_report or mark_handled a single time).

Therefore, I removed the check for online_status from inside
process_existing_files and stop calling the function if online_status is false.

724. By Olivier Gayot on 2022-01-31

Add option to process existing files and exit

The new --no-polling option can be used to process existing files and exit
directly. This new option implies --assume-online for the following reason:
 * No matter what value assume_online takes, the first call to
process_existing_files always ignores it because we have online_status set to
true at the start of the process.

Therefore, if we exit after a single call to process_existing_files, the
variable assume_online is irrelevant. But theoretically, we _are_ assuming that
the network is online so it makes sense to set the value to true.

725. By Olivier Gayot on 2022-02-15

Bump version number to 0.2.77

[Brian Murray (brian-murray)]

This looks great and I'm happy to upload it, but I have one question which appears in the diff.

review: Needs Information

[Olivier Gayot (ogayot)]

> This looks great and I'm happy to upload it, but I have one question which
> appears in the diff.

Thanks for the feedback, I appreciate it!

Although whoopsie is not single-threaded strictly speaking, both `process_existing_files` and `network_changed` are callbacks managed by a single event-loop. Therefore, they shouldn't have the opportunity to execute in parallel.

I wanted to confirm that my understanding was correct so I ran a quick test where whoopsie sleeps for 5 seconds between each crash report it tries to process (there are 3 crash reports). I brought the network down while whoopsie was slowly reading the crash directory and it did not detect the network change until it finishes processing the files, so it should be good :)

[22:08:30] Not a paid data plan: /org/freedesktop/NetworkManager/ActiveConnection/15
[22:08:30] Found usable connection: /org/freedesktop/NetworkManager/ActiveConnection/15
[22:08:35] Now moving on to file _usr_bin_devnull-http.0.crash [ online_state is 1 ]
$ nmcli networking off
[22:08:40] Now moving on to file _usr_bin_debdiff-apply.1000.crash [ online_state is 1 ]
[22:08:45] Now moving on to file _usr_lib_chromium-browser_chromium-browser.1000.crash [ online_state is 1 ]
[22:08:45] offline

Thanks,
Olivier

[Brian Murray (brian-murray)]

This was manually merged from a debdiff that was created.

=== modified file 'debian/changelog'
--- debian/changelog	2021-03-06 01:03:04 +0000
+++ debian/changelog	2022-02-15 16:01:51 +0000
@@ -1,3 +1,13 @@
+whoopsie (0.2.77) UNRELEASED; urgency=medium
+
+  * Added the --no-polling switch so that whoopsie can process existing files
+    and exit directly (LP: #1424768)
+  * Don't try to process files when we know we're offline. Eventually, we
+    would just be ignoring each report ; which would result in useless CPU and
+    IO usage.
+
+ -- Olivier Gayot <[e-mail]>  Tue, 15 Feb 2022 16:52:07 +0100
+
 whoopsie (0.2.76) hirsute; urgency=medium
 
   * Drop build-dependency on obsolete dh-systemd

=== modified file 'src/whoopsie.c'
--- src/whoopsie.c	2021-02-02 23:56:03 +0000
+++ src/whoopsie.c	2022-02-15 16:01:51 +0000
@@ -97,9 +97,16 @@
 
 #ifndef TEST
 static int assume_online = 0;
+
+/*
+ * Tells whether whoopsie will exit right after processing existing files.
+ */
+static int use_polling = 1;
+
 static GOptionEntry option_entries[] = {
     { "foreground", 'f', 0, G_OPTION_ARG_NONE, &foreground, "Run in the foreground", NULL },
     { "assume-online", 'a', 0, G_OPTION_ARG_NONE, &assume_online, "Always assume there is a route to $CRASH_DB_URL.", NULL },
+    { "no-polling", 0, G_OPTION_FLAG_REVERSE, G_OPTION_ARG_NONE, &use_polling, "Process existing files and exit. Implies --assume-online.", NULL },
     { NULL }
 };
 #endif
@@ -794,7 +801,16 @@
     return success;
 }
 
-gboolean
+static gboolean
+process_existing_files_if_online (const char* report_dir)
+{
+    if (online_state) {
+        process_existing_files (report_dir);
+    }
+    return G_SOURCE_CONTINUE;
+}
+
+void
 process_existing_files (const char* report_dir)
 {
     GDir* dir = NULL;
@@ -826,7 +842,7 @@
             g_free (upload_file);
             g_free (crash_file);
             continue;
-        } else if (online_state && parse_and_upload_report (crash_file)) {
+        } else if (parse_and_upload_report (crash_file)) {
             if (!mark_handled (crash_file, last_uploaded_oopsid)) {
                 log_msg ("Unable to mark report as seen (%s) removing it.\n", crash_file);
                 g_unlink (crash_file);
@@ -837,8 +853,6 @@
         g_free (crash_file);
     }
     g_dir_close (dir);
-
-    return G_SOURCE_CONTINUE;
 }
 
 void daemonize (void)
@@ -1067,6 +1081,16 @@
     sigaction (SIGINT, &action, NULL);
 }
 
+static void
+start_polling (void)
+{
+    g_timeout_add_seconds (PROCESS_OUTSTANDING_TIMEOUT,
+                           (GSourceFunc) process_existing_files_if_online, (gpointer) report_dir);
+
+    loop = g_main_loop_new (NULL, FALSE);
+    g_main_loop_run (loop);
+}
+
 int
 main (int argc, char** argv)
 {
@@ -1077,6 +1101,18 @@
     setup_signals ();
     parse_arguments (&argc, &argv);
 
+    if (!use_polling) {
+        /*
+         * In non polling mode, we exit after the first invocation of
+         * process_existing_files.
+         * Since we start the process with online_status already set to TRUE,
+         * the first invocation of process_existing_files will always ignore
+         * the assume_online variable. Therefore, assume_online is irrelevant
+         * in non polling mode.
+         */
+        assume_online = TRUE;
+    }
+
     if (!foreground) {
         open_log ();
         log_msg ("whoopsie " VERSION " starting up.\n");
@@ -1148,12 +1184,12 @@
     if (!assume_online)
         monitor_connectivity (crash_db_url, network_changed);
 
-    process_existing_files (report_dir);
-    g_timeout_add_seconds (PROCESS_OUTSTANDING_TIMEOUT,
-                           (GSourceFunc) process_existing_files, (gpointer) report_dir);
+    /* As long as we keep online_state to TRUE by default, this initial call
+     * happens unconditionally. */
+    process_existing_files_if_online (report_dir);
 
-    loop = g_main_loop_new (NULL, FALSE);
-    g_main_loop_run (loop);
+    if (use_polling)
+        start_polling ();
 
     unmonitor_directory (monitor, check_online_then_upload);
     if (!assume_online)

=== modified file 'src/whoopsie.h'
--- src/whoopsie.h	2020-08-05 22:00:17 +0000
+++ src/whoopsie.h	2022-02-15 16:01:51 +0000
@@ -25,7 +25,7 @@
 gsize get_report_max_size (void);
 void destroy_key_and_value (gpointer key, gpointer value, gpointer user_data);
 void drop_privileges (GError** error);
-gboolean process_existing_files (const char* report_dir);
+void process_existing_files (const char* report_dir);
 extern char* last_uploaded_oopsid;
 
 #endif /* WHOOPSIE_H */