commit 4bdff5e3e9d4ea11c7e8043e75d63c4f366558e8
Author: Nick Mathewson <nickm(a)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;
}