From 50ebf865890180791ef4f78354cfb4523ed771f7 Mon Sep 17 00:00:00 2001 From: Chewing_Bever Date: Wed, 7 Dec 2022 12:29:37 +0100 Subject: [PATCH] refactor: started refactoring trie api (contains segfaults) --- CMakeLists.txt | 4 +- src/main.cpp | 60 +++++++++++----------- trie/include/trie.h | 38 +++++++------- trie/src/trie.c | 122 +++++++++++++++++++++----------------------- 4 files changed, 109 insertions(+), 115 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4b4d05d..79f1b4a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -4,7 +4,7 @@ project(lander C CXX) set(CMAKE_C_STANDARD 17) set(CMAKE_CXX_STANDARD 17) -include_directories(crow/include trie/include) +include_directories(trie/include) add_subdirectory(crow) add_subdirectory(trie) @@ -16,5 +16,5 @@ else() endif() add_executable(lander src/main.cpp) - target_link_libraries(lander PUBLIC trie) + target_link_libraries(lander PUBLIC Crow::Crow trie) endif() diff --git a/src/main.cpp b/src/main.cpp index 07db216..a36505f 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -7,6 +7,7 @@ extern "C" { #include "trie.h" } +static const std::string file_path = "lander.data"; static const std::string index_page = R"( @@ -38,17 +39,18 @@ crow::response add_redirect(std::string base_url, Trie *trie, const char *url, // The key already gets copied into the trie, so this pointer is safe to use // ever after unlocking the trie trie_wlock(trie); - char *key = trie_add_random(trie, new_entry, secure); + char *key; + TrieExitCode res = trie_add_random(trie, &key, new_entry, secure); trie_unlock(trie); - if (key == NULL) { + if (res != Ok) { return crow::response(crow::status::INTERNAL_SERVER_ERROR); } - std::string res = base_url + key; + std::string out = base_url + key; free(key); - return crow::response(res); + return crow::response(out); } bool store_paste(const char *key, const char *body) { @@ -71,10 +73,11 @@ crow::response add_paste(std::string base_url, Trie *trie, const char *body, Entry *new_entry = entry_new(Paste, ""); trie_wlock(trie); - char *key = trie_add_random(trie, new_entry, secure); + char *key; + TrieExitCode res = trie_add_random(trie, &key, new_entry, secure); trie_unlock(trie); - if (key == NULL) { + if (res != Ok) { return crow::response(crow::status::INTERNAL_SERVER_ERROR); } @@ -82,10 +85,10 @@ crow::response add_paste(std::string base_url, Trie *trie, const char *body, return crow::response(crow::status::INTERNAL_SERVER_ERROR); } - std::string res = base_url + key; + std::string out = base_url + key; free(key); - return crow::response(res); + return crow::response(out); } int main() { @@ -95,26 +98,21 @@ int main() { ENV(api_key, "LANDER_API_KEY"); ENV(base_url, "LANDER_BASE_URL"); - // Initialize trie and populate from data file - Trie *trie = trie_init(); - - std::string file_path = "lander.data"; - - std::cout << "Populating trie from file '" << file_path << "'..." + std::cout << "Initializing trie from file '" << file_path << "'..." << std::endl; - // Web server hasn't started yet, so there's no point in locking the trie - int count = trie_populate(trie, file_path.c_str()); + // Initialize trie and populate from data file + Trie *trie; + int res = trie_init(&trie, file_path.c_str()); - if (count == -1) { - std::cout << "An error occured while populating the trie." << std::endl; + if (res != 0) { + std::cout << "An error occured while initializing the trie." << std::endl; exit(1); - } else { - std::cout << "Added " << count << " (" << trie_size(trie) - << ") entries to trie." << std::endl; } + std::cout << "Added " << trie_size(trie) << " entries to trie." << std::endl; + // Create pastes directory if not present // TODO don't just ignore errors here mkdir("pastes", 0700); @@ -130,9 +128,10 @@ int main() { .methods(crow::HTTPMethod::Get)( [trie](crow::response &res, std::string key) { trie_rlock(trie); - Entry *entry = trie_search(trie, key.c_str()); + Entry *entry; + TrieExitCode status = trie_search(trie, &entry, key.c_str()); - if (entry != NULL) { + if (status == Ok) { if (entry->type == Redirect) { res.redirect(entry->string); } else if (entry->type == Paste) { @@ -173,14 +172,17 @@ int main() { Entry *new_entry = entry_new(Redirect, req.body.c_str()); trie_wlock(trie); - bool added = trie_add(trie, key.c_str(), new_entry); + TrieExitCode status = trie_add(trie, key.c_str(), new_entry); trie_unlock(trie); - if (!added) { + switch (status) { + case Ok: + return crow::response(base_url + key); + case AlreadyPresent: return crow::response(crow::status::CONFLICT); + default: + return crow::response(crow::status::INTERNAL_SERVER_ERROR); } - - return crow::response(base_url + key); }); // Add a new Paste with a short randomly generated key @@ -209,10 +211,10 @@ int main() { Entry *new_entry = entry_new(Paste, ""); trie_wlock(trie); - bool added = trie_add(trie, key.c_str(), new_entry); + TrieExitCode status = trie_add(trie, key.c_str(), new_entry); trie_unlock(trie); - if (!added) { + if (status != Ok) { return crow::response(crow::status::CONFLICT); } diff --git a/trie/include/trie.h b/trie/include/trie.h index 82a0f33..c32dadc 100644 --- a/trie/include/trie.h +++ b/trie/include/trie.h @@ -41,23 +41,22 @@ typedef struct entry { char *string; } Entry; +typedef enum trie_exit_code { + Ok = 0, + NotFound, + AlreadyPresent, + FileError +} TrieExitCode; + Entry *entry_new(EntryType type, const char *string); /** - * Allocate and initialize an empty Trie. + * Allocate & initialize a new trie, and populate it with the data from the + * given data file. * - * @return a pointer to an empty Trie struct + * @return 0 if everything was successful, non-zero otherwise */ -Trie *trie_init(); - -/** - * Populate trie with entries stored in the given file. - * - * @param trie - * @param file_path path to file containing entries - * @return amount of entries added; -1 if an error occured - */ -int trie_populate(Trie *trie, const char *file_path); +TrieExitCode trie_init(Trie **trie_ptr, const char *file_path); /** * De-allocate a trie by freeing the memory occupied by this trie. @@ -70,10 +69,11 @@ void trie_free(Trie *trie); * Search for an entry in the trie. * * @param trie + * @param entry_ptr pointer to Entry will be stored here, if found * @param key key representing the entry - * @return pointer to entry; NULL if not found + * @return 0 if the search was successful, 1 if not found */ -Entry *trie_search(Trie *trie, const char *key); +TrieExitCode trie_search(Trie *trie, Entry **entry_ptr, const char *key); /** * Add a string to this trie. @@ -81,12 +81,9 @@ Entry *trie_search(Trie *trie, const char *key); * @param trie * @param key key to represent entry with * @param entry entry to add - * @return true if the trie was changed by this operation, false if it was - * already present + * @return 0 if added, 1 if already in trie, something else if other errors */ -bool trie_add(Trie *trie, const char *key, Entry *entry); - -bool trie_add_no_lock(Trie *trie, const char *key, Entry *entry); +TrieExitCode trie_add(Trie *trie, const char *key, Entry *entry); /** * Add an entry by generating a random string as the key. @@ -97,7 +94,8 @@ bool trie_add_no_lock(Trie *trie, const char *key, Entry *entry); * @return pointer to the generated key. This pointer is safe to use after * unlocking the trie, and should be freed manually. */ -char *trie_add_random(Trie *trie, Entry *entry, bool secure); +TrieExitCode trie_add_random(Trie *trie, char **key_ptr, Entry *entry, + bool secure); /** * Remove an entry from this trie given its key. diff --git a/trie/src/trie.c b/trie/src/trie.c index 9f49aef..7276e58 100644 --- a/trie/src/trie.c +++ b/trie/src/trie.c @@ -15,39 +15,25 @@ typedef struct ttrie { pthread_rwlock_t lock; } Trie; +TrieExitCode trie_add_no_lock(Trie *trie, const char *key, Entry *entry); + /** * Allocate and initialize an empty Trie * * @return pointer to the empty Trie */ -Trie *trie_init() { +TrieExitCode trie_init(Trie **trie_ptr, const char *file_path) { + // Allocate & initialize trie Trie *trie = calloc(1, sizeof(Trie)); trie->root = tnode_init(); + trie->file_path = strdup(file_path); pthread_rwlock_init(&trie->lock, NULL); - return trie; -} - -/** - * De-allocate a TernaryTree by freeing its entire underlying structure. - * - * @param trie trie to free - */ -void trie_free(Trie *trie) { - tnode_free(trie->root); - free(trie); -} - -bool trie_add_no_lock(Trie *trie, const char *key, Entry *entry); - -int trie_populate(Trie *trie, const char *file_path) { - trie->file_path = strdup(file_path); - + // Populate trie with data from file FILE *fp = fopen(file_path, "r"); - // TODO properly handle this if (fp == NULL) { - return -1; + return FileError; } // We read in lines of at most 8192 characters (sounds like enough) @@ -89,7 +75,19 @@ int trie_populate(Trie *trie, const char *file_path) { fclose(fp); - return entries; + *trie_ptr = trie; + + return Ok; +} + +/** + * De-allocate a TernaryTree by freeing its entire underlying structure. + * + * @param trie trie to free + */ +void trie_free(Trie *trie) { + tnode_free(trie->root); + free(trie); } typedef struct searchresult { @@ -145,16 +143,16 @@ SearchResult trie_search_node(Trie *trie, const char *key) { * @param string string to look up * @return true if the string is present in the trie, false otherwise */ -Entry *trie_search(Trie *trie, const char *key) { +TrieExitCode trie_search(Trie *trie, Entry **entry_ptr, const char *key) { SearchResult res = trie_search_node(trie, key); - Entry *return_value = NULL; - - if (res.child != NULL) { - return_value = res.child->entry; + if (res.child == NULL) { + return NotFound; } - return return_value; + *entry_ptr = res.child->entry; + + return Ok; } /** @@ -165,7 +163,7 @@ Entry *trie_search(Trie *trie, const char *key) { * @return true if the string wasn't present in the trie and thus added, false * otherwise */ -bool trie_add_no_lock(Trie *trie, const char *string, Entry *entry) { +TrieExitCode trie_add_no_lock(Trie *trie, const char *string, Entry *entry) { size_t i = 0; uint8_t offset; TrieNode **node_ptr = &(trie->root); @@ -207,7 +205,7 @@ bool trie_add_no_lock(Trie *trie, const char *string, Entry *entry) { child_node->entry = entry; trie->size++; - return true; + return Ok; } while (offset < (*child_node_ptr)->string_len) { @@ -258,43 +256,44 @@ bool trie_add_no_lock(Trie *trie, const char *string, Entry *entry) { } while (string[i] != DELIMITER); if ((*child_node_ptr)->represents) { - return false; + return AlreadyPresent; } (*child_node_ptr)->represents = true; trie->size++; - return true; + return Ok; } -bool trie_add(Trie *trie, const char *key, Entry *entry) { - if (trie->file_path != NULL) { - // Easiest way to make sure we don't add duplicate entries - // We use an internal function that doesn't require a read lock, as we're - // already inside a write lock - if (trie_search_node(trie, key).child != NULL) { - return false; - } - - FILE *fp = fopen(trie->file_path, "a"); - - if (fp == NULL) { - return false; - } - - fputs(key, fp); - fputs(" ", fp); - fputc(entry_type_to_char(entry->type), fp); - fputs(" ", fp); - fputs(entry->string, fp); - fputs("\n", fp); +TrieExitCode trie_add(Trie *trie, const char *key, Entry *entry) { + // Easiest way to make sure we don't add duplicate entries + // We use an internal function that doesn't require a read lock, as we're + // already inside a write lock + if (trie_search_node(trie, key).child != NULL) { + return AlreadyPresent; } - // This function *should* always return true. Otherwise, the function would've + FILE *fp = fopen(trie->file_path, "a"); + + if (fp == NULL) { + return FileError; + } + + fputs(key, fp); + fputs(" ", fp); + fputc(entry_type_to_char(entry->type), fp); + fputs(" ", fp); + fputs(entry->string, fp); + fputs("\n", fp); + + fclose(fp); + + // This function *should* always return Ok. Otherwise, the function would've // exited because the string was found in the trie. return trie_add_no_lock(trie, key, entry); } -char *trie_add_random(Trie *trie, Entry *entry, bool secure) { +TrieExitCode trie_add_random(Trie *trie, char **key_ptr, Entry *entry, + bool secure) { // Generate random key bool ok = false; int key_length = secure ? RANDOM_KEY_LENGTH_LONG : RANDOM_KEY_LENGTH_SHORT; @@ -312,13 +311,11 @@ char *trie_add_random(Trie *trie, Entry *entry, bool secure) { ok = trie_search_node(trie, key).child == NULL; } - bool res = trie_add(trie, key, entry); - char *return_value; + TrieExitCode return_value = trie_add(trie, key, entry); - if (res) { - return_value = key; + if (return_value == Ok) { + *key_ptr = key; } else { - return_value = NULL; free(key); } @@ -398,7 +395,4 @@ int trie_rlock(Trie *trie) { return pthread_rwlock_rdlock(&trie->lock); } int trie_wlock(Trie *trie) { return pthread_rwlock_wrlock(&trie->lock); } -int trie_unlock(Trie *trie) { - printf("sup\n"); - return pthread_rwlock_unlock(&trie->lock); -} +int trie_unlock(Trie *trie) { return pthread_rwlock_unlock(&trie->lock); }