Merge pull request #291 from kulaginm/cmdline-refactor

remove backup_subcmd global variable and useless command_name variable

src/help.c

@@ -7,8 +7,11 @@
  *-------------------------------------------------------------------------
  */
 
+#include <assert.h>
 #include "pg_probackup.h"
 
+static void help_nocmd(void);
+static void help_internal(void);
 static void help_init(void);
 static void help_backup(void);
 static void help_restore(void);
@@ -24,58 +27,61 @@ static void help_del_instance(void);
 static void help_archive_push(void);
 static void help_archive_get(void);
 static void help_checkdb(void);
+static void help_help(void);
+static void help_version(void);
 
 void
-help_command(char *command)
+help_print_version(void)
 {
-	if (strcmp(command, "init") == 0)
-		help_init();
-	else if (strcmp(command, "backup") == 0)
-		help_backup();
-	else if (strcmp(command, "restore") == 0)
-		help_restore();
-	else if (strcmp(command, "validate") == 0)
-		help_validate();
-	else if (strcmp(command, "show") == 0)
-		help_show();
-	else if (strcmp(command, "delete") == 0)
-		help_delete();
-	else if (strcmp(command, "merge") == 0)
-		help_merge();
-	else if (strcmp(command, "set-backup") == 0)
-		help_set_backup();
-	else if (strcmp(command, "set-config") == 0)
-		help_set_config();
-	else if (strcmp(command, "show-config") == 0)
-		help_show_config();
-	else if (strcmp(command, "add-instance") == 0)
-		help_add_instance();
-	else if (strcmp(command, "del-instance") == 0)
-		help_del_instance();
-	else if (strcmp(command, "archive-push") == 0)
-		help_archive_push();
-	else if (strcmp(command, "archive-get") == 0)
-		help_archive_get();
-	else if (strcmp(command, "checkdb") == 0)
-		help_checkdb();
-	else if (strcmp(command, "--help") == 0
-			 || strcmp(command, "help") == 0
-			 || strcmp(command, "-?") == 0
-			 || strcmp(command, "--version") == 0
-			 || strcmp(command, "version") == 0
-			 || strcmp(command, "-V") == 0)
-		printf(_("No help page for \"%s\" command. Try pg_probackup help\n"), command);
-	else
-		printf(_("Unknown command \"%s\". Try pg_probackup help\n"), command);
-	exit(0);
+#ifdef PGPRO_VERSION
+	fprintf(stdout, "%s %s (Postgres Pro %s %s)\n",
+			PROGRAM_NAME, PROGRAM_VERSION,
+			PGPRO_VERSION, PGPRO_EDITION);
+#else
+	fprintf(stdout, "%s %s (PostgreSQL %s)\n",
+			PROGRAM_NAME, PROGRAM_VERSION, PG_VERSION);
+#endif
+}
+
+void
+help_command(ProbackupSubcmd const subcmd)
+{
+	typedef void (* help_function_ptr)(void);
+	/* Order is important, keep it in sync with utils/configuration.h:enum ProbackupSubcmd declaration */
+	static help_function_ptr const help_functions[] =
+	{
+		&help_nocmd,
+		&help_init,
+		&help_add_instance,
+		&help_del_instance,
+		&help_archive_push,
+		&help_archive_get,
+		&help_backup,
+		&help_restore,
+		&help_validate,
+		&help_delete,
+		&help_merge,
+		&help_show,
+		&help_set_config,
+		&help_set_backup,
+		&help_show_config,
+		&help_checkdb,
+		&help_internal, // SSH_CMD
+		&help_internal, // AGENT_CMD
+		&help_help,
+		&help_version,
+	};
+
+	Assert((int)subcmd < sizeof(help_functions) / sizeof(help_functions[0]));
+        help_functions[(int)subcmd]();
 }
 
 void
 help_pg_probackup(void)
 {
-	printf(_("\n%s - utility to manage backup/recovery of PostgreSQL database.\n\n"), PROGRAM_NAME);
+	printf(_("\n%s - utility to manage backup/recovery of PostgreSQL database.\n"), PROGRAM_NAME);
 
-	printf(_("  %s help [COMMAND]\n"), PROGRAM_NAME);
+	printf(_("\n  %s help [COMMAND]\n"), PROGRAM_NAME);
 
 	printf(_("\n  %s version\n"), PROGRAM_NAME);
 
@@ -247,7 +253,18 @@ help_pg_probackup(void)
 		if (PROGRAM_EMAIL)
 			printf("Report bugs to <%s>.\n", PROGRAM_EMAIL);
 	}
