Skip to content

Commit

Permalink
Improve options and arguments parsing. (#100)
Browse files Browse the repository at this point in the history 
* Improve options and arguments parsing.

Our nested command and sub-commands parsing is not compatible with the
default GNU getopt behavior where they will re-order the command line so
that users are free to mix options and arguments as they want.

When we are on the final sub-command though, we could re-enable the default
behavior of GNU getopt_long for more flexibility to the user. That's what we
do with this patch, making it possible to use either form for the same
command as in the following example:

  $ pg_autoctl -q config get --pgdata /tmp/sm/a postgresql.port
  3001

  $ pg_autoctl -q config get postgresql.port --pgdata /tmp/sm/a
  3001

Without this patch, only the first case works.

As most of our commands only process options, only two places need to be
adjusted to make that behavior available. The other commands don't need this
at the moment.

* Implement --version, --verbose, and --quiet on every final subcommand.

That way users don't have to remember to use those options only after the
main program name and before any subcommand, it just works everywhere (or
almost) in the command line:

  pg_autoctl -q config get --pgdata /tmp/sm/a postgresql.port
  pg_autoctl config get --pgdata /tmp/sm/a postgresql.port -q

That's now implemented for all our commands.

* Fix --help options in all commands.

This require changing the --pghost short option letter to -H, but as it's
not documented anyway, it should be ok for us to reclaim -h for --help.
  • Loading branch information
@DimCitus
DimCitus committed Nov 12, 2019
1 parent 215e60e commit 319bc3a
Show file tree
Hide file tree
Showing 13 changed files with 611 additions and 68 deletions.
3 changes: 2 additions & 1 deletion src/bin/lib/subcommands.c/commandline.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ commandline_run(CommandLine *command, int argc, char **argv)
return;
}

current_command = command;

