Skip to content

Commit

Permalink
libart: better defined behavior when a full prefix key is encountered
Browse files Browse the repository at this point in the history
Summary: a facet of the libart implementation that is undocumented
except for a series of issues (linked below) is this: an inserted key
must not be a full prefix of another key that is already stored in the
tree.  The recommendation is to store keys that have a terminator
character.

armon/libart#4
armon/libart#12
armon/libart#14
armon/libart#17

Our usage in watchman can't guarantee to provide a NUL-terminated input,
so this diff adjusts the key comparison routines to generate an implicit
or synthetic NUL terminator character when comparing one character
beyond the end of a key, and by asserting (or allowing ASAN to complain)
if we try to look more than 1 character beyond the end.

I also tidied up the function signatures of a couple of matching
functions so that the semantics are clearer (return boolean true for
success rather than integer 0) and removed some redundant casts when
invoking callbacks.

Test Plan: added an explicit test for the full-prefix issue.

Built with -fsanitize=address and ran the integration tests as well as
the sparse/unsparse checks I've been using for the pending list changes.
  • Loading branch information
wez committed Jun 1, 2016
1 parent e94c806 commit 6e1ab78
Show file tree
Hide file tree
Showing 6 changed files with 204 additions and 71 deletions.
3 changes: 2 additions & 1 deletion Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,8 @@ integration: all py-integration
tests_art_t_CPPFLAGS = $(THIRDPARTY_CPPFLAGS)
tests_art_t_LDADD = $(ART_LIB) $(TAP_LIB)
tests_art_t_SOURCES = \
tests/art_test.c
tests/art_test.c \
log.c

tests_argv_t_CPPFLAGS = $(THIRDPARTY_CPPFLAGS)
tests_argv_t_LDADD = $(JSON_LIB) $(TAP_LIB)
Expand Down
46 changes: 45 additions & 1 deletion tests/art_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,26 @@

#include "thirdparty/tap.h"
#include "thirdparty/libart/src/art.h"
#include "watchman.h"

#define stringy2(line) #line
#define stringy(line) stringy2(line)
#define fail_unless(__x) ok(__x, __FILE__ ":" stringy(__LINE__) " " # __x)

void w_request_shutdown(void) {}

bool w_should_log_to_clients(int level)
{
unused_parameter(level);
return false;
}

void w_log_to_clients(int level, const char *buf)
{
unused_parameter(level);
unused_parameter(buf);
}

void test_art_init_and_destroy(void) {
art_tree t;
int res = art_tree_init(&t);
Expand Down Expand Up @@ -430,6 +445,34 @@ void test_art_long_prefix(void) {
fail_unless(res == 0);
}

static int dump_iter(void *data, const unsigned char *key, unsigned int key_len,
void *value) {
diag("iter leaf: data=%p key_len=%d %.*s value=%p", data, (int)key_len,
(int)key_len, key, value);
return 0;
}

void test_art_prefix(void) {
art_tree t;
void *v;

art_tree_init(&t);

fail_unless(art_insert(&t, (const unsigned char*)"food", 4, "food") == NULL);
fail_unless(art_insert(&t, (const unsigned char*)"foo", 3, "foo") == NULL);
diag("size is now %d", art_size(&t));
fail_unless(art_size(&t) == 2);
fail_unless((v = art_search(&t, (const unsigned char*)"food", 4)) != NULL);
diag("food lookup yields %s", v);
fail_unless(v && strcmp((char*)v, "food") == 0);

art_iter(&t, dump_iter, NULL);

fail_unless((v = art_search(&t, (const unsigned char*)"foo", 3)) != NULL);
diag("foo lookup yields %s", v);
fail_unless(v && strcmp((char*)v, "foo") == 0);
}

void test_art_insert_search_uuid(void) {
art_tree t;
art_leaf *l;
Expand Down Expand Up @@ -486,7 +529,7 @@ int main(int argc, char **argv) {
(void)argc;
(void)argv;

plan_tests(109);
plan_tests(116);
test_art_init_and_destroy();
test_art_insert();
test_art_insert_verylong();
Expand All @@ -496,6 +539,7 @@ int main(int argc, char **argv) {
test_art_iter_prefix();
test_art_long_prefix();
test_art_insert_search_uuid();
test_art_prefix();

return exit_status();
}
1 change: 0 additions & 1 deletion tests/ignore_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

#include "watchman.h"
#include "thirdparty/tap.h"
#include "thirdparty/libart/src/art.h"

// A list that looks similar to one used in one of our repos
const char *ignore_dirs[] = {".buckd",
Expand Down
Loading

0 comments on commit 6e1ab78

Please sign in to comment.