This is an automated email from the ASF dual-hosted git repository.
kgiusti pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/qpid-dispatch.git
The following commit(s) were added to refs/heads/master by this push:
new ee82acd DISPATCH-1642: Change parse tree insert to fail on duplicate
entry
ee82acd is described below
commit ee82acda5656ca0b5bb6ef86b9869f9ecfac1559
Author: Kenneth Giusti <kgiu...@apache.org>
AuthorDate: Wed May 13 10:25:54 2020 -0400
DISPATCH-1642: Change parse tree insert to fail on duplicate entry
---
src/parse_tree.c | 49 ++++++++++++++++----------------
src/parse_tree.h | 39 ++++++++++++++++++--------
src/policy.c | 19 ++++++-------
src/router_core/agent_config_address.c | 47 +++++++++++++++++--------------
src/router_core/route_control.c | 27 ++++++------------
tests/parse_tree_tests.c | 51 ++++++++++++++++++++--------------
6 files changed, 124 insertions(+), 108 deletions(-)
diff --git a/src/parse_tree.c b/src/parse_tree.c
index da12c4a..bf5b695 100644
--- a/src/parse_tree.c
+++ b/src/parse_tree.c
@@ -20,6 +20,7 @@
#include "parse_tree.h"
#include <qpid/dispatch/log.h>
+#include <qpid/dispatch/alloc.h>
// token parsing
@@ -278,24 +279,24 @@ static qd_parse_node_t *parse_node_find_child(const
qd_parse_node_t *node, const
}
-// Add a new pattern and associated data to the tree. Returns the old payload
-// if the pattern has already been added to the tree.
+// Add a new pattern and associated data to the tree.
+// Return QD_ERROR_ALREADY_EXISTS if the pattern is already in the tree.
//
-static void *parse_node_add_pattern(qd_parse_node_t *node,
- token_iterator_t *key,
- const char *pattern,
- void *payload)
+static qd_error_t parse_node_add_pattern(qd_parse_node_t *node,
+ token_iterator_t *key,
+ const char *pattern,
+ void *payload)
{
- if (token_iterator_done(key)) {
- // this node's pattern
- void *old;
- if (!node->pattern) {
- node->pattern = strdup(pattern);
+ if (token_iterator_done(key)) { // empty leaf node?
+
+ if (node->pattern) {
+ // this node is not empty
+ return QD_ERROR_ALREADY_EXISTS;
}
- assert(strcmp(node->pattern, pattern) == 0);
- old = node->payload;
+
+ node->pattern = strdup(pattern);
node->payload = payload;
- return old;
+ return QD_ERROR_NONE;
}
if (token_iterator_is_match_1(key)) {
@@ -608,13 +609,12 @@ void qd_parse_tree_search(qd_parse_tree_t *node,
}
-// returns old payload or NULL if new
-void *qd_parse_tree_add_pattern(qd_parse_tree_t *node,
- const qd_iterator_t *pattern,
- void *payload)
+// Add match pattern to tree
+qd_error_t qd_parse_tree_add_pattern(qd_parse_tree_t *node,
+ const qd_iterator_t *pattern,
+ void *payload)
{
token_iterator_t key;
- void *rc = NULL;
// @TODO(kgiusti) for now:
qd_iterator_t *dup = qd_iterator_dup(pattern);
char *str = (char *)qd_iterator_copy(dup);
@@ -624,7 +624,7 @@ void *qd_parse_tree_add_pattern(qd_parse_tree_t *node,
"Parse tree add address pattern '%s'", str);
token_iterator_init(&key, node->type, str);
- rc = parse_node_add_pattern(node, &key, str, payload);
+ qd_error_t rc = parse_node_add_pattern(node, &key, str, payload);
free(str);
qd_iterator_free(dup);
return rc;
@@ -784,12 +784,11 @@ void qd_parse_tree_free(qd_parse_node_t *node)
//
// returns old payload or NULL if new
-void *qd_parse_tree_add_pattern_str(qd_parse_tree_t *node,
- const char *pattern,
- void *payload)
+qd_error_t qd_parse_tree_add_pattern_str(qd_parse_tree_t *node,
+ const char *pattern,
+ void *payload)
{
token_iterator_t key;
- void *rc = NULL;
char *str = strdup(pattern);
normalize_pattern(node->type, str);
@@ -797,7 +796,7 @@ void *qd_parse_tree_add_pattern_str(qd_parse_tree_t *node,
"Parse tree(str) add address pattern '%s'", str);
token_iterator_init(&key, node->type, str);
- rc = parse_node_add_pattern(node, &key, str, payload);
+ qd_error_t rc = parse_node_add_pattern(node, &key, str, payload);
free(str);
return rc;
}
diff --git a/src/parse_tree.h b/src/parse_tree.h
index 26613a3..e01f606 100644
--- a/src/parse_tree.h
+++ b/src/parse_tree.h
@@ -20,10 +20,8 @@
*/
#include <stdbool.h>
-
-#include <qpid/dispatch/ctools.h>
+#include <qpid/dispatch/error.h>
#include <qpid/dispatch/iterator.h>
-#include <qpid/dispatch/alloc.h>
typedef struct qd_parse_node qd_parse_tree_t;
@@ -58,17 +56,28 @@ const char *qd_parse_address_token_sep();
bool qd_parse_tree_validate_pattern(const qd_parse_tree_t *tree,
const qd_iterator_t *pattern);
-// returns old payload or NULL if new
-void *qd_parse_tree_add_pattern(qd_parse_tree_t *node,
- const qd_iterator_t *pattern,
- void *payload);
+// Inserts payload in tree using pattern as the lookup key
+//
+// @param node root of parse tree
+// @param pattern match pattern (copied)
+// @param payload stored in tree (referenced)
+// @return QD_ERROR_NONE (0) on success else qd_error_t code
+//
+qd_error_t qd_parse_tree_add_pattern(qd_parse_tree_t *node,
+ const qd_iterator_t *pattern,
+ void *payload);
// returns old payload or NULL if not present
+//
+// @param node root of parse tree
+// @param pattern match pattern of payload to remove
+//
void *qd_parse_tree_remove_pattern(qd_parse_tree_t *node,
const qd_iterator_t *pattern);
// retrieves the payload pointer
-// returns true if pattern found
+// returns true if pattern found and sets *payload
+//
bool qd_parse_tree_get_pattern(qd_parse_tree_t *tree,
const qd_iterator_t *pattern,
void **payload);
@@ -114,10 +123,16 @@ bool qd_parse_tree_walk(qd_parse_tree_t *tree,
qd_parse_tree_visit_t *callback,
// parse tree functions using string interface
//
-// returns old payload or NULL if new
-void *qd_parse_tree_add_pattern_str(qd_parse_tree_t *node,
- const char *pattern,
- void *payload);
+// Inserts payload in tree using pattern as the lookup key
+//
+// @param node root of parse tree
+// @param pattern match pattern (copied)
+// @param payload stored in tree (referenced)
+// @return QD_ERROR_NONE (0) on success else qd_error_t code
+//
+qd_error_t qd_parse_tree_add_pattern_str(qd_parse_tree_t *node,
+ const char *pattern,
+ void *payload);
// returns true on match and sets *payload
bool qd_parse_tree_retrieve_match_str(qd_parse_tree_t *tree,
diff --git a/src/policy.c b/src/policy.c
index 91166aa..8534065 100644
--- a/src/policy.c
+++ b/src/policy.c
@@ -1350,21 +1350,18 @@ bool qd_policy_host_pattern_add(qd_policy_t *policy,
const char *hostPattern)
{
void *payload = strdup(hostPattern);
sys_mutex_lock(policy->tree_lock);
- void *oldp = qd_parse_tree_add_pattern_str(policy->hostname_tree,
hostPattern, payload);
- if (oldp) {
- void *recovered = qd_parse_tree_add_pattern_str(policy->hostname_tree,
(char *)oldp, oldp);
- assert (recovered);
- (void)recovered; /* Silence compiler complaints of unused
variable */
- }
+ qd_error_t rc = qd_parse_tree_add_pattern_str(policy->hostname_tree,
hostPattern, payload);
sys_mutex_unlock(policy->tree_lock);
- if (oldp) {
+
+ if (rc != QD_ERROR_NONE) {
+ const char *err = qd_error_name(rc);
free(payload);
qd_log(policy->log_source,
- QD_LOG_WARNING,
- "vhost hostname pattern '%s' failed to replace optimized pattern
'%s'",
- hostPattern, (char *)oldp);
+ QD_LOG_WARNING,
+ "vhost hostname pattern '%s' add failed: %s",
+ hostPattern, err ? err : "unknown error");
}
- return oldp == 0;
+ return rc == QD_ERROR_NONE;
}
diff --git a/src/router_core/agent_config_address.c
b/src/router_core/agent_config_address.c
index c04267e..27e213c 100644
--- a/src/router_core/agent_config_address.c
+++ b/src/router_core/agent_config_address.c
@@ -337,7 +337,6 @@ void qdra_config_address_create_CT(qdr_core_t *core,
qd_parsed_field_t *in_body)
{
char *pattern = NULL;
- qd_iterator_t *iter = NULL;
while (true) {
//
@@ -399,7 +398,7 @@ void qdra_config_address_create_CT(qdr_core_t *core,
if (fallback && (waypoint || in_phase > 0 || out_phase > 0)) {
msg = "Fallback cannot be specified with waypoint or non-zero
ingress and egress phases";
}
-
+
if (msg) {
query->status = QD_AMQP_BAD_REQUEST;
query->status.description = msg;
@@ -418,19 +417,6 @@ void qdra_config_address_create_CT(qdr_core_t
*core,
break;
}
- iter = qd_iterator_string(pattern, ITER_VIEW_ALL);
-
- //
- // Ensure that there isn't another configured address with the same
pattern
- //
-
- if (qd_parse_tree_get_pattern(core->addr_parse_tree, iter, (void
**)&addr)) {
- query->status = QD_AMQP_BAD_REQUEST;
- query->status.description = "Address prefix conflicts with an
existing entity";
- qd_log(core->agent_log, QD_LOG_ERROR, "Error performing CREATE of
%s: %s", CONFIG_ADDRESS_TYPE, query->status.description);
- break;
- }
-
//
// Handle the address-phasing logic. If the phases are provided, use
them. Otherwise
// use the waypoint flag to set the most common defaults.
@@ -461,11 +447,34 @@ void qdra_config_address_create_CT(qdr_core_t
*core,
}
//
- // The request is good. Create the entity and insert it into the hash
index and list.
+ // The request is valid. Attempt to insert the address pattern into
+ // the parse tree, fail if there is already an entry for that pattern
//
-
addr = new_qdr_address_config_t();
+ if (!addr) {
+ query->status = QD_AMQP_BAD_REQUEST;
+ query->status.description = "Out of memory";
+ qd_log(core->agent_log, QD_LOG_ERROR, "Error performing CREATE of
%s: %s", CONFIG_ADDRESS_TYPE, query->status.description);
+ break;
+ }
ZERO(addr);
+
+ //
+ // Insert the uninitialized address to check if it already exists in
+ // the parse tree. On success initialize it. This is thread safe
+ // since the current thread (core) is the only thread allowed to use
+ // the parse tree
+ //
+
+ qd_error_t rc = qd_parse_tree_add_pattern_str(core->addr_parse_tree,
pattern, addr);
+ if (rc) {
+ free_qdr_address_config_t(addr);
+ query->status = QD_AMQP_BAD_REQUEST;
+ query->status.description = qd_error_name(rc);
+ qd_log(core->agent_log, QD_LOG_ERROR, "Error performing CREATE of
%s: %s", CONFIG_ADDRESS_TYPE, query->status.description);
+ break;
+ }
+
addr->ref_count = 1; // Represents the reference from the addr_config
list
addr->name = name ? (char*) qd_iterator_copy(name) : 0;
addr->identity = qdr_identifier(core);
@@ -478,9 +487,6 @@ void qdra_config_address_create_CT(qdr_core_t *core,
addr->fallback = fallback;
pattern = 0;
- qd_iterator_reset_view(iter, ITER_VIEW_ALL);
- qd_parse_tree_add_pattern(core->addr_parse_tree, iter, addr);
-
DEQ_INSERT_TAIL(core->addr_config, addr);
if (name) {
qd_iterator_view_t iter_view = qd_iterator_get_view(name);
@@ -517,7 +523,6 @@ void qdra_config_address_create_CT(qdr_core_t *core,
qdr_query_free(query);
free(pattern);
- qd_iterator_free(iter);
}
diff --git a/src/router_core/route_control.c b/src/router_core/route_control.c
index f6915b1..748f17c 100644
--- a/src/router_core/route_control.c
+++ b/src/router_core/route_control.c
@@ -702,20 +702,15 @@ void qdr_link_route_map_pattern_CT(qdr_core_t *core,
qd_iterator_t *address, qdr
{
qd_direction_t dir;
char *pattern = qdr_address_to_link_route_pattern(address, &dir);
- qd_iterator_t *iter = qd_iterator_string(pattern, ITER_VIEW_ALL);
- qdr_address_t *other_addr;
- bool found = qd_parse_tree_get_pattern(core->link_route_tree[dir], iter,
(void **)&other_addr);
- if (!found) {
- qd_parse_tree_add_pattern(core->link_route_tree[dir], iter, addr);
- } else {
+ qd_error_t rc = qd_parse_tree_add_pattern_str(core->link_route_tree[dir],
pattern, addr);
+ if (rc) {
// the pattern is mapped once when the address is added to the hash
// table. It should not be mapped twice
- qd_log(core->log, QD_LOG_CRITICAL, "Link route %s mapped redundantly!",
- pattern);
+ qd_log(core->log, QD_LOG_CRITICAL, "Link route %s mapped redundantly:
%s!",
+ pattern, qd_error_name(rc));
}
- qd_iterator_free(iter);
free(pattern);
}
@@ -727,19 +722,15 @@ void qdr_link_route_unmap_pattern_CT(qdr_core_t *core,
qd_iterator_t *address)
{
qd_direction_t dir;
char *pattern = qdr_address_to_link_route_pattern(address, &dir);
- qd_iterator_t *iter = qd_iterator_string(pattern, ITER_VIEW_ALL);
- qdr_address_t *addr;
- bool found = qd_parse_tree_get_pattern(core->link_route_tree[dir], iter,
(void **)&addr);
- if (found) {
- qd_parse_tree_remove_pattern(core->link_route_tree[dir], iter);
- } else {
- // expected that the pattern is removed when the address is deleted.
- // Attempting to remove it twice is unexpected
+ qdr_address_t *addr = (qdr_address_t *)
qd_parse_tree_remove_pattern_str(core->link_route_tree[dir],
+
pattern);
+ if (!addr) {
+ // expected that the pattern is in the tree.
+ // unexpected if it hasn't been mapped or has already been removed
qd_log(core->log, QD_LOG_CRITICAL, "link route pattern ummap: Pattern
'%s' not found",
pattern);
}
- qd_iterator_free(iter);
free(pattern);
}
diff --git a/tests/parse_tree_tests.c b/tests/parse_tree_tests.c
index 47b5fac..50b1741 100644
--- a/tests/parse_tree_tests.c
+++ b/tests/parse_tree_tests.c
@@ -152,19 +152,20 @@ static char *test_add_and_match_str(void *context)
return NULL;
}
-static char *test_usurpation_recovery_str(void *context)
+static char *test_duplicate_error_str(void *context)
{
+ // While A and B are different strings they are semantically the same
+ // pattern and should trigger duplicate detection in the parse tree
const char *A = "#";
const char *B = "#.#.#.#";
qd_parse_tree_t *node = qd_parse_tree_new(QD_PARSE_TREE_ADDRESS);
void *payload;
- void *usurped;
- void *deposed;
+ qd_error_t rc;
- // rightful owner is ensconsced
- if (qd_parse_tree_add_pattern_str(node, A, (void *)A)) {
+ rc = qd_parse_tree_add_pattern_str(node, A, (void *)A);
+ if (rc) {
qd_parse_tree_free(node);
- return "Add returned existing value (1)";
+ return (char *)qd_error_name(rc);
}
// matches on A or B both return A
@@ -188,11 +189,12 @@ static char *test_usurpation_recovery_str(void *context)
return "Got bad pattern";
}
- // usurper comes along
- usurped = qd_parse_tree_add_pattern_str(node, B, (void *)B);
- if (!usurped || strcmp(A, (char *)usurped)) {
+ // attempt to add B pattern, expect duplication error
+
+ rc = qd_parse_tree_add_pattern_str(node, B, (void *)B);
+ if (!rc) {
qd_parse_tree_free(node);
- return "Usurper should have grabbed '#' optimized match";
+ return "Duplicate pattern NOT detected";
}
// matches on A or B both return B
@@ -201,7 +203,7 @@ static char *test_usurpation_recovery_str(void *context)
return "Could not get pattern";
}
- if (!payload || strcmp(B, (char *)payload)) {
+ if (!payload || strcmp(A, (char *)payload)) {
qd_parse_tree_free(node);
return "Got bad pattern";
}
@@ -211,25 +213,32 @@ static char *test_usurpation_recovery_str(void *context)
return "Could not get pattern";
}
- if (!payload || strcmp(B, (char *)payload)) {
+ if (!payload || strcmp(A, (char *)payload)) {
qd_parse_tree_free(node);
return "Got bad pattern";
}
- // Restore rightful owner
- deposed = qd_parse_tree_add_pattern_str(node, usurped, usurped);
- if (!deposed || strcmp(B, (char *)deposed)) {
+ // now replace A with B correctly
+
+ payload = qd_parse_tree_remove_pattern_str(node, A);
+ if (!payload || strcmp(A, (char *)payload)) {
qd_parse_tree_free(node);
- return "Failed to depose B";
+ return "remove pattern failed";
}
- // matches on A or B both return A
+ rc = qd_parse_tree_add_pattern_str(node, B, (void *)B);
+ if (rc) {
+ qd_parse_tree_free(node);
+ return "Replace add failed";
+ }
+
+ // matches on A or B both return B
if (!qd_parse_tree_retrieve_match_str(node, A, &payload)) {
qd_parse_tree_free(node);
return "Could not get pattern";
}
- if (!payload || strcmp(A, (char *)payload)) {
+ if (!payload || strcmp(B, (char *)payload)) {
qd_parse_tree_free(node);
return "Got bad pattern";
}
@@ -239,7 +248,7 @@ static char *test_usurpation_recovery_str(void *context)
return "Could not get pattern";
}
- if (!payload || strcmp(A, (char *)payload)) {
+ if (!payload || strcmp(B, (char *)payload)) {
qd_parse_tree_free(node);
return "Got bad pattern";
}
@@ -293,7 +302,7 @@ static char *check_normalize(const char *input,
qd_iterator_t *iter = qd_iterator_string(input, ITER_VIEW_ALL);
void *payload;
- if (qd_parse_tree_add_pattern(node, iter, (void *)input) != NULL) {
+ if (qd_parse_tree_add_pattern(node, iter, (void *)input)) {
qd_parse_tree_free(node);
qd_iterator_free(iter);
return "Unexpected duplicate pattern";
@@ -835,7 +844,7 @@ int parse_tree_tests(void)
TEST_CASE(test_add_remove, 0);
TEST_CASE(test_add_and_match_str, 0);
- TEST_CASE(test_usurpation_recovery_str, 0);
+ TEST_CASE(test_duplicate_error_str, 0);
TEST_CASE(test_normalization, 0);
TEST_CASE(test_matches, 0);
TEST_CASE(test_multiple_matches, 0);
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@qpid.apache.org
For additional commands, e-mail: commits-h...@qpid.apache.org
