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