-	exit(0);
+}
+
+static void
+help_nocmd(void)
+{
+	printf(_("\nUnknown command. Try pg_probackup help\n\n"));
+}
+
+static void
+help_internal(void)
+{
+	printf(_("\nThis command is intended for internal use\n\n"));
 }
 
 static void
@@ -971,3 +988,17 @@ help_archive_get(void)
 	printf(_("      --ssh-options=ssh_options    additional ssh options (default: none)\n"));
 	printf(_("                                   (example: --ssh-options='-c cipher_spec -F configfile')\n\n"));
 }
+
+static void
+help_help(void)
+{
+	printf(_("\n%s help [command]\n"), PROGRAM_NAME);
+	printf(_("%s command --help\n\n"), PROGRAM_NAME);
+}
+
+static void
+help_version(void)
+{
+	printf(_("\n%s version\n"), PROGRAM_NAME);
+	printf(_("%s --version\n\n"), PROGRAM_NAME);
+}

src/pg_probackup.c

@@ -27,27 +27,6 @@ const char  *PROGRAM_FULL_PATH = NULL;
 const char  *PROGRAM_URL = "https://github.com/postgrespro/pg_probackup";
 const char  *PROGRAM_EMAIL = "https://github.com/postgrespro/pg_probackup/issues";
 
-typedef enum ProbackupSubcmd
-{
-	NO_CMD = 0,
-	INIT_CMD,
-	ADD_INSTANCE_CMD,
-	DELETE_INSTANCE_CMD,
-	ARCHIVE_PUSH_CMD,
-	ARCHIVE_GET_CMD,
-	BACKUP_CMD,
-	RESTORE_CMD,
-	VALIDATE_CMD,
-	DELETE_CMD,
-	MERGE_CMD,
-	SHOW_CMD,
-	SET_CONFIG_CMD,
-	SET_BACKUP_CMD,
-	SHOW_CONFIG_CMD,
-	CHECKDB_CMD
-} ProbackupSubcmd;
-
-
 /* directory options */
 char	   *backup_path = NULL;
 /*
@@ -152,7 +131,6 @@ static pgSetBackupParams *set_backup_params = NULL;
 
 /* current settings */
 pgBackup	current;
-static ProbackupSubcmd backup_subcmd = NO_CMD;
 
 static bool help_opt = false;
 
@@ -160,7 +138,7 @@ static void opt_incr_restore_mode(ConfigOption *opt, const char *arg);
 static void opt_backup_mode(ConfigOption *opt, const char *arg);
 static void opt_show_format(ConfigOption *opt, const char *arg);
 
-static void compress_init(void);
+static void compress_init(ProbackupSubcmd const subcmd);
 
 static void opt_datname_exclude_list(ConfigOption *opt, const char *arg);
 static void opt_datname_include_list(ConfigOption *opt, const char *arg);
@@ -259,32 +237,14 @@ static ConfigOption cmd_options[] =
 	{ 0 }
 };
 
