From c687d0d670562f921f2e8197bc3634976138a27e Mon Sep 17 00:00:00 2001 From: Gabriele Bartolini Date: Thu, 16 Dec 2010 22:13:30 +0100 Subject: [PATCH 1/5] Reviewed the code, fixed a small bug with option name detection --- config.c | 55 +++++++++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/config.c b/config.c index 4d15f0b3..9304900c 100644 --- a/config.c +++ b/config.c @@ -10,35 +10,36 @@ void parse_config(const char *config_file, char *cluster_name, int *node, char *conninfo) { - char *s, buff[256]; + char *s, buff[MAXLEN]; + char name[MAXLEN]; + char value[MAXLEN]; + FILE *fp = fopen (config_file, "r"); if (fp == NULL) - return; + return; /* Read next line */ while ((s = fgets (buff, sizeof buff, fp)) != NULL) { - char name[MAXLEN]; - char value[MAXLEN]; + /* Skip blank lines and comments */ + if (buff[0] == '\n' || buff[0] == '#') + continue; - /* Skip blank lines and comments */ - if (buff[0] == '\n' || buff[0] == '#') - continue; - - /* Parse name/value pair from line */ + /* Parse name/value pair from line */ parse_line(buff, name, value); - /* Copy into correct entry in parameters struct */ - if (strcmp(name, "cluster") == 0) - strncpy (cluster_name, value, MAXLEN); - else if (strcmp(name, "node") == 0) - *node = atoi(value); - else if (strcmp(name, "conninfo") == 0) - strncpy (conninfo, value, MAXLEN); - else - printf ("WARNING: %s/%s: Unknown name/value pair!\n", name, value); - } + /* Copy into correct entry in parameters struct */ + if (strncmp(name, "cluster", 7) == 0) + strncpy (cluster_name, value, MAXLEN); + else if (strncmp(name, "node", 4) == 0) + *node = atoi(value); + else if (strncmp(name, "conninfo", 8) == 0) + strncpy (conninfo, value, MAXLEN); + else + printf ("WARNING: %s/%s: Unknown name/value pair!\n", name, value); + } + /* Close file */ fclose (fp); @@ -52,12 +53,12 @@ trim (char *s) /* Trim and delimit right side */ while ( (isspace (*s2)) && (s2 >= s1) ) - s2--; + --s2; *(s2+1) = '\0'; /* Trim left side */ while ( (isspace (*s1)) && (s1 < s2) ) - s1++; + ++s1; /* Copy finished string */ strcpy (s, s1); @@ -67,14 +68,13 @@ trim (char *s) void parse_line(char *buff, char *name, char *value) { - int i; - int j; + int i = 0; + int j = 0; /* * first we find the name of the parameter */ - j = 0; - for (i = 0; i < MAXLEN; i++) + for ( ; i < MAXLEN; ++i) { if (buff[i] != '=') name[j++] = buff[i]; @@ -83,12 +83,11 @@ parse_line(char *buff, char *name, char *value) } name[j] = '\0'; - i++; /* * Now the value */ j = 0; - for ( ; i < MAXLEN; i++) + for ( ++i ; i < MAXLEN; i++) if (buff[i] == '\'') continue; else if (buff[i] != '\n') @@ -96,5 +95,5 @@ parse_line(char *buff, char *name, char *value) else break; value[j] = '\0'; - trim(value); + trim(value); } From 763a1e8b3d834b72b3b3910f2d1d99dd9bb54919 Mon Sep 17 00:00:00 2001 From: Gabriele Bartolini Date: Thu, 16 Dec 2010 22:14:18 +0100 Subject: [PATCH 2/5] Reviewed the code, fixed a small bug with option name detection --- config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config.c b/config.c index 9304900c..4052dd7e 100644 --- a/config.c +++ b/config.c @@ -87,7 +87,7 @@ parse_line(char *buff, char *name, char *value) * Now the value */ j = 0; - for ( ++i ; i < MAXLEN; i++) + for ( ++i ; i < MAXLEN; ++i) if (buff[i] == '\'') continue; else if (buff[i] != '\n') From 05e88a2cc8ea0878f226d8064cbc8996c86fbdba Mon Sep 17 00:00:00 2001 From: Gabriele Bartolini Date: Thu, 16 Dec 2010 22:21:27 +0100 Subject: [PATCH 3/5] removed strncmp improper usage, initialise values asap --- config.c | 6 +++--- dbutils.c | 34 +++++++++++++++++----------------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/config.c b/config.c index 4052dd7e..3bedcaff 100644 --- a/config.c +++ b/config.c @@ -30,11 +30,11 @@ parse_config(const char *config_file, char *cluster_name, int *node, char *conni parse_line(buff, name, value); /* Copy into correct entry in parameters struct */ - if (strncmp(name, "cluster", 7) == 0) + if (strcmp(name, "cluster") == 0) strncpy (cluster_name, value, MAXLEN); - else if (strncmp(name, "node", 4) == 0) + else if (strcmp(name, "node") == 0) *node = atoi(value); - else if (strncmp(name, "conninfo", 8) == 0) + else if (strcmp(name, "conninfo") == 0) strncpy (conninfo, value, MAXLEN); else printf ("WARNING: %s/%s: Unknown name/value pair!\n", name, value); diff --git a/dbutils.c b/dbutils.c index 8287e1b0..96a7111d 100644 --- a/dbutils.c +++ b/dbutils.c @@ -11,20 +11,20 @@ PGconn * establishDBConnection(const char *conninfo, const bool exit_on_error) { - PGconn *conn; - /* Make a connection to the database */ - conn = PQconnectdb(conninfo); - /* Check to see that the backend connection was successfully made */ - if ((PQstatus(conn) != CONNECTION_OK)) - { - fprintf(stderr, "Connection to database failed: %s", + /* Make a connection to the database */ + PGconn *conn = PQconnectdb(conninfo); + + /* Check to see that the backend connection was successfully made */ + if ((PQstatus(conn) != CONNECTION_OK)) + { + fprintf(stderr, "Connection to database failed: %s", PQerrorMessage(conn)); if (exit_on_error) { - PQfinish(conn); + PQfinish(conn); exit(1); } - } + } return conn; } @@ -34,17 +34,17 @@ establishDBConnection(const char *conninfo, const bool exit_on_error) bool is_standby(PGconn *conn) { - PGresult *res; + PGresult *res; bool result; - res = PQexec(conn, "SELECT pg_is_in_recovery()"); - if (PQresultStatus(res) != PGRES_TUPLES_OK) - { - fprintf(stderr, "Can't query server mode: %s", PQerrorMessage(conn)); - PQclear(res); - PQfinish(conn); + res = PQexec(conn, "SELECT pg_is_in_recovery()"); + if (PQresultStatus(res) != PGRES_TUPLES_OK) + { + fprintf(stderr, "Can't query server mode: %s", PQerrorMessage(conn)); + PQclear(res); + PQfinish(conn); exit(1); - } + } if (strcmp(PQgetvalue(res, 0, 0), "f") == 0) result = false; From f2bec9a08f99ee06b9efaebf2b4de14a1deee303 Mon Sep 17 00:00:00 2001 From: Gabriele Bartolini Date: Thu, 16 Dec 2010 22:31:26 +0100 Subject: [PATCH 4/5] Some cosmetic changes --- config.c | 4 +++- dbutils.c | 14 +++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/config.c b/config.c index 3bedcaff..63f4b2d1 100644 --- a/config.c +++ b/config.c @@ -7,10 +7,12 @@ #include "repmgr.h" +#define MAXLINELENGTH 4096 + void parse_config(const char *config_file, char *cluster_name, int *node, char *conninfo) { - char *s, buff[MAXLEN]; + char *s, buff[MAXLINELENGTH]; char name[MAXLEN]; char value[MAXLEN]; diff --git a/dbutils.c b/dbutils.c index 96a7111d..626c9b67 100644 --- a/dbutils.c +++ b/dbutils.c @@ -8,6 +8,9 @@ #include "repmgr.h" +#define MAXQUERY 8192 +#define MAXCONNINFO 1024 + PGconn * establishDBConnection(const char *conninfo, const bool exit_on_error) { @@ -82,6 +85,7 @@ pg_version(PGconn *conn) major_version2 = PQgetvalue(res, 0, 1); PQclear(res); + /* FIX: this is never deallocated */ major_version = malloc(10); if (major_version1 >= 9) { @@ -99,7 +103,7 @@ bool guc_setted(PGconn *conn, const char *parameter, const char *op, const char *value) { PGresult *res; - char sqlquery[8192]; + char sqlquery[MAXQUERY]; sprintf(sqlquery, "SELECT true FROM pg_settings " " WHERE name = '%s' AND setting %s '%s'", @@ -129,7 +133,7 @@ get_cluster_size(PGconn *conn) { PGresult *res; const char *size; - char sqlquery[8192]; + char sqlquery[MAXQUERY]; sprintf(sqlquery, "SELECT pg_size_pretty(SUM(pg_database_size(oid))::bigint) " " FROM pg_database "); @@ -157,8 +161,8 @@ getMasterConnection(PGconn *standby_conn, int id, char *cluster, int *master_id) PGconn *master_conn = NULL; PGresult *res1; PGresult *res2; - char sqlquery[8192]; - char master_conninfo[8192]; + char sqlquery[MAXQUERY]; + char master_conninfo[MAXCONNINFO]; int i; /* find all nodes belonging to this cluster */ @@ -179,7 +183,7 @@ getMasterConnection(PGconn *standby_conn, int id, char *cluster, int *master_id) { /* initialize with the values of the current node being processed */ *master_id = atoi(PQgetvalue(res1, i, 0)); - strcpy(master_conninfo, PQgetvalue(res1, i, 2)); + strncpy(master_conninfo, PQgetvalue(res1, i, 2), MAXCONNINFO); master_conn = establishDBConnection(master_conninfo, false); if (PQstatus(master_conn) != CONNECTION_OK) continue; From d88783a4d9b3b2cbc8ba66558b89d9f2fb52fd2e Mon Sep 17 00:00:00 2001 From: Gabriele Bartolini Date: Thu, 16 Dec 2010 22:48:19 +0100 Subject: [PATCH 5/5] Changed pg_version() prototype in order to remove the small memory leak --- dbutils.c | 7 ++----- dbutils.h | 2 +- repmgr.c | 28 ++++++++++++++-------------- repmgr.h | 1 + repmgrd.c | 4 ++-- 5 files changed, 20 insertions(+), 22 deletions(-) diff --git a/dbutils.c b/dbutils.c index 626c9b67..a9b38307 100644 --- a/dbutils.c +++ b/dbutils.c @@ -64,10 +64,9 @@ is_standby(PGconn *conn) * if 8 or inferior returns an empty string */ char * -pg_version(PGconn *conn) +pg_version(PGconn *conn, char* major_version) { PGresult *res; - char *major_version; int major_version1; char *major_version2; @@ -85,12 +84,10 @@ pg_version(PGconn *conn) major_version2 = PQgetvalue(res, 0, 1); PQclear(res); - /* FIX: this is never deallocated */ - major_version = malloc(10); if (major_version1 >= 9) { /* form a major version string */ - sprintf(major_version, "%d.%s", major_version1, major_version2); + snprintf(major_version, MAXVERSIONSTR, "%d.%s", major_version1, major_version2); } else strcpy(major_version, ""); diff --git a/dbutils.h b/dbutils.h index a1f96a3f..3bbbca67 100644 --- a/dbutils.h +++ b/dbutils.h @@ -6,7 +6,7 @@ PGconn *establishDBConnection(const char *conninfo, const bool exit_on_error); bool is_standby(PGconn *conn); -char *pg_version(PGconn *conn); +char *pg_version(PGconn *conn, char* major_version); bool guc_setted(PGconn *conn, const char *parameter, const char *op, const char *value); const char *get_cluster_size(PGconn *conn); PGconn * getMasterConnection(PGconn *standby_conn, int id, char *cluster, int *master_id); diff --git a/repmgr.c b/repmgr.c index 18874c3b..eb91e6ed 100644 --- a/repmgr.c +++ b/repmgr.c @@ -287,7 +287,7 @@ do_master_register(void) char conninfo[MAXLEN]; bool schema_exists = false; - const char *master_version = NULL; + char master_version[MAXVERSIONSTR]; /* * Read the configuration file: repmgr.conf @@ -303,7 +303,7 @@ do_master_register(void) conn = establishDBConnection(conninfo, true); /* master should be v9 or better */ - master_version = pg_version(conn); + pg_version(conn, master_version); if (strcmp(master_version, "") == 0) { PQfinish(conn); @@ -464,8 +464,8 @@ do_standby_register(void) int myLocalId = -1; char conninfo[MAXLEN]; - const char *master_version = NULL; - const char *standby_version = NULL; + char master_version[MAXVERSIONSTR]; + char standby_version[MAXVERSIONSTR]; /* * Read the configuration file: repmgr.conf @@ -481,7 +481,7 @@ do_standby_register(void) conn = establishDBConnection(conninfo, true); /* should be v9 or better */ - standby_version = pg_version(conn); + pg_version(conn, standby_version); if (strcmp(standby_version, "") == 0) { PQfinish(conn); @@ -523,7 +523,7 @@ do_standby_register(void) return; /* master should be v9 or better */ - master_version = pg_version(master_conn); + pg_version(master_conn, master_version); if (strcmp(master_version, "") == 0) { PQfinish(conn); @@ -600,7 +600,7 @@ do_standby_clone(void) const char *first_wal_segment = NULL; const char *last_wal_segment = NULL; - const char *master_version = NULL; + char master_version[MAXVERSIONSTR]; /* if dest_dir hasn't been provided, initialize to current directory */ if (dest_dir == NULL) @@ -683,7 +683,7 @@ do_standby_clone(void) } /* primary should be v9 or better */ - master_version = pg_version(conn); + pg_version(conn, master_version); if (strcmp(master_version, "") == 0) { PQfinish(conn); @@ -968,7 +968,7 @@ do_standby_promote(void) char recovery_file_path[MAXLEN]; char recovery_done_path[MAXLEN]; - const char *standby_version = NULL; + char standby_version[MAXVERSIONSTR]; /* * Read the configuration file: repmgr.conf @@ -985,7 +985,7 @@ do_standby_promote(void) conn = establishDBConnection(conninfo, true); /* we need v9 or better */ - standby_version = pg_version(conn); + pg_version(conn, standby_version); if (strcmp(standby_version, "") == 0) { PQfinish(conn); @@ -1074,8 +1074,8 @@ do_standby_follow(void) int r; char data_dir[MAXLEN]; - const char *master_version = NULL; - const char *standby_version = NULL; + char master_version[MAXVERSIONSTR]; + char standby_version[MAXVERSIONSTR]; /* * Read the configuration file: repmgr.conf @@ -1099,7 +1099,7 @@ do_standby_follow(void) } /* should be v9 or better */ - standby_version = pg_version(conn); + pg_version(conn, standby_version); if (strcmp(standby_version, "") == 0) { PQfinish(conn); @@ -1125,7 +1125,7 @@ do_standby_follow(void) } /* should be v9 or better */ - master_version = pg_version(master_conn); + pg_version(master_conn, master_version); if (strcmp(master_version, "") == 0) { PQfinish(conn); diff --git a/repmgr.h b/repmgr.h index 4768f966..5e152174 100644 --- a/repmgr.h +++ b/repmgr.h @@ -20,5 +20,6 @@ #define MAXLEN 80 #define CONFIG_FILE "repmgr.conf" +#define MAXVERSIONSTR 16 #endif diff --git a/repmgrd.c b/repmgrd.c index 42e9fe7c..7d3b3e91 100644 --- a/repmgrd.c +++ b/repmgrd.c @@ -81,7 +81,7 @@ main(int argc, char **argv) int c; char conninfo[MAXLEN]; - const char *standby_version = NULL; + char standby_version[MAXVERSIONSTR]; progname = get_progname(argv[0]); @@ -138,7 +138,7 @@ main(int argc, char **argv) myLocalConn = establishDBConnection(conninfo, true); /* should be v9 or better */ - standby_version = pg_version(myLocalConn); + pg_version(myLocalConn, standby_version); if (strcmp(standby_version, "") == 0) { PQfinish(myLocalConn);