/* Otherwise let the command parse any options that occur here. */
if (command->getopt != NULL)
{
Expand All @@ -59,7 +61,6 @@ commandline_run(CommandLine *command, int argc, char **argv)

if (command->run != NULL)
{
current_command = command;
return command->run(argc, argv);
}
else if (argc == 0)
Expand Down
158 changes: 151 additions & 7 deletions src/bin/pg_autoctl/cli_common.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@
#include <inttypes.h>
#include <getopt.h>

#include "commandline.h"

#include "cli_common.h"
#include "cli_root.h"
#include "commandline.h"
#include "ipaddr.h"
#include "keeper.h"
Expand All @@ -35,19 +38,22 @@ bool allowRemovingPgdata = false;
* static struct option long_options[] = {
* { "pgctl", required_argument, NULL, 'C' },
* { "pgdata", required_argument, NULL, 'D' },
* { "pghost", required_argument, NULL, 'h' },
* { "pghost", required_argument, NULL, 'H' },
* { "pgport", required_argument, NULL, 'p' },
* { "listen", required_argument, NULL, 'l' },
* { "proxyport", required_argument, NULL, 'y' },
* { "username", required_argument, NULL, 'U' },
{ "auth", required_argument, NULL, 'A' },
* { "auth", required_argument, NULL, 'A' },
* { "dbname", required_argument, NULL, 'd' },
* { "nodename", required_argument, NULL, 'n' },
* { "formation", required_argument, NULL, 'f' },
* { "group", required_argument, NULL, 'g' },
* { "monitor", required_argument, NULL, 'm' },
* { "allow-removing-pgdata", no_argument, NULL, 'R' },
* { "help", no_argument, NULL, 0 },
* { "version", no_argument, NULL, 'V' },
* { "verbose", no_argument, NULL, 'v' },
* { "quiet", no_argument, NULL, 'q' },
* { "help", no_argument, NULL, 'h' },
* { NULL, 0, NULL, 0 }
* };
*
Expand All @@ -60,6 +66,7 @@ cli_create_node_getopts(int argc, char **argv,
{
KeeperConfig LocalOptionConfig = { 0 };
int c, option_index, errors = 0;
int verboseCount = 0;

/* force some non-zero default values */
LocalOptionConfig.groupId = -1;
Expand Down Expand Up @@ -98,7 +105,7 @@ cli_create_node_getopts(int argc, char **argv,
break;
}

case 'h':
case 'H':
{
/* { "pghost", required_argument, NULL, 'h' } */
strlcpy(LocalOptionConfig.pgSetup.pghost, optarg, _POSIX_HOST_NAME_MAX);
Expand Down Expand Up @@ -217,6 +224,46 @@ cli_create_node_getopts(int argc, char **argv,
break;
}

case 'V':
{
/* keeper_cli_print_version prints version and exits. */
keeper_cli_print_version(argc, argv);
break;
}

case 'v':
{
++verboseCount;
switch (verboseCount)
{
case 1:
log_set_level(LOG_INFO);
break;

case 2:
log_set_level(LOG_DEBUG);
break;

default:
log_set_level(LOG_TRACE);
break;
}
break;
}

case 'q':
{
log_set_level(LOG_ERROR);
break;
}

case 'h':
{
commandline_help(stderr);
exit(EXIT_CODE_QUIT);
break;
}

default:
{
/* getopt_long already wrote an error message */
Expand Down Expand Up @@ -266,16 +313,30 @@ int
keeper_cli_getopt_pgdata(int argc, char **argv)
{
KeeperConfig options = { 0 };
int c, option_index = 0;
int c, option_index = 0, errors = 0;
int verboseCount = 0;

static struct option long_options[] = {
{ "pgdata", required_argument, NULL, 'D' },
{ "version", no_argument, NULL, 'V' },
{ "verbose", no_argument, NULL, 'v' },
{ "quiet", no_argument, NULL, 'q' },
{ "help", no_argument, NULL, 'h' },
{ NULL, 0, NULL, 0 }
};
optind = 0;

while ((c = getopt_long(argc, argv, "D:",
/*
* The only command lines that are using keeper_cli_getopt_pgdata are
* terminal ones: they don't accept subcommands. In that case our option
* parsing can happen in any order and we don't need getopt_long to behave
* in a POSIXLY_CORRECT way.
*
* The unsetenv() call allows getopt_long() to reorder arguments for us.
*/
unsetenv("POSIXLY_CORRECT");

while ((c = getopt_long(argc, argv, "D:Vvqh",
long_options, &option_index)) != -1)
{
switch (c)
Expand All @@ -286,9 +347,62 @@ keeper_cli_getopt_pgdata(int argc, char **argv)
log_trace("--pgdata %s", options.pgSetup.pgdata);
break;
}

case 'V':
{
/* keeper_cli_print_version prints version and exits. */
keeper_cli_print_version(argc, argv);
break;
}

case 'v':
{
++verboseCount;
switch (verboseCount)
{
case 1:
log_set_level(LOG_INFO);
break;

case 2:
log_set_level(LOG_DEBUG);
break;

default:
log_set_level(LOG_TRACE);
break;
}
break;
}

case 'q':
{
log_set_level(LOG_ERROR);
break;
}

case 'h':
{
commandline_help(stderr);
exit(EXIT_CODE_QUIT);
break;
}

default:
{
/* getopt_long already wrote an error message */
errors++;
break;
}
}
}

if (errors > 0)
{
commandline_help(stderr);
exit(EXIT_CODE_BAD_ARGS);
}

if (IS_EMPTY_STRING_BUFFER(options.pgSetup.pgdata))
{
char *pgdata = getenv("PGDATA");
Expand All @@ -306,7 +420,8 @@ keeper_cli_getopt_pgdata(int argc, char **argv)
log_info("Managing PostgreSQL installation at \"%s\"",
options.pgSetup.pgdata);

if (!keeper_config_set_pathnames_from_pgdata(&options.pathnames, options.pgSetup.pgdata))
if (!keeper_config_set_pathnames_from_pgdata(&options.pathnames,
options.pgSetup.pgdata))
{
/* errors have already been logged */
exit(EXIT_CODE_BAD_ARGS);
Expand Down Expand Up @@ -513,3 +628,32 @@ exit_unless_role_is_keeper(KeeperConfig *kconfig)
}
}
}


/*
* Provide help.
*/
void
keeper_cli_help(int argc, char **argv)
{
CommandLine command = root;

if (getenv(PG_AUTOCTL_DEBUG) != NULL)
{
command = root_with_debug;
}

(void) commandline_print_command_tree(&command, stdout);
}


/*
* keeper_cli_print_version prints the pg_autoctl version and exits with
* successful exit code of zero.
*/
void
keeper_cli_print_version(int argc, char **argv)
{
fprintf(stdout, "pg_autoctl version %s\n", PG_AUTOCTL_VERSION);
exit(0);
}
5 changes: 3 additions & 2 deletions src/bin/pg_autoctl/cli_common.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ extern CommandLine show_state_command;
extern CommandLine systemd_cat_service_file_command;


void keeper_cli_help(int argc, char **argv);
void keeper_cli_print_version(int argc, char **argv);

int cli_create_node_getopts(int argc, char **argv,
struct option *long_options,
const char *optstring,
Expand All @@ -105,6 +108,4 @@ bool cli_create_config(Keeper *keeper, KeeperConfig *config);
void cli_create_pg(Keeper *keeper, KeeperConfig *config);
bool check_or_discover_nodename(KeeperConfig *config);



#endif /* CLI_COMMON_H */
64 changes: 59 additions & 5 deletions src/bin/pg_autoctl/cli_create_drop_node.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ cli_create_postgres_getopts(int argc, char **argv)
static struct option long_options[] = {
{ "pgctl", required_argument, NULL, 'C' },
{ "pgdata", required_argument, NULL, 'D' },
{ "pghost", required_argument, NULL, 'h' },
{ "pghost", required_argument, NULL, 'H' },
{ "pgport", required_argument, NULL, 'p' },
{ "listen", required_argument, NULL, 'l' },
{ "username", required_argument, NULL, 'U' },
Expand All @@ -203,13 +203,16 @@ cli_create_postgres_getopts(int argc, char **argv)
{ "formation", required_argument, NULL, 'f' },
{ "monitor", required_argument, NULL, 'm' },
{ "allow-removing-pgdata", no_argument, NULL, 'R' },
{ "help", no_argument, NULL, 0 },
{ "version", no_argument, NULL, 'V' },
{ "verbose", no_argument, NULL, 'v' },
{ "quiet", no_argument, NULL, 'q' },
{ "help", no_argument, NULL, 'h' },
{ NULL, 0, NULL, 0 }
};

int optind =
cli_create_node_getopts(argc, argv,
long_options, "C:D:h:p:l:U:A:d:n:f:m:R",
long_options, "C:D:H:p:l:U:A:d:n:f:m:RVvqh",
&options);

/* publish our option parsing in the global variable */
Expand Down Expand Up @@ -258,6 +261,7 @@ cli_create_monitor_getopts(int argc, char **argv)
{
MonitorConfig options = { 0 };
int c, option_index, errors = 0;
int verboseCount = 0;

static struct option long_options[] = {
{ "pgctl", required_argument, NULL, 'C' },
Expand All @@ -266,7 +270,10 @@ cli_create_monitor_getopts(int argc, char **argv)
{ "nodename", required_argument, NULL, 'n' },
{ "listen", required_argument, NULL, 'l' },
{ "auth", required_argument, NULL, 'A' },
{ "help", no_argument, NULL, 0 },
{ "version", no_argument, NULL, 'V' },
{ "verbose", no_argument, NULL, 'v' },
{ "quiet", no_argument, NULL, 'q' },
{ "help", no_argument, NULL, 'h' },
{ NULL, 0, NULL, 0 }
};

Expand All @@ -275,7 +282,7 @@ cli_create_monitor_getopts(int argc, char **argv)

optind = 0;

while ((c = getopt_long(argc, argv, "C:D:p:A:",
while ((c = getopt_long(argc, argv, "C:D:p:A:Vvqh",
long_options, &option_index)) != -1)
{
switch (c)
Expand Down Expand Up @@ -327,6 +334,47 @@ cli_create_monitor_getopts(int argc, char **argv)
log_trace("--auth %s", options.pgSetup.authMethod);
break;
}

case 'V':
{
/* keeper_cli_print_version prints version and exits. */
keeper_cli_print_version(argc, argv);
break;
}

case 'v':
{
++verboseCount;
switch (verboseCount)
{
case 1:
log_set_level(LOG_INFO);
break;

case 2:
log_set_level(LOG_DEBUG);
break;

default:
log_set_level(LOG_TRACE);
break;
}
break;
}

case 'q':
{
log_set_level(LOG_ERROR);
break;
}

case 'h':
{
commandline_help(stderr);
exit(EXIT_CODE_QUIT);
break;
}

default:
{
/* getopt_long already wrote an error message */
Expand All @@ -336,6 +384,12 @@ cli_create_monitor_getopts(int argc, char **argv)
}
}

if (errors > 0)
{
commandline_help(stderr);
exit(EXIT_CODE_BAD_ARGS);
}

/*
* We're not using pg_setup_init() here: we are following a very different
* set of rules. We just want to check:
Expand Down

0 comments on commit 319bc3a

Please sign in to comment.