-static void
-setMyLocation(void)
-{
-
-#ifdef WIN32
-	if (IsSshProtocol())
-		elog(ERROR, "Currently remote operations on Windows are not supported");
-#endif
-
-	MyLocation = IsSshProtocol()
-		? (backup_subcmd == ARCHIVE_PUSH_CMD || backup_subcmd == ARCHIVE_GET_CMD)
-		   ? FIO_DB_HOST
-		   : (backup_subcmd == BACKUP_CMD || backup_subcmd == RESTORE_CMD || backup_subcmd == ADD_INSTANCE_CMD)
-		      ? FIO_BACKUP_HOST
-		      : FIO_LOCAL_HOST
-		: FIO_LOCAL_HOST;
-}
-
 /*
  * Entry point of pg_probackup command.
  */
 int
 main(int argc, char *argv[])
 {
-	char	   *command = NULL,
-			   *command_name;
+	char	   *command = NULL;
+	ProbackupSubcmd backup_subcmd = NO_CMD;
 
 	PROGRAM_NAME_FULL = argv[0];
 
@@ -322,91 +282,58 @@ main(int argc, char *argv[])
 	/* Parse subcommands and non-subcommand options */
 	if (argc > 1)
 	{
-		if (strcmp(argv[1], "archive-push") == 0)
-			backup_subcmd = ARCHIVE_PUSH_CMD;
-		else if (strcmp(argv[1], "archive-get") == 0)
-			backup_subcmd = ARCHIVE_GET_CMD;
-		else if (strcmp(argv[1], "add-instance") == 0)
-			backup_subcmd = ADD_INSTANCE_CMD;
-		else if (strcmp(argv[1], "del-instance") == 0)
-			backup_subcmd = DELETE_INSTANCE_CMD;
-		else if (strcmp(argv[1], "init") == 0)
-			backup_subcmd = INIT_CMD;
-		else if (strcmp(argv[1], "backup") == 0)
-			backup_subcmd = BACKUP_CMD;
-		else if (strcmp(argv[1], "restore") == 0)
-			backup_subcmd = RESTORE_CMD;
-		else if (strcmp(argv[1], "validate") == 0)
-			backup_subcmd = VALIDATE_CMD;
-		else if (strcmp(argv[1], "delete") == 0)
-			backup_subcmd = DELETE_CMD;
-		else if (strcmp(argv[1], "merge") == 0)
-			backup_subcmd = MERGE_CMD;
-		else if (strcmp(argv[1], "show") == 0)
-			backup_subcmd = SHOW_CMD;
-		else if (strcmp(argv[1], "set-config") == 0)
-			backup_subcmd = SET_CONFIG_CMD;
-		else if (strcmp(argv[1], "set-backup") == 0)
-			backup_subcmd = SET_BACKUP_CMD;
-		else if (strcmp(argv[1], "show-config") == 0)
-			backup_subcmd = SHOW_CONFIG_CMD;
-		else if (strcmp(argv[1], "checkdb") == 0)
-			backup_subcmd = CHECKDB_CMD;
+		backup_subcmd = parse_subcmd(argv[1]);
+		switch(backup_subcmd)
+		{
+			case SSH_CMD:
 #ifdef WIN32
-		else if (strcmp(argv[1], "ssh") == 0)
-		    launch_ssh(argv);
-#endif
-		else if (strcmp(argv[1], "agent") == 0)
-		{
-			/* 'No forward compatibility' sanity:
-			 *   /old/binary  -> ssh execute -> /newer/binary agent version_num
-			 * If we are executed as an agent for older binary, then exit with error
-			 */
-			if (argc > 2)
-			{
-				elog(ERROR, "Version mismatch, pg_probackup binary with version '%s' "
-						"is launched as an agent for pg_probackup binary with version '%s'",
-						PROGRAM_VERSION, argv[2]);
-			}
-			fio_communicate(STDIN_FILENO, STDOUT_FILENO);
-			return 0;
-		}
-		else if (strcmp(argv[1], "--help") == 0 ||
-				 strcmp(argv[1], "-?") == 0 ||
-				 strcmp(argv[1], "help") == 0)
-		{
-			if (argc > 2)
-				help_command(argv[2]);
-			else
-				help_pg_probackup();
-		}
-		else if (strcmp(argv[1], "--version") == 0
-				 || strcmp(argv[1], "version") == 0
-				 || strcmp(argv[1], "-V") == 0)
-		{
-#ifdef PGPRO_VERSION
-			fprintf(stdout, "%s %s (Postgres Pro %s %s)\n",
-					PROGRAM_NAME, PROGRAM_VERSION,
-					PGPRO_VERSION, PGPRO_EDITION);
+				launch_ssh(argv);
+				break;
 #else
-			fprintf(stdout, "%s %s (PostgreSQL %s)\n",
-					PROGRAM_NAME, PROGRAM_VERSION, PG_VERSION);
+				elog(ERROR, "\"ssh\" command implemented only for Windows");
 #endif
-			exit(0);
+			case AGENT_CMD:
+				/* 'No forward compatibility' sanity:
+				 *   /old/binary  -> ssh execute -> /newer/binary agent version_num
+				 * If we are executed as an agent for older binary, then exit with error
+				 */
+				if (argc > 2)
+					elog(ERROR, "Version mismatch, pg_probackup binary with version '%s' "
+							"is launched as an agent for pg_probackup binary with version '%s'",
+							PROGRAM_VERSION, argv[2]);
+				fio_communicate(STDIN_FILENO, STDOUT_FILENO);
+				return 0;
+			case HELP_CMD:
+				if (argc > 2)
+				{
+					/* 'pg_probackup help command' style */
+					help_command(parse_subcmd(argv[2]));
+					exit(0);
+				}
+				else
+				{
+					help_pg_probackup();
+					exit(0);
+				}
+				break;
+			case VERSION_CMD:
+				help_print_version();
+				exit(0);
+			case NO_CMD:
+				elog(ERROR, "Unknown subcommand \"%s\"", argv[1]);
+			default:
+				/* Silence compiler warnings */
+				break;
 		}
-		else
-			elog(ERROR, "Unknown subcommand \"%s\"", argv[1]);
 	}
-
-	if (backup_subcmd == NO_CMD)
-		elog(ERROR, "No subcommand specified");
+	else
+		elog(ERROR, "No subcommand specified. Please run with \"help\" argument to see possible subcommands.");
 
 	/*
 	 * Make command string before getopt_long() will call. It permutes the
 	 * content of argv.
 	 */
 	/* TODO why do we do that only for some commands? */
-	command_name = pstrdup(argv[1]);
 	if (backup_subcmd == BACKUP_CMD ||
 		backup_subcmd == RESTORE_CMD ||
 		backup_subcmd == VALIDATE_CMD ||
@@ -450,9 +377,14 @@ main(int argc, char *argv[])
 		show_color = false;
 
 	if (help_opt)
-		help_command(command_name);
+	{
+		/* 'pg_probackup command --help' style */
+		help_command(backup_subcmd);
+		exit(0);
+	}
+
+	setMyLocation(backup_subcmd);
 
-	/* backup_path is required for all pg_probackup commands except help and checkdb */
 	if (backup_path == NULL)
 	{
 		/*
@@ -460,12 +392,8 @@ main(int argc, char *argv[])
 		 * from environment variable
 		 */
 		backup_path = getenv("BACKUP_PATH");
-		if (backup_path == NULL && backup_subcmd != CHECKDB_CMD)
-			elog(ERROR, "required parameter not specified: BACKUP_PATH (-B, --backup-path)");
 	}
 
-	setMyLocation();
-
 	if (backup_path != NULL)
 	{
 		canonicalize_path(backup_path);
@@ -474,11 +402,9 @@ main(int argc, char *argv[])
 		if (!is_absolute_path(backup_path))
 			elog(ERROR, "-B, --backup-path must be an absolute path");
 	}
-
-	/* Ensure that backup_path is an absolute path */
-	if (backup_path && !is_absolute_path(backup_path))
-		elog(ERROR, "-B, --backup-path must be an absolute path");
-
+	/* backup_path is required for all pg_probackup commands except help, version and checkdb */
+	if (backup_path == NULL && backup_subcmd != CHECKDB_CMD && backup_subcmd != HELP_CMD && backup_subcmd != VERSION_CMD)
+		elog(ERROR, "required parameter not specified: BACKUP_PATH (-B, --backup-path)");
 
 	/*
 	 * Option --instance is required for all commands except
@@ -574,7 +500,8 @@ main(int argc, char *argv[])
 			else
 				config_read_opt(path, instance_options, ERROR, true, false);
 		}
-		setMyLocation();
+		/* Зачем второй раз устанавливать? */
+		setMyLocation(backup_subcmd);
 	}
 
 	/*
@@ -676,7 +603,7 @@ main(int argc, char *argv[])
 			backup_subcmd != SET_BACKUP_CMD &&
 			backup_subcmd != SHOW_CMD)
 			elog(ERROR, "Cannot use -i (--backup-id) option together with the \"%s\" command",
-				 command_name);
+				 get_subcmd_name(backup_subcmd));
 
 		current.backup_id = base36dec(backup_id_string);
 		if (current.backup_id == 0)
@@ -709,7 +636,7 @@ main(int argc, char *argv[])
 
 		if (force && backup_subcmd != RESTORE_CMD)
 			elog(ERROR, "You cannot specify \"--force\" flag with the \"%s\" command",
-				command_name);
+				get_subcmd_name(backup_subcmd));
 
 		if (force)
 			no_validate = true;
@@ -779,7 +706,7 @@ main(int argc, char *argv[])
 	/* sanity */
 	if (backup_subcmd == VALIDATE_CMD && restore_params->no_validate)
 		elog(ERROR, "You cannot specify \"--no-validate\" option with the \"%s\" command",
-			command_name);
+			get_subcmd_name(backup_subcmd));
 
 	if (num_threads < 1)
 		num_threads = 1;
@@ -787,7 +714,7 @@ main(int argc, char *argv[])
 	if (batch_size < 1)
 		batch_size = 1;
 
-	compress_init();
+	compress_init(backup_subcmd);
 
 	/* do actual operation */
 	switch (backup_subcmd)
@@ -881,6 +808,13 @@ main(int argc, char *argv[])
 		case NO_CMD:
 			/* Should not happen */
 			elog(ERROR, "Unknown subcommand");
+		case SSH_CMD:
+		case AGENT_CMD:
+			/* Может перейти на использование какого-нибудь do_agent() для однобразия? */
+		case HELP_CMD:
+		case VERSION_CMD:
+			/* Silence compiler warnings, these already handled earlier */
+			break;
 	}
 
 	return 0;
