cli: remove editor threadbg/cli-readline-async-rh1368353

Currently the editor runs in a dedicated thread so that the blocking
call to readline() doesn't stop the processing of D-Bus events in the
main loop. The editor thread can access objects concurrently with the
main thread and this can cause races and crashes.

Remove the editor thread and use the non-blocking readline API.

https://bugzilla.gnome.org/show_bug.cgi?id=732097
https://bugzilla.redhat.com/show_bug.cgi?id=1368353

4 files changed, 97 insertions, 219 deletions

diff --git a/clients/cli/common.c b/clients/cli/common.c
index f1ec46a11e..88668e8c9e 100644
--- a/clients/cli/common.c
+++ b/clients/cli/common.c
@@ -33,6 +33,8 @@
 #include "common.h"
 #include "utils.h"
 
+extern GMainLoop *loop;
+
 /* Available fields for IPv4 group */
 NmcOutputField nmc_fields_ip4_config[] = {
 	{"GROUP",      N_("GROUP")},    /* 0 */
@@ -1157,6 +1159,11 @@ nmc_unique_connection_name (const GPtrArray *connections, const char *try_name)
 	return new_name;
 }
 
+/* readline state variables */
+static gboolean nmcli_in_readline = FALSE;
+static gboolean rl_got_line;
+static char *rl_string;
+
 /**
  * nmc_cleanup_readline:
  *
@@ -1170,88 +1177,94 @@ nmc_cleanup_readline (void)
 	rl_cleanup_after_signal ();
 }
 
-
-static gboolean nmcli_in_readline = FALSE;
-static pthread_mutex_t readline_mutex = PTHREAD_MUTEX_INITIALIZER;
-
 gboolean
 nmc_get_in_readline (void)
 {
-	gboolean in_readline;
-
-	pthread_mutex_lock (&readline_mutex);
-	in_readline = nmcli_in_readline;
-	pthread_mutex_unlock (&readline_mutex);
-	return in_readline;
+	return nmcli_in_readline;
 }
 
 void
 nmc_set_in_readline (gboolean in_readline)
 {
-	pthread_mutex_lock (&readline_mutex);
 	nmcli_in_readline = in_readline;
-	pthread_mutex_unlock (&readline_mutex);
+}
+
+static void
+readline_cb (char *line)
+{
+	rl_got_line = TRUE;
+	rl_string = line;
+	rl_callback_handler_remove ();
+}
+
+static gboolean
+stdin_ready_cb (GIOChannel * io, GIOCondition condition, gpointer data)
+{
+	rl_callback_read_char ();
+	return TRUE;
 }
 
 static char *
 nmc_readline_helper (const char *prompt)
 {
-	char *str;
-	int b;
+	GIOChannel *io = NULL;
+	guint io_watch_id;
 
-readline_mark:
-	/* We are in readline -> Ctrl-C should not quit nmcli */
 	nmc_set_in_readline (TRUE);
-	str = readline (prompt);
-	/* We are outside readline -> Ctrl-C should quit nmcli */
-	nmc_set_in_readline (FALSE);
 
