From 5b8a336bb3d7a25ba43f444bc586b27fb9f42746 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Pokorn=C3=BD?= 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 #include #include #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?= 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?= 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?= 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