From a96f478a431311d749632e39338cfd665a53964e Mon Sep 17 00:00:00 2001 From: Ian Barwick Date: Wed, 27 Jan 2016 09:10:19 +0900 Subject: [PATCH] Allow negative values in configuration parameters, where appropriate. Make the code match the documentation. As pointed out by GitHub user phyber (#142). Also various other minor improvements to error reporting during config file parsing. --- config.c | 59 +++++++++++++++++++++++++------------------------------- config.h | 3 ++- repmgr.c | 8 ++++---- 3 files changed, 32 insertions(+), 38 deletions(-) diff --git a/config.c b/config.c index 52600c5d..46ab9c25 100644 --- a/config.c +++ b/config.c @@ -198,11 +198,13 @@ parse_config(t_configuration_options *options) /* For sanity-checking provided conninfo string */ PQconninfoOption *conninfo_options; - char *conninfo_errmsg = NULL; + char *conninfo_errmsg = NULL; /* Collate configuration file errors here for friendlier reporting */ static ErrorList config_errors = { NULL, NULL }; + bool node_found = false; + /* Initialize configuration options with sensible defaults * note: the default log level is set in log.c and does not need * to be initialised here @@ -290,9 +292,12 @@ parse_config(t_configuration_options *options) if (strcmp(name, "cluster") == 0) strncpy(options->cluster_name, value, MAXLEN); else if (strcmp(name, "node") == 0) - options->node = repmgr_atoi(value, "node", &config_errors); + { + options->node = repmgr_atoi(value, "node", &config_errors, false); + node_found = true; + } else if (strcmp(name, "upstream_node") == 0) - options->upstream_node = repmgr_atoi(value, "upstream_node", &config_errors); + options->upstream_node = repmgr_atoi(value, "upstream_node", &config_errors, false); else if (strcmp(name, "conninfo") == 0) strncpy(options->conninfo, value, MAXLEN); else if (strcmp(name, "rsync_options") == 0) @@ -323,7 +328,7 @@ parse_config(t_configuration_options *options) } } else if (strcmp(name, "priority") == 0) - options->priority = repmgr_atoi(value, "priority", &config_errors); + options->priority = repmgr_atoi(value, "priority", &config_errors, true); else if (strcmp(name, "node_name") == 0) strncpy(options->node_name, value, MAXLEN); else if (strcmp(name, "promote_command") == 0) @@ -331,16 +336,16 @@ parse_config(t_configuration_options *options) else if (strcmp(name, "follow_command") == 0) strncpy(options->follow_command, value, MAXLEN); else if (strcmp(name, "master_response_timeout") == 0) - options->master_response_timeout = repmgr_atoi(value, "master_response_timeout", &config_errors); + options->master_response_timeout = repmgr_atoi(value, "master_response_timeout", &config_errors, false); /* 'primary_response_timeout' as synonym for 'master_response_timeout' - * we'll switch terminology in a future release (3.1?) */ else if (strcmp(name, "primary_response_timeout") == 0) - options->master_response_timeout = repmgr_atoi(value, "primary_response_timeout", &config_errors); + options->master_response_timeout = repmgr_atoi(value, "primary_response_timeout", &config_errors, false); else if (strcmp(name, "reconnect_attempts") == 0) - options->reconnect_attempts = repmgr_atoi(value, "reconnect_attempts", &config_errors); + options->reconnect_attempts = repmgr_atoi(value, "reconnect_attempts", &config_errors, false); else if (strcmp(name, "reconnect_interval") == 0) - options->reconnect_interval = repmgr_atoi(value, "reconnect_interval", &config_errors); + options->reconnect_interval = repmgr_atoi(value, "reconnect_interval", &config_errors, false); else if (strcmp(name, "pg_bindir") == 0) strncpy(options->pg_bindir, value, MAXLEN); else if (strcmp(name, "pg_ctl_options") == 0) @@ -350,12 +355,12 @@ parse_config(t_configuration_options *options) else if (strcmp(name, "logfile") == 0) strncpy(options->logfile, value, MAXLEN); else if (strcmp(name, "monitor_interval_secs") == 0) - options->monitor_interval_secs = repmgr_atoi(value, "monitor_interval_secs", &config_errors); + options->monitor_interval_secs = repmgr_atoi(value, "monitor_interval_secs", &config_errors, false); else if (strcmp(name, "retry_promote_interval_secs") == 0) - options->retry_promote_interval_secs = repmgr_atoi(value, "retry_promote_interval_secs", &config_errors); + options->retry_promote_interval_secs = repmgr_atoi(value, "retry_promote_interval_secs", &config_errors, false); else if (strcmp(name, "use_replication_slots") == 0) /* XXX we should have a dedicated boolean argument format */ - options->use_replication_slots = repmgr_atoi(value, "use_replication_slots", &config_errors); + options->use_replication_slots = repmgr_atoi(value, "use_replication_slots", &config_errors, false); else if (strcmp(name, "event_notification_command") == 0) strncpy(options->event_notification_command, value, MAXLEN); else if (strcmp(name, "event_notifications") == 0) @@ -387,29 +392,17 @@ parse_config(t_configuration_options *options) fclose(fp); - /* Check config settings */ - /* The following checks are for the presence of the parameter */ - if (*options->cluster_name == '\0') + if (node_found == false) { - error_list_append(&config_errors, _("\"cluster\": parameter was not found\n")); + error_list_append(&config_errors, _("\"node\": parameter was not found")); + } + else if (options->node == 0) + { + error_list_append(&config_errors, _("\"node\": must be greater than zero")); } - if (options->node == -1) - { - error_list_append(&config_errors, _("\"node\": parameter was not found\n")); - } - - if (*options->node_name == '\0') - { - error_list_append(&config_errors, _("\"node_name\": parameter was not found\n")); - } - - if (*options->conninfo == '\0') - { - error_list_append(&config_errors, _("\"conninfo\": parameter was not found\n")); - } - else + if (strlen(options->conninfo)) { /* Sanity check the provided conninfo string @@ -791,7 +784,7 @@ error_list_append(ErrorList *error_list, char *error_message) * otherwise exit */ int -repmgr_atoi(const char *value, const char *config_item, ErrorList *error_list) +repmgr_atoi(const char *value, const char *config_item, ErrorList *error_list, bool allow_negative) { char *endptr; long longval = 0; @@ -822,8 +815,8 @@ repmgr_atoi(const char *value, const char *config_item, ErrorList *error_list) } } - /* Currently there are no values which could be negative */ - if (longval < 0) + /* Disallow negative values for most parameters */ + if (allow_negative == false && longval < 0) { snprintf(error_message_buf, MAXLEN, diff --git a/config.h b/config.h index 34db6037..3d65637f 100644 --- a/config.h +++ b/config.h @@ -106,6 +106,7 @@ char *trim(char *s); void error_list_append(ErrorList *error_list, char *error_message); int repmgr_atoi(const char *s, const char *config_item, - ErrorList *error_list); + ErrorList *error_list, + bool allow_negative); #endif diff --git a/repmgr.c b/repmgr.c index 2a9ed7a9..458c315c 100644 --- a/repmgr.c +++ b/repmgr.c @@ -263,7 +263,7 @@ main(int argc, char **argv) connection_param_provided = true; break; case 'p': - repmgr_atoi(optarg, "-p/--port", &cli_errors); + repmgr_atoi(optarg, "-p/--port", &cli_errors, false); strncpy(runtime_options.masterport, optarg, MAXLEN); @@ -281,7 +281,7 @@ main(int argc, char **argv) break; case 'l': /* -l/--local-port is deprecated */ - repmgr_atoi(optarg, "-l/--local-port", &cli_errors); + repmgr_atoi(optarg, "-l/--local-port", &cli_errors, false); strncpy(runtime_options.localport, optarg, MAXLEN); @@ -293,14 +293,14 @@ main(int argc, char **argv) strncpy(runtime_options.remote_user, optarg, MAXLEN); break; case 'w': - repmgr_atoi(optarg, "-w/--wal-keep-segments", &cli_errors); + repmgr_atoi(optarg, "-w/--wal-keep-segments", &cli_errors, false); strncpy(runtime_options.wal_keep_segments, optarg, MAXLEN); wal_keep_segments_used = true; break; case 'k': - runtime_options.keep_history = repmgr_atoi(optarg, "-k/--keep-history", &cli_errors); + runtime_options.keep_history = repmgr_atoi(optarg, "-k/--keep-history", &cli_errors, false); break; case 'F': runtime_options.force = true;