Refactor command line option checks

README.md

@@ -8,3 +8,8 @@ replication, and perform administrative tasks such as failover or switchover
 operations.
 
 `repmgr 4` is a complete rewrite of the existing `repmgr` codebase.
+
+Commands
+--------
+
+repmgr cluster event [--all] [--node-id] [--node-name] [--event] [--event-matching]

config.c

@@ -656,33 +656,6 @@ exit_with_errors(ItemList *config_errors, ItemList *config_warnings)
 }
 
 
-void
-item_list_append(ItemList *item_list, char *error_message)
-{
-	ItemListCell *cell;
-
-	cell = (ItemListCell *) pg_malloc0(sizeof(ItemListCell));
-
-	if (cell == NULL)
-	{
-		log_error(_("unable to allocate memory; terminating."));
-		exit(ERR_BAD_CONFIG);
-	}
-
-	cell->string = pg_malloc0(MAXLEN);
-	strncpy(cell->string, error_message, MAXLEN);
-
-	if (item_list->tail)
-	{
-		item_list->tail->next = cell;
-	}
-	else
-	{
-		item_list->head = cell;
-	}
-
-	item_list->tail = cell;
-}
 
 
 /*

config.h

@@ -26,18 +26,6 @@ typedef struct EventNotificationList
 } EventNotificationList;
 
 
-typedef struct ItemListCell
-{
-	struct ItemListCell *next;
-	char			    *string;
-} ItemListCell;
-
-typedef struct ItemList
-{
-	ItemListCell *head;
-	ItemListCell *tail;
-} ItemList;
-
 
 typedef struct TablespaceListCell
 {
@@ -145,7 +133,6 @@ bool		load_config(const char *config_file, bool verbose, t_configuration_options
 bool		parse_config(t_configuration_options *options);
 bool		reload_config(t_configuration_options *orig_options);
 
-void		item_list_append(ItemList *item_list, char *error_message);
 
 int			repmgr_atoi(const char *s,
 						const char *config_item,

repmgr-client.c

@@ -11,6 +11,8 @@
  * [ MASTER | PRIMARY ] REGISTER
  *
  * STANDBY CLONE (wip)
+ *
+ * CLUSTER EVENT
  */
 
 #include <unistd.h>
@@ -58,7 +60,7 @@ main(int argc, char **argv)
 	logger_output_mode = OM_COMMAND_LINE;
 
 
