commit 4bdff5e3e9d4ea11c7e8043e75d63c4f366558e8 Author: Nick Mathewson nickm@torproject.org Date: Thu Mar 14 08:54:20 2019 -0400

practracker compliance: split lint_message into more logical parts --- src/lib/pubsub/pubsub_check.c | 237 ++++++++++++++++++++++++++---------------- 1 file changed, 149 insertions(+), 88 deletions(-)

diff --git a/src/lib/pubsub/pubsub_check.c b/src/lib/pubsub/pubsub_check.c index e87c347c4..94a55beeb 100644 --- a/src/lib/pubsub/pubsub_check.c +++ b/src/lib/pubsub/pubsub_check.c @@ -171,115 +171,119 @@ pubsub_cfg_dump(const pubsub_cfg_t *cfg, int severity, const char *prefix) }

/** - * Check whether there are any errors or inconsistencies for the message - * described by <b>msg</b> in <b>map</b>. If there are problems, log about - * them, and return -1. Otherwise return 0. + * Helper: fill a bitarray <b>out</b> with entries corresponding to the + * subsystems listed in <b>items</b>. If any subsystem is listed more than + * once, log a warning. Return 0 on success, -1 on failure. **/ static int -lint_message(const pubsub_adjmap_t *map, message_id_t msg) +get_message_bitarray(const pubsub_adjmap_t *map, + message_id_t msg, + const smartlist_t *items, + const char *operation, + bitarray_t **out) { - /* NOTE: Some of the checks in this function are maybe over-zealous, and we - * might not want to have them forever. I've marked them with [?] below. - */ - if (BUG(msg >= map->n_msgs)) - return 0; // LCOV_EXCL_LINE - - const smartlist_t *pub = map->pub_by_msg[msg]; - const smartlist_t *sub = map->sub_by_msg[msg]; + bool ok = true; + *out = bitarray_init_zero((unsigned)map->n_subsystems); + if (! items) + return 0;

- const size_t n_pub = smartlist_len_opt(pub); - const size_t n_sub = smartlist_len_opt(sub); + SMARTLIST_FOREACH_BEGIN(items, const pubsub_cfg_t *, cfg) { + if (bitarray_is_set(*out, cfg->subsys)) { + log_warn(LD_MESG|LD_BUG, + "Message %u (%s) is configured to be %s by subsystem " + "%u (%s) more than once.", + msg, get_message_id_name(msg), operation, + cfg->subsys, get_subsys_id_name(cfg->subsys)); + ok = false; + } + bitarray_set(*out, cfg->subsys); + } SMARTLIST_FOREACH_END(cfg);

- if (n_pub == 0 && n_sub == 0) { - log_info(LD_MESG, "Nobody is publishing or subscribing to message %u " - "(%s).", - msg, get_message_id_name(msg)); - return 0; // No publishers or subscribers: nothing to do. - } + return ok ? 0 : -1; +}

- /* We'll set this to false if there are any problems. */ +/** + * Helper for lint_message: check that all the pubsub_cfg_t items in the two + * respective smartlists obey our local graph topology rules. + * + * (Right now this is just a matter of "each subsystem only + * publishes/subscribes once; no subsystem is a publisher and subscriber for + * the same message.") + * + * Return 0 on success, -1 on failure. + **/ +static int +lint_message_graph(const pubsub_adjmap_t *map, + message_id_t msg, + const smartlist_t *pub, + const smartlist_t *sub) +{ + bitarray_t *published_by = NULL; + bitarray_t *subscribed_by = NULL; bool ok = true;

- /* First make sure that if there are publishers, there are subscribers. */ - if (n_pub == 0) { - log_warn(LD_MESG|LD_BUG, - "Message %u (%s) has subscribers, but no publishers.", - msg, get_message_id_name(msg)); + if (get_message_bitarray(map, msg, pub, "published", &published_by) < 0) ok = false; - } else if (n_sub == 0) { - log_warn(LD_MESG|LD_BUG, - "Message %u (%s) has publishers, but no subscribers.", - msg, get_message_id_name(msg)); + if (get_message_bitarray(map, msg, sub, "subscribed", &subscribed_by) < 0) ok = false; + + /* Check whether any subsystem is publishing and subscribing the same + * message. [??] + */ + for (unsigned i = 0; i < map->n_subsystems; ++i) { + if (bitarray_is_set(published_by, i) && + bitarray_is_set(subscribed_by, i)) { + log_warn(LD_MESG|LD_BUG, + "Message %u (%s) is published and subscribed by the same " + "subsystem %u (%s)", + msg, get_message_id_name(msg), + i, get_subsys_id_name(i)); + ok = false; + } }

+ bitarray_free(published_by); + bitarray_free(subscribed_by); + + return ok ? 0 : -1; +} + +/** + * Helper for lint_message: check that all the pubsub_cfg_t items in the two + * respective smartlists have compatible flags, channels, and types. + **/ +static int +lint_message_consistency(message_id_t msg, + const smartlist_t *pub, + const smartlist_t *sub) +{ + if (!smartlist_len_opt(pub) && !smartlist_len_opt(sub)) + return 0; // LCOV_EXCL_LINE -- this was already checked. + /* The 'all' list has the publishers and the subscribers. */ smartlist_t *all = smartlist_new(); if (pub) smartlist_add_all(all, pub); if (sub) smartlist_add_all(all, sub); + const pubsub_cfg_t *item0 = smartlist_get(all, 0);

/* Indicates which subsystems we've found publishing/subscribing here. */ - bitarray_t *published_by = bitarray_init_zero((unsigned)map->n_subsystems); - bitarray_t *subscribed_by = bitarray_init_zero((unsigned)map->n_subsystems); bool pub_excl = false, sub_excl = false, chan_same = true, type_same = true;

- /* Make sure that the messages all have the same channel and type; - * check whether the DISP_FLAG_EXCL flag is used; - * and if any subsystem is publishing or subscribing to it twice [??]. + /* Simple message consistency properties across messages. */ SMARTLIST_FOREACH_BEGIN(all, const pubsub_cfg_t *, cfg) { - if (cfg->channel != item0->channel) { - chan_same = false; - } - if (cfg->type != item0->type) { - type_same = false; - } - if (cfg->flags & DISP_FLAG_EXCL) { - if (cfg->is_publish) - pub_excl = true; - else - sub_excl = true; - } - if (cfg->is_publish) { - if (bitarray_is_set(published_by, cfg->subsys)) { - log_warn(LD_MESG|LD_BUG, - "Message %u (%s) is configured to be published by subsystem " - "%u (%s) more than once.", - msg, get_message_id_name(msg), - cfg->subsys, get_subsys_id_name(cfg->subsys)); - ok = false; - } - bitarray_set(published_by, cfg->subsys); - } else { - if (bitarray_is_set(subscribed_by, cfg->subsys)) { - log_warn(LD_MESG|LD_BUG, - "Message %u (%s) is configured to be subscribed by subsystem " - "%u (%s) more than once.", - msg, get_message_id_name(msg), - cfg->subsys, get_subsys_id_name(cfg->subsys)); - ok = false; - } - bitarray_set(subscribed_by, cfg->subsys); - } + chan_same &= (cfg->channel == item0->channel); + type_same &= (cfg->type == item0->type); + if (cfg->is_publish) + pub_excl |= (cfg->flags & DISP_FLAG_EXCL) != 0; + else + sub_excl |= (cfg->flags & DISP_FLAG_EXCL) != 0; } SMARTLIST_FOREACH_END(cfg);

- /* Check whether any subsystem is publishing and subscribing the same - * message. [??] - */ - for (unsigned i = 0; i < map->n_subsystems; ++i) { - if (bitarray_is_set(published_by, i) && - bitarray_is_set(subscribed_by, i)) { - log_warn(LD_MESG|LD_BUG, - "Message %u (%s) is published and subscribed by the same " - "subsystem %u (%s)", - msg, get_message_id_name(msg), - i, get_subsys_id_name(i)); - ok = false; - } - } + bool ok = true;

if (! chan_same) { log_warn(LD_MESG|LD_BUG, @@ -314,17 +318,74 @@ lint_message(const pubsub_adjmap_t *map, message_id_t msg) ok = false; }

+ smartlist_free(all); + + return ok ? 0 : -1; +} + +/** + * Check whether there are any errors or inconsistencies for the message + * described by <b>msg</b> in <b>map</b>. If there are problems, log about + * them, and return -1. Otherwise return 0. + **/ +static int +lint_message(const pubsub_adjmap_t *map, message_id_t msg) +{ + /* NOTE: Some of the checks in this function are maybe over-zealous, and we + * might not want to have them forever. I've marked them with [?] below. + */ + if (BUG(msg >= map->n_msgs)) + return 0; // LCOV_EXCL_LINE + + const smartlist_t *pub = map->pub_by_msg[msg]; + const smartlist_t *sub = map->sub_by_msg[msg]; + + const size_t n_pub = smartlist_len_opt(pub); + const size_t n_sub = smartlist_len_opt(sub); + + if (n_pub == 0 && n_sub == 0) { + log_info(LD_MESG, "Nobody is publishing or subscribing to message %u " + "(%s).", + msg, get_message_id_name(msg)); + return 0; // No publishers or subscribers: nothing to do. + } + /* We'll set this to false if there are any problems. */ + bool ok = true; + + /* First make sure that if there are publishers, there are subscribers. */ + if (n_pub == 0) { + log_warn(LD_MESG|LD_BUG, + "Message %u (%s) has subscribers, but no publishers.", + msg, get_message_id_name(msg)); + ok = false; + } else if (n_sub == 0) { + log_warn(LD_MESG|LD_BUG, + "Message %u (%s) has publishers, but no subscribers.", + msg, get_message_id_name(msg)); + ok = false; + } + + /* Check the message graph topology. */ + if (lint_message_graph(map, msg, pub, sub) < 0) + ok = false; + + /* Check whether the messages have the same fields set on them. */ + if (lint_message_consistency(msg, pub, sub) < 0) + ok = false; + if (!ok) { /* There was a problem -- let's log all the publishers and subscribers on * this message */ - SMARTLIST_FOREACH(all, pubsub_cfg_t *, cfg, - pubsub_cfg_dump(cfg, LOG_WARN, " ")); + if (pub) { + SMARTLIST_FOREACH(pub, pubsub_cfg_t *, cfg, + pubsub_cfg_dump(cfg, LOG_WARN, " ")); + } + if (sub) { + SMARTLIST_FOREACH(sub, pubsub_cfg_t *, cfg, + pubsub_cfg_dump(cfg, LOG_WARN, " ")); + } }

- smartlist_free(all); - bitarray_free(published_by); - bitarray_free(subscribed_by); - return ok ? 0 : -1; }