From 86727730b8e4cfce2a4ecf2e787c8bf7f57af924 Mon Sep 17 00:00:00 2001 From: David Härdeman Date: Wed, 1 Jul 2020 08:07:18 +0200 Subject: Improve line/error reporting in server config parsing --- shared/config-parser.c | 229 ++++++++++++++++++------------------------------- 1 file changed, 83 insertions(+), 146 deletions(-) (limited to 'shared/config-parser.c') diff --git a/shared/config-parser.c b/shared/config-parser.c index c238bb7..878262e 100644 --- a/shared/config-parser.c +++ b/shared/config-parser.c @@ -156,12 +156,13 @@ static bool handle_dns(struct server_config *scfg, enum scfg_keys type, break; default: error("invalid DNS lookup type"); + /* Note: we'll leak dns here, but not much we can do */ return false; } if (!notification_cb) { error("async DNS lookup received but not requested"); - xfree(dns); + /* Ditto */ return false; } @@ -225,169 +226,112 @@ static char *systemd_object_path(const char *service) return r; } -/* FIXME: Add a strict argument */ -bool scfg_validate(struct server_config *scfg) +#define ERROR(msg) do { *error = (msg); return false; } while (0) + +bool scfg_validate(struct server_config *scfg, const char **error) { struct saddr *saddr; uint16_t port; - if (!scfg) { - error("invalid arguments"); - return false; - } + assert_return(scfg && error, false); - if (!scfg->filename) { - error("filename not set"); - return false; - } + if (!scfg) + ERROR("invalid arguments"); - if (!list_empty(&scfg->dnslookups)) { - error("%s: pending DNS requests", - scfg->filename); - return false; - } + if (!scfg->filename) + ERROR("filename not set"); + + if (!list_empty(&scfg->dnslookups)) + ERROR("pending DNS requests"); if (scfg->stop_method == SERVER_STOP_METHOD_RCON && - list_empty(&scfg->rcons)) { - error("%s: rcon stop method missing rcon address", - scfg->filename); - return false; - } + list_empty(&scfg->rcons)) + ERROR("rcon stop method missing rcon address"); if (scfg->stop_method == SERVER_STOP_METHOD_RCON && - !scfg->rcon_password) { - error("%s: rcon stop method missing rcon password", - scfg->filename); - return false; - } + !scfg->rcon_password) + ERROR("rcon stop method missing rcon password"); if ((scfg->start_method == SERVER_START_METHOD_SYSTEMD || scfg->stop_method == SERVER_STOP_METHOD_SYSTEMD)) { - if (!scfg->systemd_service || !scfg->systemd_obj) { - error("%s: systemd start/stop method missing " - "systemd service", scfg->filename); - return false; - } + if (!scfg->systemd_service || !scfg->systemd_obj) + ERROR("systemd start/stop missing service"); } else { - if (scfg->systemd_service || scfg->systemd_obj) { - error("%s: systemd service set but not used", - scfg->filename); - return false; - } + if (scfg->systemd_service || scfg->systemd_obj) + ERROR("systemd service set but not used"); } if (scfg->idle_timeout > 0 && - scfg->stop_method == SERVER_STOP_METHOD_UNDEFINED) { - error("%s: idle_timeout set but missing stop method", - scfg->filename); - return false; - } + scfg->stop_method == SERVER_STOP_METHOD_UNDEFINED) + ERROR("idle_timeout set but missing stop method"); switch (scfg->type) { case SERVER_TYPE_ANNOUNCE: - if (scfg->announce_port == 0) { - error("%s: missing announce port", scfg->filename); - return false; - } + if (scfg->announce_port == 0) + ERROR("missing announce port"); - if (scfg->start_method != SERVER_START_METHOD_UNDEFINED) { - error("%s: can't set start_method for announce server", - scfg->filename); - return false; - } + if (scfg->start_method != SERVER_START_METHOD_UNDEFINED) + ERROR("can't set start_method for announce server"); - if (scfg->start_exec) { - error("%s: can't set start_exec for announce server", - scfg->filename); - return false; - } + if (scfg->start_exec) + ERROR("can't set start_exec for announce server"); - if (!list_empty(&scfg->locals)) { - error("%s: can't set local addresses for announce server", - scfg->filename); - return false; - } + if (!list_empty(&scfg->locals)) + ERROR("can't set local addresses for announce server"); - if (!list_empty(&scfg->remotes)) { - error("%s: can't set remote addresses for announce server", - scfg->filename); - return false; - } + if (!list_empty(&scfg->remotes)) + ERROR("can't set remote addresses for announce server"); break; case SERVER_TYPE_PROXY: - if (scfg->announce_port != 0) { - error("%s: can't set announce port for proxy server", - scfg->filename); - return false; - } + if (scfg->announce_port != 0) + ERROR("can't set announce port for proxy server"); - if (list_empty(&scfg->locals)) { - error("%s: missing local addresses for proxy server", - scfg->filename); - return false; - } + if (list_empty(&scfg->locals)) + ERROR("missing local addresses for proxy server"); - if (list_empty(&scfg->remotes)) { - error("%s: missing remote addresses for proxy server", - scfg->filename); - return false; - } + if (list_empty(&scfg->remotes)) + ERROR("missing remote addresses for proxy server"); list_for_each_entry(saddr, &scfg->locals, list) { port = saddr_port(saddr); - if (port == 0) { - error("%s: invalid local port", scfg->filename); - return false; - } + if (port == 0) + ERROR("invalid local port"); if (scfg->announce_port == 0) scfg->announce_port = port; - else if (scfg->announce_port != port) { - error("%s: multiple local ports", scfg->filename); - return false; - } + else if (scfg->announce_port != port) + ERROR("multiple local ports"); } - if (scfg->announce_port == 0) { - error("%s: can't determine which port to announce", - scfg->filename); - return false; - } + if (scfg->announce_port == 0) + ERROR("can't determine which port to announce"); break; default: - error("%s: can't determine server type", scfg->filename); - return false; + ERROR("can't determine server type"); } return true; } -#define INVALID(fmt, ...) \ -do { \ - verbose("%s: " fmt, scfg->filename __VA_OPT__(,) __VA_ARGS__); \ - valid = false; \ -} while(0) - bool scfg_parse(struct server_config *scfg, char *buf, - notification_cb_t notification_cb) + notification_cb_t notification_cb, unsigned *lineno, + const char **error) { - unsigned lineno; char *pos; - bool valid = true; - assert_return(scfg && buf, false); + assert_return(scfg && buf && lineno && error, false); pos = buf; - if (!config_parse_header(SERVER_CFG_HEADER, &pos, &lineno)) { - INVALID("missing/invalid header"); - goto out; - } + *lineno = 0; + + if (!config_parse_header(SERVER_CFG_HEADER, &pos, lineno)) + ERROR("missing/invalid header"); while (true) { int key; @@ -395,41 +339,38 @@ bool scfg_parse(struct server_config *scfg, char *buf, struct cfg_value value; if (!config_parse_line(scfg->filename, &pos, scfg_key_map, &key, - &keyname, &value, !!notification_cb, &lineno)) + &keyname, &value, !!notification_cb, lineno)) break; - /* FIXME: Improve error reporting */ - if (key == SCFG_KEY_INVALID) { - INVALID("invalid line in config file"); - continue; - } + if (key == SCFG_KEY_INVALID) + ERROR("invalid line in config file"); debug(DBG_CFG, "%s: key %s", scfg->filename, keyname); switch (key) { case SCFG_KEY_TYPE: if (scfg->type != SERVER_TYPE_UNDEFINED) - INVALID("type defined multiple times"); + ERROR("server type defined multiple times"); else if (streq(value.str, "proxy")) scfg->type = SERVER_TYPE_PROXY; else if (streq(value.str, "announce")) scfg->type = SERVER_TYPE_ANNOUNCE; else - INVALID("unknown type: %s", value.str); + ERROR("unknown server type"); break; case SCFG_KEY_NAME: if (scfg->pretty_name) - INVALID("name defined multiple times"); + ERROR("server name defined multiple times"); else if (empty_str(value.str)) - INVALID("name is an empty string"); + ERROR("server name is an empty string"); else if (!(scfg->pretty_name = xstrdup(value.str))) - INVALID("strdup: %m"); + ERROR("strdup failure"); break; case SCFG_KEY_PORT: if (scfg->announce_port != 0) - INVALID("port defined multiple times"); + ERROR("port defined multiple times"); else scfg->announce_port = value.uint16; break; @@ -437,25 +378,25 @@ bool scfg_parse(struct server_config *scfg, char *buf, case SCFG_KEY_LOCAL: if (!handle_dns(scfg, SCFG_KEY_LOCAL, &value, notification_cb)) - INVALID("handle_dns failed"); + ERROR("handle_dns failed"); break; case SCFG_KEY_REMOTE: if (!handle_dns(scfg, SCFG_KEY_REMOTE, &value, notification_cb)) - INVALID("handle_dns failed"); + ERROR("handle_dns failed"); break; case SCFG_KEY_IDLE_TIMEOUT: if (scfg->idle_timeout != 0) - INVALID("idle_timeout already set"); + ERROR("idle_timeout already set"); else scfg->idle_timeout = value.uint16; break; case SCFG_KEY_STOP_METHOD: if (scfg->stop_method != SERVER_STOP_METHOD_UNDEFINED) - INVALID("stop method defined multiple times"); + ERROR("stop method defined multiple times"); else if (streq(value.str, "exec")) scfg->stop_method = SERVER_STOP_METHOD_EXEC; else if (streq(value.str, "rcon")) @@ -463,50 +404,50 @@ bool scfg_parse(struct server_config *scfg, char *buf, else if (streq(value.str, "systemd")) scfg->stop_method = SERVER_STOP_METHOD_SYSTEMD; else - INVALID("unknown stop method: %s", value.str); + ERROR("unknown stop method"); break; case SCFG_KEY_START_METHOD: if (scfg->start_method != SERVER_START_METHOD_UNDEFINED) - INVALID("start method defined multiple times"); + ERROR("start method defined multiple times"); else if (streq(value.str, "exec")) scfg->start_method = SERVER_START_METHOD_EXEC; else if (streq(value.str, "systemd")) scfg->start_method = SERVER_START_METHOD_SYSTEMD; else - INVALID("unknown start method: %s", value.str); + ERROR("unknown start method"); break; case SCFG_KEY_STOP_EXEC: if (scfg->stop_exec) - INVALID("stop exec defined multiple times"); + ERROR("stop exec defined multiple times"); else if (!(scfg->stop_exec = xstrdup(value.str))) - INVALID("strdup: %m"); + ERROR("strdup failure"); break; case SCFG_KEY_START_EXEC: if (scfg->start_exec) - INVALID("start exec defined multiple times"); + ERROR("start exec defined multiple times"); else if (!(scfg->start_exec = xstrdup(value.str))) - INVALID("strdup: %m"); + ERROR("strdup failure"); break; case SCFG_KEY_RCON: if (!handle_dns(scfg, SCFG_KEY_RCON, &value, notification_cb)) - INVALID("handle_dns failed"); + ERROR("handle_dns failed"); break; case SCFG_KEY_RCON_PASSWORD: if (scfg->rcon_password) - INVALID("rcon_password defined multiple times"); + ERROR("rcon_password defined multiple times"); else if (!(scfg->rcon_password = xstrdup(value.str))) - INVALID("strdup: %m"); + ERROR("strdup failure"); break; case SCFG_KEY_SYSTEMD_SERVICE: if (scfg->systemd_service) - INVALID("systemd_service defined multiple times"); + ERROR("systemd service defined multiple times"); else { const char *suffix; char *tmp; @@ -521,29 +462,27 @@ bool scfg_parse(struct server_config *scfg, char *buf, } else tmp = xstrdup(value.str); - if (!tmp) { - INVALID("malloc/strdup: %m"); - break; - } + if (!tmp) + ERROR("malloc/strdup failure"); scfg->systemd_service = tmp; scfg->systemd_obj = systemd_object_path(scfg->systemd_service); if (!scfg->systemd_obj) - INVALID("object_path: %m"); + ERROR("failed to create object_path"); } break; case SCFG_KEY_INVALID: + _fallthrough_; default: - INVALID("invalid line"); + ERROR("invalid line"); break; } } -out: - return valid; + return true; } void scfg_delete(struct server_config *scfg) @@ -1074,8 +1013,6 @@ bool config_parse_header(const char *title, char **buf, unsigned *lineno) assert_return(!empty_str(title) && buf && *buf && lineno, false); - *lineno = 0; - line = get_line(buf, lineno); if (line) { char titlehdr[strlen(title) + 3]; -- cgit v1.2.3