-	while ((c = getopt_long(argc, argv, "?Vf:vtFb:S:L:", long_options,
+	while ((c = getopt_long(argc, argv, "?Vf:Fb:S:D:L:vt", long_options,
 							&optindex)) != -1)
 	{
 		/*
@@ -112,6 +114,10 @@ main(int argc, char **argv)
 				strncpy(runtime_options.superuser, optarg, MAXLEN);
 				break;
 
+			case 'D':
+				strncpy(runtime_options.data_dir, optarg, MAXPGPATH);
+				break;
+
 			/* logging options
 			 * --------------- */
 
@@ -182,7 +188,7 @@ main(int argc, char **argv)
 	 *	 STANDBY {REGISTER | UNREGISTER | CLONE [node] | PROMOTE | FOLLOW [node] | SWITCHOVER | REWIND} |
 	 *	 WITNESS { CREATE | REGISTER | UNREGISTER } |
 	 *	 BDR { REGISTER | UNREGISTER } |
-	 *	 CLUSTER { CROSSCHECK | MATRIX | SHOW | CLEANUP }
+	 *	 CLUSTER { CROSSCHECK | MATRIX | SHOW | CLEANUP | EVENT }
 	 *
 	 * [node] is an optional hostname, provided instead of the -h/--host optipn
 	 */
@@ -207,6 +213,11 @@ main(int argc, char **argv)
 			if (strcasecmp(repmgr_action, "REGISTER") == 0)
 				action = MASTER_REGISTER;
 		}
+		else if(strcasecmp(repmgr_node_type, "CLUSTER") == 0)
+		{
+			if (strcasecmp(repmgr_action, "EVENT") == 0)
+				action = CLUSTER_EVENT;
+		}
 		else
 		{
 			valid_repmgr_node_type_found = false;
@@ -246,6 +257,16 @@ main(int argc, char **argv)
 		item_list_append(&cli_errors, command_error.data);
 	}
 
+	if (optind < argc)
+	{
+		PQExpBufferData too_many_args;
+		initPQExpBuffer(&too_many_args);
+		appendPQExpBuffer(&too_many_args, _("too many command-line arguments (first extra is \"%s\")"), argv[optind]);
+		item_list_append(&cli_errors, too_many_args.data);
+	}
+
+	check_cli_parameters(action);
+
 	/*
 	 * Sanity checks for command line parameters completed by now;
 	 * any further errors will be runtime ones
@@ -255,6 +276,15 @@ main(int argc, char **argv)
 		exit_with_errors();
 	}
 
+	/*
+	 * Print any warnings about inappropriate command line options,
+	 * unless -t/--terse set
+	 */
+	if (cli_warnings.head != NULL && runtime_options.terse == false)
+	{
+		log_warning(_("following problems with command line parameters detected:"));
+		print_item_list(&cli_warnings);
+	}
 
 	/*
 	 * The configuration file is not required for some actions (e.g. 'standby clone'),
@@ -354,6 +384,9 @@ main(int argc, char **argv)
 		case STANDBY_CLONE:
 			do_standby_clone();
 			break;
+		case CLUSTER_EVENT:
+			do_cluster_event();
+			break;
 		default:
 			/* An action will have been determined by this point  */
 			break;
@@ -364,12 +397,96 @@ main(int argc, char **argv)
 
 
 
+/*
+ * Check for useless or conflicting parameters, and also whether a
+ * configuration file is required.
+ *
+ * Messages will be added to the command line warning and error lists
+ * as appropriate.
+ *
+ * XXX for each individual actions, check only required actions
+ * for non-required actions check warn if provided
+ */
+
+static void
+check_cli_parameters(const int action)
+{
+	/* ========================================================================
+	 * check all parameters required for an action are provided, and warn
+	 * about ineffective actions
+	 * ========================================================================
+	 */
+	switch (action)
+	{
+		case MASTER_REGISTER:
+			/* no required parameters */
+		case CLUSTER_EVENT:
+			/* no required parameters */
+			break;
+
+	}
+
+	/* ========================================================================
+	 * warn if parameters provided for an action where they're not relevant
+	 * ========================================================================
+	 */
+
+	/* --host etc.*/
+	if (runtime_options.connection_param_provided)
+	{
+		switch (action)
+		{
+			case STANDBY_CLONE:
+			case STANDBY_FOLLOW:
+				break;
+			default:
+				item_list_append_format(&cli_warnings,
+										_("datqabase connection parameters not required when executing %s"),
+										action_name(action));
+		}
+	}
+
+	/* -D/--data-dir */
+	if (runtime_options.data_dir[0])
+	{
+		switch (action)
+		{
+			case STANDBY_CLONE:
+			case STANDBY_FOLLOW:
+			case STANDBY_RESTORE_CONFIG:
+				break;
+			default:
+				item_list_append_format(&cli_warnings,
+										_("-D/--pgdata not required when executing %s"),
+										action_name(action));
+		}
+	}
+
+
+}
+
+
+static const char*
+action_name(const int action)
+{
+	switch(action)
+	{
+		case MASTER_REGISTER:
+			return "MASTER REGISTER";
+		case STANDBY_CLONE:
+			return "STANDBY CLONE";
+		case CLUSTER_EVENT:
+			return "CLUSTER EVENT";
+	}
+
+	return "UNKNOWN ACTION";
+}
 static void
 exit_with_errors(void)
 {
 	fprintf(stderr, _("The following command line errors were encountered:\n"));
 
-	print_error_list(&cli_errors, LOG_ERR);
+	print_item_list(&cli_errors);
 
 	fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname());
 
@@ -378,23 +495,13 @@ exit_with_errors(void)
 
 
 static void
-print_error_list(ItemList *error_list, int log_level)
+print_item_list(ItemList *item_list)
 {
 	ItemListCell *cell;
 
-	for (cell = error_list->head; cell; cell = cell->next)
+	for (cell = item_list->head; cell; cell = cell->next)
 	{
-		fprintf(stderr, "  ");
-		switch(log_level)
-		{
-			/* Currently we only need errors and warnings */
-			case LOG_ERROR:
-				log_error("%s", cell->string);
-				break;
-			case LOG_WARNING:
-				log_warning("%s", cell->string);
-				break;
-		}
+		fprintf(stderr, "  %s\n", cell->string);
 	}
 }
 
@@ -593,7 +700,30 @@ do_standby_clone(void)
 	puts("standby clone");
 }
 
-// this should be the only place where superuser rights required
+
+/*
+ * CLUSTER EVENT
+ *
+ * Parameters:
+ *   --limit[=20]
+ *   --all
+ *   --node_[id|name]
+ *   --event
+ *   --event-matching
+ */
+static void
+do_cluster_event(void)
+{
+	puts("cluster event");
+}
+
+
+/*
+ * Create the repmgr extension, and grant access to the repmgr
+ * user if not a superuser.
+ *
+ * Note: this should be the only place where superuser rights are required
+ */
 static
 bool create_repmgr_extension(PGconn *conn)
 {

repmgr-client.h

@@ -37,8 +37,9 @@
 #define CLUSTER_CLEANUP		   14
 #define CLUSTER_MATRIX		   15
 #define CLUSTER_CROSSCHECK	   16
-#define BDR_REGISTER		   17
-#define BDR_UNREGISTER		   18
+#define CLUSTER_EVENT		   17
+#define BDR_REGISTER		   18
+#define BDR_UNREGISTER		   19
 
 /* command line options without short versions */
 #define OPT_HELP						 1
@@ -72,6 +73,9 @@ static struct option long_options[] =
 
 /* connection options */
 	{"superuser", required_argument, NULL, 'S'},
+	{"pgdata", required_argument, NULL, 'D'},
+	/* legacy alias for -D/--pgdata*/
+	{"data-dir", required_argument, NULL, 'D'},
 
 /* logging options */
 	{"log-level", required_argument, NULL, 'L'},
@@ -79,12 +83,11 @@ static struct option long_options[] =
 	{"terse", required_argument, NULL, 't'},
 	{"verbose", no_argument, NULL, 'v'},
 
+/* not yet handled */
 	{"dbname", required_argument, NULL, 'd'},
 	{"host", required_argument, NULL, 'h'},
 	{"port", required_argument, NULL, 'p'},
 	{"username", required_argument, NULL, 'U'},
-	{"superuser", required_argument, NULL, 'S'},
-	{"pgdata", required_argument, NULL, 'D'},
 	{"remote-user", required_argument, NULL, 'R'},
 	{"wal-keep-segments", required_argument, NULL, 'w'},
 	{"keep-history", required_argument, NULL, 'k'},
@@ -115,6 +118,11 @@ static struct option long_options[] =
 
 typedef struct
 {
+	/* configuration metadata */
+	bool		conninfo_provided;
+	bool		connection_param_provided;
+	bool		host_param_provided;
+
 	/* general configuration options */
 	char		config_file[MAXPGPATH];
 	bool		force;
@@ -127,25 +135,34 @@ typedef struct
 	bool		verbose;
 
 	/* connection options */
+	char		data_dir[MAXPGPATH];
 	char		superuser[MAXLEN];
 
 
 }	t_runtime_options;
 
 #define T_RUNTIME_OPTIONS_INITIALIZER { \
+		/* configuration metadata */ \
+		false, false, false, \
 		/* general configuration options */	\
 		"", false, "", \
 		/* logging options */ \
 		"", false, false, false, \
 		/* connection options */ \
-		""}
+		"", ""}
 
 static void do_help(void);
 static void do_master_register(void);
+
 static void do_standby_clone(void);
 
+static void do_cluster_event(void);
+
+
+static const char *action_name(const int action);
 static void exit_with_errors(void);
-static void print_error_list(ItemList *error_list, int log_level);
+static void print_item_list(ItemList *item_list);
+static void check_cli_parameters(const int action);
 static int	check_server_version(PGconn *conn, char *server_type, bool exit_on_error, char *server_version_string);
 static bool create_repmgr_extension(PGconn *conn);
 

strutil.c

@@ -24,8 +24,8 @@ xvsnprintf(char *str, size_t size, const char *format, va_list ap)
 
 	if (retval >= (int) size)
 	{
-		log_error(_("Buffer of size not large enough to format entire string '%s'"),
-				str);
+		log_error(_("buffer of size not large enough to format entire string '%s'"),
+				  str);
 		exit(ERR_STR_OVERFLOW);
 	}
 
@@ -60,6 +60,42 @@ maxlen_snprintf(char *str, const char *format,...)
 }
 
 
