From 5b8a336bb3d7a25ba43f444bc586b27fb9f42746 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= <jpokorny@redhat.com>

Date: Tue, 13 Sep 2016 17:47:03 +0200

Subject: [PATCH 1/4] Low: make find_site_by_name failure set error code

At one instance (query_get_string_answer) it just flips the sign as it

is customary to return negative value upon error (for uniform

treatment).

---

 src/attr.c | 2 ++

 src/main.c | 3 ++-

 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/attr.c b/src/attr.c

index d9e5c91..0e407b6 100644

--- a/src/attr.c

+++ b/src/attr.c

@@ -16,6 +16,7 @@

  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.

  */

+#include <errno.h>

 #include <stdio.h>

 #include <string.h>

 #include "attr.h"

@@ -162,6 +163,7 @@ int do_attr_command(cmd_request_t cmd)

 	else {

 		if (!find_site_by_name(cl.site, &site, 1)) {

 			log_error("Site \"%s\" not configured.", cl.site);

+			rv = -ENOENT;

 			goto out_close;

 		}

 	}

diff --git a/src/main.c b/src/main.c

index 206c881..b1ff1e7 100644

--- a/src/main.c

+++ b/src/main.c

@@ -665,7 +665,7 @@ static int query_get_string_answer(cmd_request_t cmd)

 		site = local;

 	else if (!find_site_by_name(cl.site, &site, 1)) {

 		log_error("cannot find site \"%s\"", cl.site);

-		rv = ENOENT;

+		rv = -ENOENT;

 		goto out;

 	}

@@ -741,6 +741,7 @@ static int do_command(cmd_request_t cmd)

 	else {

 		if (!find_site_by_name(cl.site, &site, 1)) {

 			log_error("Site \"%s\" not configured.", cl.site);

+			rv = -ENOENT;

 			goto out_close;

 		}

 	}

-- 

2.4.11

From bcf1117d7e1f37165c6d0da022cadc63e391a2fa Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= <jpokorny@redhat.com>

Date: Tue, 13 Sep 2016 18:20:17 +0200

Subject: [PATCH 2/4] High: ensure local site resolved for all effective

 actions

Previously, running:

  touch /etc/booth/booth.conf

  booth grant a_ticket

would result in a segfault due to not guarding resolution of local

site properly in some circumstances, so do it at the central place.

Also error messaging is now centralized.

---

 src/main.c | 19 ++++---------------

 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/src/main.c b/src/main.c

index b1ff1e7..c05446f 100644

--- a/src/main.c

+++ b/src/main.c

@@ -377,9 +377,10 @@ static int setup_config(int type)

 			return -EINVAL;

 		}

 		local->local = 1;

-	} else

-		find_myself(NULL, type == CLIENT || type == GEOSTORE);

-

+	} else if (!find_myself(NULL, type == CLIENT || type == GEOSTORE)) {

+		log_error("Cannot find myself in the configuration.");

+		return -EINVAL;

+	}

 	rv = check_config(type);

 	if (rv < 0)

@@ -1302,13 +1303,6 @@ static int do_status(int type)

 		goto quit;

 	}

-

-	if (!local) {

-		reason = "No Service IP active here.";

-		goto quit;

-	}

-