-	/* Check for an I/O error by attempting to peek into the input buffer.
-	 * Readline just inserts newlines when errors occur so we need to check ourselves. */
-	if (ioctl (0, FIONREAD, &b) == -1) {
-		g_free (str);
-		str = NULL;
+	io = g_io_channel_unix_new (STDIN_FILENO);
+	io_watch_id = g_io_add_watch (io, G_IO_IN, stdin_ready_cb, NULL);
+	g_io_channel_unref (io);
+
+read_again:
+	rl_string = NULL;
+	rl_got_line = FALSE;
+	rl_callback_handler_install (prompt, readline_cb);
+
+	while (   !rl_got_line
+	       && g_main_loop_is_running (loop)
+	       && !nmc_seen_sigint ())
+		g_main_context_iteration (NULL, TRUE);
+
+	/* If Ctrl-C was detected, complete the line */
+	if (nmc_seen_sigint ()) {
+		rl_echo_signal_char (SIGINT);
+		rl_stuff_char ('\n');
+		rl_callback_read_char ();
 	}
 
 	/* Add string to the history */
-	if (str && *str)
-		add_history (str);
-
-	/*-- React on Ctrl-C and Ctrl-D --*/
-	/* We quit on Ctrl-D when line is empty */
-	if (str == NULL) {
-		/* Send SIGQUIT to itself */
-		nmc_set_sigquit_internal ();
-		kill (getpid (), SIGQUIT);
-		/* Sleep in this thread so that we don't do anything else until exit */
-		for (;;)
-			sleep (3);
-	}
-	/* Ctrl-C */
+	if (rl_string && *rl_string)
+		add_history (rl_string);
+
 	if (nmc_seen_sigint ()) {
+		/* Ctrl-C */
 		nmc_clear_sigint ();
-		if (nm_cli.in_editor || *str) {
+		if (   nm_cli.in_editor
+		    || (rl_string  && *rl_string)) {
 			/* In editor, or the line is not empty */
 			/* Call readline again to get new prompt (repeat) */
-			g_free (str);
-			goto readline_mark;
+			g_free (rl_string);
+			goto read_again;
 		} else {
-			/* Not in editor and line is empty */
-			/* Send SIGQUIT to itself */
-			nmc_set_sigquit_internal ();
-			kill (getpid (), SIGQUIT);
-			/* Sleep in this thread so that we don't do anything else until exit */
-			for (;;)
-				sleep (3);
+			/* Not in editor and line is empty, exit */
+			nmc_exit ();
 		}
+	} else if (!rl_string) {
+		/* Ctrl-D, exit */
+		nmc_exit ();
 	}
 
 	/* Return NULL, not empty string */
-	if (str && *str == '\0') {
-		g_free (str);
-		str = NULL;
+	if (rl_string && *rl_string == '\0') {
+		g_free (rl_string);
+		rl_string = NULL;
 	}
-	return str;
+
+	g_source_remove (io_watch_id);
+	nmc_set_in_readline (FALSE);
+
+	return rl_string;
 }
 
 /**
diff --git a/clients/cli/connections.c b/clients/cli/connections.c
index 3b809e2ab1..25f43f77f0 100644
--- a/clients/cli/connections.c
+++ b/clients/cli/connections.c
@@ -6243,8 +6243,6 @@ typedef struct {
 static gboolean nmc_editor_cb_called;
 static GError *nmc_editor_error;
 static MonitorACInfo *nmc_editor_monitor_ac;
-static GMutex nmc_editor_mutex;
-static GCond nmc_editor_cond;
 
 /*
  * Store 'error' to shared 'nmc_editor_error' and monitoring info to
@@ -6254,12 +6252,9 @@ static GCond nmc_editor_cond;
 static void
 set_info_and_signal_editor_thread (GError *error, MonitorACInfo *monitor_ac_info)
 {
-	g_mutex_lock (&nmc_editor_mutex);
 	nmc_editor_cb_called = TRUE;
 	nmc_editor_error = error ? g_error_copy (error) : NULL;
 	nmc_editor_monitor_ac = monitor_ac_info;
-	g_cond_signal (&nmc_editor_cond);
-	g_mutex_unlock (&nmc_editor_mutex);
 }
 
 static void
@@ -7403,10 +7398,9 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t
 					update_connection (persistent, rem_con, update_connection_editor_cb, NULL);
 				}
 
-				g_mutex_lock (&nmc_editor_mutex);
 				//FIXME: add also a timeout for cases the callback is not called
 				while (!nmc_editor_cb_called)
-					g_cond_wait (&nmc_editor_cond, &nmc_editor_mutex);
+					g_main_context_iteration (NULL, TRUE);
 
 				if (nmc_editor_error) {
 					g_print (_("Error: Failed to save '%s' (%s) connection: %s\n"),
@@ -7448,7 +7442,6 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t
 
 				nmc_editor_cb_called = FALSE;
 				nmc_editor_error = NULL;
-				g_mutex_unlock (&nmc_editor_mutex);
 			} else {
 				g_print (_("Error: connection verification failed: %s\n"),
 				         err1 ? err1->message : _("(unknown error)"));
@@ -7493,9 +7486,8 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t
 				break;
 			}
 
-			g_mutex_lock (&nmc_editor_mutex);
 			while (!nmc_editor_cb_called)
-				g_cond_wait (&nmc_editor_cond, &nmc_editor_mutex);
+				g_main_context_iteration (NULL, TRUE);
 
 			if (nmc_editor_error) {
 				g_print (_("Error: Failed to activate '%s' (%s) connection: %s\n"),
@@ -7504,8 +7496,7 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t
 				         nmc_editor_error->message);
 				g_error_free (nmc_editor_error);
 			} else {
-				g_print (_("Monitoring connection activation (press any key to continue)\n"));
-				nmc_get_user_input ("");
+				nmc_readline (_("Monitoring connection activation (press any key to continue)\n"));
 			}
 
 			if (nmc_editor_monitor_ac) {
@@ -7516,7 +7507,6 @@ editor_menu_main (NmCli *nmc, NMConnection *connection, const char *connection_t
 			nmc_editor_cb_called = FALSE;
 			nmc_editor_error = NULL;
 			nmc_editor_monitor_ac = NULL;
-			g_mutex_unlock (&nmc_editor_mutex);
 
 			/* Update timestamp in local connection */
 			update_connection_timestamp (NM_CONNECTION (rem_con), connection);
@@ -7734,7 +7724,7 @@ editor_init_existing_connection (NMConnection *connection)
 }
 
 static NMCResultCode
-do_connection_edit_func (NmCli *nmc, int argc, char **argv)
+do_connection_edit (NmCli *nmc, int argc, char **argv)
 {
 	NMConnection *connection = NULL;
 	NMSettingConnection *s_con;
@@ -7908,55 +7898,12 @@ do_connection_edit_func (NmCli *nmc, int argc, char **argv)
 		g_object_unref (connection);
 	g_free (nmc_tab_completion.con_type);
 
-	nmc->should_wait++;
 	return nmc->return_value;
 
 error:
 	g_assert (!connection);
 	g_free (type_ask);
 
-	nmc->should_wait++;
-	return nmc->return_value;
-}
-
-typedef struct {
-	NmCli *nmc;
-	int argc;
-	char **argv;
-} NmcEditorThreadData;
-
-static GThread *editor_thread;
-static NmcEditorThreadData editor_thread_data;
-
-/*
- * We need to run do_connection_edit_func() in a thread so that
- * glib main loop is not blocked and could receive and process D-Bus
- * return messages.
- */
-static gpointer
-connection_editor_thread_func (gpointer data)
-{
-	NmcEditorThreadData *td = (NmcEditorThreadData *) data;
-
-	/* run editor for editing/adding connections */
-	td->nmc->return_value = do_connection_edit_func (td->nmc, td->argc, td->argv);
-
-	/* quit glib main loop now that we are done with this thread */
-	quit ();
-
-	return NULL;
-}
-
-static NMCResultCode
-do_connection_edit (NmCli *nmc, int argc, char **argv)
-{
-	nmc->should_wait++;
-	editor_thread_data.nmc = nmc;
-	editor_thread_data.argc = argc;
-	editor_thread_data.argv = argv;
-	editor_thread = g_thread_new ("editor-thread", connection_editor_thread_func, &editor_thread_data);
-	g_thread_unref (editor_thread);
-
 	return nmc->return_value;
 }
 
diff --git a/clients/cli/nmcli.c b/clients/cli/nmcli.c
index 8bc8828b41..890dc0d062 100644
--- a/clients/cli/nmcli.c
+++ b/clients/cli/nmcli.c
@@ -25,10 +25,10 @@
 #include <string.h>
 #include <stdlib.h>
 #include <signal.h>
-#include <pthread.h>
 #include <termios.h>
 #include <unistd.h>
 #include <locale.h>
+#include <glib-unix.h>
 #include <readline/readline.h>
 #include <readline/history.h>
 
@@ -61,7 +61,6 @@ typedef struct {
 
 /* --- Global variables --- */
 GMainLoop *loop = NULL;
-static sigset_t signal_set;
 struct termios termios_orig;
 
 static void
@@ -385,124 +384,50 @@ parse_command_line (NmCli *nmc, int argc, char **argv)
 }
 
 static gboolean nmcli_sigint = FALSE;
-static pthread_mutex_t sigint_mutex = PTHREAD_MUTEX_INITIALIZER;
-static gboolean nmcli_sigquit_internal = FALSE;
 
 gboolean
 nmc_seen_sigint (void)
 {
-	gboolean sigint;
-
-	pthread_mutex_lock (&sigint_mutex);
-	sigint = nmcli_sigint;
-	pthread_mutex_unlock (&sigint_mutex);
-	return sigint;
+	return nmcli_sigint;
 }
 
 void
 nmc_clear_sigint (void)
 {
-	pthread_mutex_lock (&sigint_mutex);
 	nmcli_sigint = FALSE;
-	pthread_mutex_unlock (&sigint_mutex);
-}
-
-void
-nmc_set_sigquit_internal (void)
-{
-	nmcli_sigquit_internal = TRUE;
 }
 
-static int
-event_hook_for_readline (void)
+void nmc_exit (void)
 {
-	/* Make readline() exit on SIGINT */
-	if (nmc_seen_sigint ()) {
-		rl_echo_signal_char (SIGINT);
-		rl_stuff_char ('\n');
-	}
-	return 0;
+	tcsetattr (STDIN_FILENO, TCSADRAIN, &termios_orig);
+	nmc_cleanup_readline ();
+	exit (1);
 }
 
-void *signal_handling_thread (void *arg);
-/*
- * Thread function waiting for signals and processing them.
- * Wait for signals in signal set. The semantics of sigwait() require that all
- * threads (including the thread calling sigwait()) have the signal masked, for
- * reliable operation. Otherwise, a signal that arrives while this thread is
- * not blocked in sigwait() might be delivered to another thread.
- */
-void *
-signal_handling_thread (void *arg) {
-	int signo;
-
-	while (1) {
-		sigwait (&signal_set, &signo);
-
-		switch (signo) {
-		case SIGINT:
-			if (nmc_get_in_readline ()) {
-				/* Don't quit when in readline, only signal we received SIGINT */
-				pthread_mutex_lock (&sigint_mutex);
-				nmcli_sigint = TRUE;
-				pthread_mutex_unlock (&sigint_mutex);
-			} else {
-				/* We can quit nmcli */
-				tcsetattr (STDIN_FILENO, TCSADRAIN, &termios_orig);
-				nmc_cleanup_readline ();
-				g_print (_("\nError: nmcli terminated by signal %s (%d)\n"),
-				         strsignal (signo), signo);
-				exit (1);
-			}
-			break;
-		case SIGQUIT:
-		case SIGTERM:
-			tcsetattr (STDIN_FILENO, TCSADRAIN, &termios_orig);
-			nmc_cleanup_readline ();
-			if (!nmcli_sigquit_internal)
-				g_print (_("\nError: nmcli terminated by signal %s (%d)\n"),
-				         strsignal (signo), signo);
-			exit (1);
-			break;
-		default:
-			break;
-		}
-	}
-	return NULL;
-}
-
-/*
- * Mask the signals we are interested in and create a signal handling thread.
- * Because all threads inherit the signal mask from their creator, all threads
- * in the process will have the signals masked. That's why setup_signals() has
- * to be called before creating other threads.
- */
 static gboolean
-setup_signals (void)
+signal_handler (gpointer user_data)
 {
-	pthread_t signal_thread_id;
-	int status;
-
-	sigemptyset (&signal_set);
-	sigaddset (&signal_set, SIGINT);
-	sigaddset (&signal_set, SIGQUIT);
-	sigaddset (&signal_set, SIGTERM);
-
-	/* Block all signals of interest. */
-	status = pthread_sigmask (SIG_BLOCK, &signal_set, NULL);
-	if (status != 0) {
-		g_printerr (_("Failed to set signal mask: %d\n"), status);
-		return FALSE;
-	}
+	int signo = GPOINTER_TO_INT (user_data);
 
-	/* Create the signal handling thread. */
-	status = pthread_create (&signal_thread_id, NULL, signal_handling_thread, NULL);
-	if (status != 0) {
-		g_printerr (_("Failed to create signal handling thread: %d\n"), status);
-		return FALSE;
+	switch (signo) {
+	case SIGINT:
+		if (nmc_get_in_readline ()) {
+			nmcli_sigint = TRUE;
+		} else {
+			g_print (_("Error: nmcli terminated by signal %s (%d)\n"),
+			         strsignal (signo),
+			         signo);
+			g_main_loop_quit (loop);
+		}
+		break;
+	case SIGTERM:
+		g_print (_("Error: nmcli terminated by signal %s (%d)\n"),
+		         strsignal (signo), signo);
+		nmc_exit ();
+		break;
 	}
 
-	return TRUE;
+	return G_SOURCE_CONTINUE;
 }
 
 static void
@@ -682,10 +607,6 @@ main (int argc, char *argv[])
 {
 	ArgsInfo args_info = { &nm_cli, argc, argv };
 
-	/* Set up unix signal handling */
-	if (!setup_signals ())
-		exit (NMC_RESULT_ERROR_UNKNOWN);
-
 	/* Set locale to use environment variables */
 	setlocale (LC_ALL, "");
 
@@ -701,12 +622,8 @@ main (int argc, char *argv[])
 	/* Save terminal settings */
 	tcgetattr (STDIN_FILENO, &termios_orig);
 
-	/* readline init */
-	rl_event_hook = event_hook_for_readline;
-	/* Set 0.01s timeout to mitigate slowness in readline when a broken version is used.
-	 * See https://bugzilla.redhat.com/show_bug.cgi?id=1109946
-	 */
-	rl_set_keyboard_input_timeout (10000);
+	g_unix_signal_add (SIGTERM, signal_handler, GINT_TO_POINTER (SIGTERM));
+	g_unix_signal_add (SIGINT, signal_handler, GINT_TO_POINTER (SIGINT));
 
 	nmc_value_transforms_register ();
 
diff --git a/clients/cli/nmcli.h b/clients/cli/nmcli.h
index a8908685bd..dc9549db3c 100644
--- a/clients/cli/nmcli.h
+++ b/clients/cli/nmcli.h
@@ -174,5 +174,6 @@ GQuark nmcli_error_quark (void);
 gboolean nmc_seen_sigint (void);
 void     nmc_clear_sigint (void);
 void     nmc_set_sigquit_internal (void);
+void     nmc_exit (void);
 
 #endif /* NMC_NMCLI_H */