+void
+item_list_append(ItemList *item_list, const char *message)
+{
+	item_list_append_format(item_list, "%s", message);
+}
+
+void
+item_list_append_format(ItemList *item_list, const char *format, ...)
+{
+	ItemListCell *cell;
+	va_list		arglist;
+
+	cell = (ItemListCell *) pg_malloc0(sizeof(ItemListCell));
+
+	if (cell == NULL)
+	{
+		log_error(_("unable to allocate memory; terminating."));
+		exit(ERR_BAD_CONFIG);
+	}
+
+	cell->string = pg_malloc0(MAXLEN);
+
+	va_start(arglist, format);
+
+	(void) xvsnprintf(cell->string, MAXLEN, format, arglist);
+	va_end(arglist);
+
+
+	if (item_list->tail)
+		item_list->tail->next = cell;
+	else
+		item_list->head = cell;
+
+	item_list->tail = cell;
+}
+
 /*
  * Escape a string for use as a parameter in recovery.conf
  * Caller must free returned value

strutil.h

@@ -17,6 +17,19 @@
 
 #define MAXLEN_STR STR(MAXLEN)
 
+typedef struct ItemListCell
+{
+	struct ItemListCell *next;
+	char			    *string;
+} ItemListCell;
+
+typedef struct ItemList
+{
+	ItemListCell *head;
+	ItemListCell *tail;
+} ItemList;
+
+
 extern int
 sqlquery_snprintf(char *str, const char *format,...)
 __attribute__((format(PG_PRINTF_ATTRIBUTE, 2, 3)));
@@ -25,7 +38,15 @@ extern int
 maxlen_snprintf(char *str, const char *format,...)
 __attribute__((format(PG_PRINTF_ATTRIBUTE, 2, 3)));
 
+extern void
+item_list_append(ItemList *item_list, const char *message);
+
+extern void
+item_list_append_format(ItemList *item_list, const char *format, ...)
+__attribute__((format(PG_PRINTF_ATTRIBUTE, 2, 3)));
+
 extern char *
 escape_recovery_conf_value(const char *src);
 
+
 #endif	 /* _STRUTIL_H_ */