@@ -943,13 +877,13 @@ opt_show_format(ConfigOption *opt, const char *arg)
  * Initialize compress and sanity checks for compress.
  */
 static void
-compress_init(void)
+compress_init(ProbackupSubcmd const subcmd)
 {
 	/* Default algorithm is zlib */
 	if (compress_shortcut)
 		instance_config.compress_alg = ZLIB_COMPRESS;
 
-	if (backup_subcmd != SET_CONFIG_CMD)
+	if (subcmd != SET_CONFIG_CMD)
 	{
 		if (instance_config.compress_level != COMPRESS_LEVEL_DEFAULT
 			&& instance_config.compress_alg == NOT_DEFINED_COMPRESS)
@@ -963,7 +897,7 @@ compress_init(void)
 	if (instance_config.compress_alg == ZLIB_COMPRESS && instance_config.compress_level == 0)
 		elog(WARNING, "Compression level 0 will lead to data bloat!");
 
-	if (backup_subcmd == BACKUP_CMD || backup_subcmd == ARCHIVE_PUSH_CMD)
+	if (subcmd == BACKUP_CMD || subcmd == ARCHIVE_PUSH_CMD)
 	{
 #ifndef HAVE_LIBZ
 		if (instance_config.compress_alg == ZLIB_COMPRESS)

src/pg_probackup.h

@@ -876,8 +876,9 @@ extern char *slurpFile(const char *datadir,
 extern char *fetchFile(PGconn *conn, const char *filename, size_t *filesize);
 
 /* in help.c */
+extern void help_print_version(void);
 extern void help_pg_probackup(void);
-extern void help_command(char *command);
+extern void help_command(ProbackupSubcmd const subcmd);
 
 /* in validate.c */
 extern void pgBackupValidate(pgBackup* backup, pgRestoreParams *params);
@@ -1162,6 +1163,7 @@ extern int send_pages(ConnectionArgs* conn_arg, const char *to_fullpath, const c
 					  BackupMode backup_mode, int ptrack_version_num, const char *ptrack_schema);
 
 /* FIO */
+extern void setMyLocation(ProbackupSubcmd const subcmd);
 extern void fio_delete(mode_t mode, const char *fullpath, fio_location location);
 extern int fio_send_pages(const char *to_fullpath, const char *from_fullpath, pgFile *file,
 	                      XLogRecPtr horizonLsn, int calg, int clevel, uint32 checksum_version,

src/utils/configuration.c

@@ -87,6 +87,62 @@ static const unit_conversion time_unit_conversion_table[] =
 	{""}						/* end of table marker */
 };
 
+/* Order is important, keep it in sync with utils/configuration.h:enum ProbackupSubcmd declaration */
+static char const * const subcmd_names[] =
+{
+	"NO_CMD",
+	"init",
+	"add-instance",
+	"del-instance",
+	"archive-push",
+	"archive-get",
+	"backup",
+	"restore",
+	"validate",
+	"delete",
+	"merge",
+	"show",
+	"set-config",
+	"set-backup",
+	"show-config",
+	"checkdb",
+	"ssh",
+	"agent",
+	"help",
+	"version",
+};
+
+ProbackupSubcmd
+parse_subcmd(char const * const subcmd_str)
+{
+	struct {
+		ProbackupSubcmd id;
+		char *name;
+	}
+	static const subcmd_additional_names[] = {
+		{ HELP_CMD, "--help" },
+		{ HELP_CMD, "-?" },
+		{ VERSION_CMD, "--version" },
+		{ VERSION_CMD, "-V" },
+	};
+
+	int i;
+	for(i = (int)NO_CMD + 1; i < sizeof(subcmd_names) / sizeof(subcmd_names[0]); ++i)
+		if(strcmp(subcmd_str, subcmd_names[i]) == 0)
+			return (ProbackupSubcmd)i;
+	for(i = 0; i < sizeof(subcmd_additional_names) / sizeof(subcmd_additional_names[0]); ++i)
+		if(strcmp(subcmd_str, subcmd_additional_names[i].name) == 0)
+			return subcmd_additional_names[i].id;
+	return NO_CMD;
+}
+
+char const *
+get_subcmd_name(ProbackupSubcmd const subcmd)
+{
+	Assert((int)subcmd < sizeof(subcmd_names) / sizeof(subcmd_names[0]));
+	return subcmd_names[(int)subcmd];
+}
+
 /*
  * Reading functions.
  */

src/utils/configuration.h

@@ -16,6 +16,31 @@
 
 #define INFINITE_STR		"INFINITE"
 
+/* Order is important, keep it in sync with configuration.c:subcmd_names[] and help.c:help_command() */
+typedef enum ProbackupSubcmd
+{
+	NO_CMD = 0,
+	INIT_CMD,
+	ADD_INSTANCE_CMD,
+	DELETE_INSTANCE_CMD,
+	ARCHIVE_PUSH_CMD,
+	ARCHIVE_GET_CMD,
+	BACKUP_CMD,
+	RESTORE_CMD,
+	VALIDATE_CMD,
+	DELETE_CMD,
+	MERGE_CMD,
+	SHOW_CMD,
+	SET_CONFIG_CMD,
+	SET_BACKUP_CMD,
+	SHOW_CONFIG_CMD,
+	CHECKDB_CMD,
+	SSH_CMD,
+	AGENT_CMD,
+	HELP_CMD,
+	VERSION_CMD
+} ProbackupSubcmd;
+
 typedef enum OptionSource
 {
 	SOURCE_DEFAULT,
@@ -75,6 +100,8 @@ struct ConfigOption
 
 #define OPTION_UNIT					(OPTION_UNIT_MEMORY | OPTION_UNIT_TIME)
 
+extern ProbackupSubcmd parse_subcmd(char const * const subcmd_str);
+extern char const *get_subcmd_name(ProbackupSubcmd const subcmd);
 extern int config_get_opt(int argc, char **argv, ConfigOption cmd_options[],
 						  ConfigOption options[]);
 extern int config_read_opt(const char *path, ConfigOption options[], int elevel,

src/utils/file.c

@@ -83,6 +83,24 @@ typedef struct
 #undef fopen(a, b)
 #endif
 
+void
+setMyLocation(ProbackupSubcmd const subcmd)
+{
+
+#ifdef WIN32
+	if (IsSshProtocol())
+		elog(ERROR, "Currently remote operations on Windows are not supported");
+#endif
+
+	MyLocation = IsSshProtocol()
+		? (subcmd == ARCHIVE_PUSH_CMD || subcmd == ARCHIVE_GET_CMD)
+		   ? FIO_DB_HOST
+		   : (subcmd == BACKUP_CMD || subcmd == RESTORE_CMD || subcmd == ADD_INSTANCE_CMD)
+		      ? FIO_BACKUP_HOST
+		      : FIO_LOCAL_HOST
+		: FIO_LOCAL_HOST;
+}
+
 /* Use specified file descriptors as stdin/stdout for FIO functions */
 void fio_redirect(int in, int out, int err)
 {