-

 	rv = _lockfile(O_RDWR, &status_lock_fd, &pid;;

 	if (status_lock_fd == -1) {

 		reason = "No PID file.";

@@ -1422,11 +1416,6 @@ static int do_server(int type)

 	if (rv < 0)

 		return rv;

-	if (!local) {

-		log_error("Cannot find myself in the configuration.");

-		exit(EXIT_FAILURE);

-	}

-

 	if (daemonize) {

 		if (daemon(0, 0) < 0) {

 			perror("daemon error");

-- 

2.4.11

From 1185487afbd2a063664863f7bd98d1480ca0a2dd Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= <jpokorny@redhat.com>

Date: Tue, 13 Sep 2016 20:18:33 +0200

Subject: [PATCH 3/4] Low: make daemon with "-s site" (debug mode) claim

 "myself"

---

 src/main.c | 4 ++--

 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/main.c b/src/main.c

index c05446f..76e62c7 100644

--- a/src/main.c

+++ b/src/main.c

@@ -372,8 +372,8 @@ static int setup_config(int type)

 	/* Set "local" pointer, ignoring errors. */

 	if (cl.type == DAEMON && cl.site[0]) {

 		if (!find_site_by_name(cl.site, &local, 1)) {

-			log_error("Cannot find \"%s\" in the configuration.",

-					cl.site);

+			log_error("Cannot find \"%s\" (myself) in the configuration.",

+				  cl.site);

 			return -EINVAL;

 		}

 		local->local = 1;

-- 

2.4.11

From 736f58db41acd32b2ea2af1b4c0ba02683d58cf5 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= <jpokorny@redhat.com>

Date: Tue, 13 Sep 2016 20:10:13 +0200

Subject: [PATCH 4/4] Refactor: call find_site_by_name just once, up the stream

Respective logic was duplicated for all "booth list/peers/grant/revoke"

and "geostore list/get/set/del" separately, so utilize a natural control

flow to carry this once-resolved target site from here, sharing it with

the special case of "daemon" role invoked with "-s site" (debug mode).

Side effect: simpler, terser code.

---

 src/attr.c | 13 +----------

 src/attr.h |  2 +-

 src/main.c | 75 ++++++++++++++++++++++++++++----------------------------------

 3 files changed, 36 insertions(+), 54 deletions(-)

diff --git a/src/attr.c b/src/attr.c

index 0e407b6..805ccb3 100644

--- a/src/attr.c

+++ b/src/attr.c

@@ -150,24 +150,13 @@ static int read_server_reply(

 	return rv;

 }

-int do_attr_command(cmd_request_t cmd)

+int do_attr_command(cmd_request_t cmd, struct booth_site *site)

 {

-	struct booth_site *site = NULL;

 	struct boothc_header *header;

 	struct booth_transport const *tpt;

 	int len, rv = -1;

 	char *msg = NULL;

-	if (!*cl.site)

-		site = local;

-	else {

-		if (!find_site_by_name(cl.site, &site, 1)) {

-			log_error("Site \"%s\" not configured.", cl.site);

-			rv = -ENOENT;

-			goto out_close;

-		}

-	}

-

 	if (site->type == ARBITRATOR) {

 		if (site == local) {

 			log_error("We're just an arbitrator, no attributes here.");

diff --git a/src/attr.h b/src/attr.h

index 1c680bd..a94ac16 100644

--- a/src/attr.h

+++ b/src/attr.h

@@ -31,7 +31,7 @@

 void print_geostore_usage(void);

 int test_attr_reply(cmd_result_t reply_code, cmd_request_t cmd);

-int do_attr_command(cmd_request_t cmd);

+int do_attr_command(cmd_request_t cmd, struct booth_site *site);

 int process_attr_request(struct client *req_client, void *buf);

 int attr_recv(void *buf, struct booth_site *source);

 int store_geo_attr(struct ticket_config *tk, const char *name, const char *val, int notime);

diff --git a/src/main.c b/src/main.c

index 76e62c7..e09536a 100644

--- a/src/main.c

+++ b/src/main.c

@@ -346,7 +346,7 @@ int update_authkey()

 	return 0;

 }

-static int setup_config(int type)

+static int setup_config(int type, struct booth_site **site)

 {

 	int rv;

@@ -369,18 +369,31 @@ static int setup_config(int type)

 #endif

 	}

-	/* Set "local" pointer, ignoring errors. */

-	if (cl.type == DAEMON && cl.site[0]) {

-		if (!find_site_by_name(cl.site, &local, 1)) {

-			log_error("Cannot find \"%s\" (myself) in the configuration.",

-				  cl.site);

-			return -EINVAL;

+	/* Determine the target based on the provided address, ignore

+	   errors with DAEMON (special debug/testing arrangement). */

+	if (*cl.site && (cl.type == DAEMON || (site && strcmp(cl.site, OTHER_SITE)))) {

+		if (!find_site_by_name(cl.site, cl.type == DAEMON ? &local : site, 1)) {

+			log_error("Cannot find \"%s\"%s in the configuration.",

+				  cl.site, cl.type == DAEMON ? " (myself)" : "");

+			if (cl.type != DAEMON)

+				return -EINVAL;

 		}

-		local->local = 1;

-	} else if (!find_myself(NULL, type == CLIENT || type == GEOSTORE)) {

+		if (cl.type == DAEMON)

+			local->local = 1;

+		else

+			site = NULL;  /* prevent from overwriting */

+	}

+	/* Self-determine us. */

+	if (!find_myself(site, type == CLIENT || type == GEOSTORE)) {

 		log_error("Cannot find myself in the configuration.");

 		return -EINVAL;

 	}

+	/* We can resolve "other" only after we've determined us. */

+	if (*cl.site && site && !strcmp(cl.site, OTHER_SITE)

+			&& !find_site_by_name(cl.site, site, 1)) {

+		log_error("Cannot find %s node in the configuration.", cl.site);

+		return -EINVAL;

+	}

 	rv = check_config(type);

 	if (rv < 0)

@@ -635,9 +648,8 @@ static int test_reply(cmd_result_t reply_code, cmd_request_t cmd)

 	return rv;

 }

-static int query_get_string_answer(cmd_request_t cmd)

+static int query_get_string_answer(cmd_request_t cmd, struct booth_site *site)

 {

-	struct booth_site *site;

 	struct boothc_hdr_msg reply;

 	struct boothc_header *header;

 	char *data;

@@ -662,14 +674,6 @@ static int query_get_string_answer(cmd_request_t cmd)

 	init_header(header, cmd, 0, cl.options, 0, 0, msg_size);

-	if (!*cl.site)

-		site = local;

-	else if (!find_site_by_name(cl.site, &site, 1)) {

-		log_error("cannot find site \"%s\"", cl.site);

-		rv = -ENOENT;

-		goto out;

-	}

-

 	tpt = booth_transport + TCP;

 	rv = tpt->open(site);

 	if (rv < 0)

@@ -709,16 +713,14 @@ out_test_reply:

 	rv = test_reply_f(ntohl(reply.header.result), cmd);

 out_close:

 	tpt->close(site);

-out:

 	if (data)

 		free(data);

 	return rv;

 }

-static int do_command(cmd_request_t cmd)

+static int do_command(cmd_request_t cmd, struct booth_site *site)

 {

-	struct booth_site *site;

 	struct boothc_ticket_msg reply;

 	struct booth_transport const *tpt;

 	uint32_t leader_id;

@@ -732,21 +734,10 @@ static int do_command(cmd_request_t cmd)

 		op_str = "revoke";

 	rv = 0;

-	site = NULL;

 	/* Always use TCP for client - at least for now. */

 	tpt = booth_transport + TCP;

-	if (!*cl.site)

-		site = local;

-	else {

-		if (!find_site_by_name(cl.site, &site, 1)) {

-			log_error("Site \"%s\" not configured.", cl.site);

-			rv = -ENOENT;

-			goto out_close;

-		}

-	}

-

 	if (site->type == ARBITRATOR) {

 		if (site == local) {

 			log_error("We're just an arbitrator, cannot grant/revoke tickets here.");

@@ -1296,7 +1287,7 @@ static int do_status(int type)

 	ret = PCMK_OCF_NOT_RUNNING;

-	rv = setup_config(type);

+	rv = setup_config(type, NULL);

 	if (rv) {

 		reason = "Error reading configuration.";

 		ret = PCMK_OCF_UNKNOWN_ERROR;

@@ -1412,7 +1403,7 @@ static int do_server(int type)

 	int rv = -1;

 	static char log_ent[128] = DAEMON_NAME "-";

-	rv = setup_config(type);

+	rv = setup_config(type, NULL);

 	if (rv < 0)

 		return rv;

@@ -1478,8 +1469,9 @@ static int do_server(int type)

 static int do_client(void)

 {

 	int rv;

+	struct booth_site *site;

-	rv = setup_config(CLIENT);

+	rv = setup_config(CLIENT, &site);

 	if (rv < 0) {

 		log_error("cannot read config");

 		goto out;

@@ -1488,12 +1480,12 @@ static int do_client(void)

 	switch (cl.op) {

 	case CMD_LIST:

 	case CMD_PEERS:

-		rv = query_get_string_answer(cl.op);

+		rv = query_get_string_answer(cl.op, site);

 		break;

 	case CMD_GRANT:

 	case CMD_REVOKE:

-		rv = do_command(cl.op);

+		rv = do_command(cl.op, site);

 		break;

 	}

@@ -1504,8 +1496,9 @@ out:

 static int do_attr(void)

 {

 	int rv = -1;

+	struct booth_site *site;

-	rv = setup_config(GEOSTORE);

+	rv = setup_config(GEOSTORE, &site);

 	if (rv < 0) {

 		log_error("cannot read config");

 		goto out;

@@ -1529,12 +1522,12 @@ static int do_attr(void)

 	switch (cl.op) {

 	case ATTR_LIST:

 	case ATTR_GET:

-		rv = query_get_string_answer(cl.op);

+		rv = query_get_string_answer(cl.op, site);

 		break;

 	case ATTR_SET:

 	case ATTR_DEL:

-		rv = do_attr_command(cl.op);

+		rv = do_attr_command(cl.op, site);

 		break;

 	}

-- 

2.4.11