From f1a7376e0ad9e9ad23bd61f93146ba3ed810e8fd Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Wed, 22 Jan 2025 17:00:38 -0700 Subject: [PATCH] Return generic result for get operations that includes error messages --- scripts/base/frameworks/storage/main.zeek | 8 +++-- scripts/base/init-bare.zeek | 8 +++++ src/storage/Backend.cc | 11 ++++--- src/storage/storage.bif | 29 ++++++++++++------- .../Baseline/plugins.storage/zeek-stderr | 6 ++-- .../out | 4 +-- .../.stderr | 1 - .../scripts.base.frameworks.storage.erase/out | 2 +- .../.stderr | 1 - .../out | 4 +-- .../out | 2 +- .../out | 2 +- .../out | 2 +- .../out | 4 +-- .../out | 6 ++-- .../out | 2 +- .../out | 2 +- testing/btest/plugins/storage.zeek | 5 +++- .../base/frameworks/storage/erase.zeek | 5 ++-- .../base/frameworks/storage/expiration.zeek | 8 +++-- .../base/frameworks/storage/overwriting.zeek | 3 +- .../storage/redis-async-reading-pcap.zeek | 3 +- .../base/frameworks/storage/redis-async.zeek | 3 +- .../frameworks/storage/redis-expiration.zeek | 3 +- .../base/frameworks/storage/redis-sync.zeek | 6 ++-- .../storage/sqlite-basic-reading-pcap.zeek | 3 +- .../base/frameworks/storage/sqlite-basic.zeek | 3 +- 27 files changed, 83 insertions(+), 53 deletions(-) diff --git a/scripts/base/frameworks/storage/main.zeek b/scripts/base/frameworks/storage/main.zeek index a1d478ca78..b0c725f9b2 100644 --- a/scripts/base/frameworks/storage/main.zeek +++ b/scripts/base/frameworks/storage/main.zeek @@ -95,9 +95,11 @@ export { ## Returns: A boolean indicating success or failure of the operation. Type ## comparison failures against the types passed to ## :zeek:see:`Storage::open_backend` for the backend will cause ``F`` to - ## be returned. + ## be returned. The caller should check the validity of the value before + ## attempting to use it. If the value is unset, an error string may be + ## available to describe the failure. global get: function(backend: opaque of Storage::BackendHandle, key: any, - async_mode: bool &default=T): any; + async_mode: bool &default=T): val_result; ## Erases an entry from the backend. ## @@ -134,7 +136,7 @@ function put(backend: opaque of Storage::BackendHandle, args: Storage::PutArgs): return Storage::__put(backend, args$key, args$value, args$overwrite, args$expire_time, args$async_mode); } -function get(backend: opaque of Storage::BackendHandle, key: any, async_mode: bool &default=T): any +function get(backend: opaque of Storage::BackendHandle, key: any, async_mode: bool &default=T): val_result { return Storage::__get(backend, key, async_mode); } diff --git a/scripts/base/init-bare.zeek b/scripts/base/init-bare.zeek index 9593efd0f1..a94d29e006 100644 --- a/scripts/base/init-bare.zeek +++ b/scripts/base/init-bare.zeek @@ -356,6 +356,14 @@ type ftp_port: record { valid: bool; ##< True if format was right. Only then are *h* and *p* valid. }; +## A generic type for returning either a value or an error string from a +## function or a BIF method. This is sort of equivalent to std::expected +## in C++. +type val_result: record { + val: any &optional; + error: string &optional; +}; + ## Statistics about what a TCP endpoint sent. ## ## .. zeek:see:: conn_stats diff --git a/src/storage/Backend.cc b/src/storage/Backend.cc index b4437b2f6f..5a94ba35e0 100644 --- a/src/storage/Backend.cc +++ b/src/storage/Backend.cc @@ -44,14 +44,13 @@ ValResultCallback::ValResultCallback(zeek::detail::trigger::TriggerPtr trigger, : ResultCallback(std::move(trigger), assoc) {} void ValResultCallback::Complete(const ValResult& res) { - zeek::Val* result; + static auto val_result_type = zeek::id::find_type("val_result"); + auto* result = new zeek::RecordVal(val_result_type); - if ( res ) { - result = res.value().get(); - Ref(result); - } + if ( res ) + result->Assign(0, res.value()); else - result = new StringVal(res.error()); + result->Assign(1, zeek::make_intrusive(res.error())); ValComplete(result); } diff --git a/src/storage/storage.bif b/src/storage/storage.bif index 1a606f1e44..a5cf473ae6 100644 --- a/src/storage/storage.bif +++ b/src/storage/storage.bif @@ -149,22 +149,29 @@ function Storage::__put%(backend: opaque of Storage::BackendHandle, key: any, va return val_mgr->Bool(true); %} -function Storage::__get%(backend: opaque of Storage::BackendHandle, key: any, async_mode: bool &default=T%): any +function Storage::__get%(backend: opaque of Storage::BackendHandle, key: any, async_mode: bool &default=T%): val_result %{ + static auto val_result_type = id::find_type("val_result"); + auto val_result = make_intrusive(val_result_type); + auto b = dynamic_cast(backend); if ( ! b ) { - emit_builtin_error("Invalid storage handle", backend); - return val_mgr->Bool(false); + val_result->Assign(1, make_intrusive("Invalid storage handlle")); + return val_result; + } + else if ( ! b->backend->IsOpen() ) { + val_result->Assign(1, make_intrusive("Backend is closed")); + return val_result; } - else if ( ! b->backend->IsOpen() ) - return val_mgr->Bool(false); ValResultCallback* cb = nullptr; if ( async_mode ) { auto trigger = init_trigger(frame, b->backend); - if ( ! trigger ) - return val_mgr->Bool(false); + if ( ! trigger ) { + val_result->Assign(1, make_intrusive("Failed to create trigger")); + return val_result; + } cb = new ValResultCallback(trigger, frame->GetTriggerAssoc()); } @@ -176,11 +183,13 @@ function Storage::__get%(backend: opaque of Storage::BackendHandle, key: any, as return nullptr; if ( ! result.has_value() ) { - emit_builtin_error(util::fmt("Failed to retrieve data: %s", result.error().c_str())); - return val_mgr->Bool(false); + val_result->Assign(1, make_intrusive( + util::fmt("Failed to retrieve data: %s", result.error().c_str()))); + return val_result; } - return result.value(); + val_result->Assign(0, result.value()); + return val_result; %} function Storage::__erase%(backend: opaque of Storage::BackendHandle, key: any, async_mode: bool &default=T%): bool diff --git a/testing/btest/Baseline/plugins.storage/zeek-stderr b/testing/btest/Baseline/plugins.storage/zeek-stderr index a039e08e9f..590aa27896 100644 --- a/testing/btest/Baseline/plugins.storage/zeek-stderr +++ b/testing/btest/Baseline/plugins.storage/zeek-stderr @@ -1,4 +1,4 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. -error in <...>/storage.zeek, line 37: Failed to retrieve data: Failed to find key (Storage::get(b, to_any_coerce key, F)) -error in <...>/storage.zeek, line 50: Failed to open backend Storage::STORAGEDUMMY: open_fail was set to true, returning error (Storage::open_backend(Storage::STORAGEDUMMY, to_any_coerce opts, to_any_coerce str, to_any_coerce str, F)) -error in <...>/storage.zeek, line 51: Invalid storage handle (Storage::close_backend(b2, F) and F) +error in <...>/storage.zeek, line 41: Failed to retrieve data: Failed to find key +error in <...>/storage.zeek, line 53: Failed to open backend Storage::STORAGEDUMMY: open_fail was set to true, returning error (Storage::open_backend(Storage::STORAGEDUMMY, to_any_coerce opts, to_any_coerce str, to_any_coerce str, F)) +error in <...>/storage.zeek, line 54: Invalid storage handle (Storage::close_backend(b2, F) and F) diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.compound-types/out b/testing/btest/Baseline/scripts.base.frameworks.storage.compound-types/out index e8f3abe594..0b2661d2e3 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.compound-types/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.compound-types/out @@ -1,7 +1,7 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. put result, T -get result, { +get result, [val={ [2] = b, [1] = a, [3] = c -} +}, error=] diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.erase/.stderr b/testing/btest/Baseline/scripts.base.frameworks.storage.erase/.stderr index 886fb29d21..49d861c74c 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.erase/.stderr +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.erase/.stderr @@ -1,2 +1 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. -error in <...>/erase.zeek, line 28: Failed to retrieve data: Failed to find row for key: no more rows available (Storage::get(b, to_any_coerce key, F)) diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.erase/out b/testing/btest/Baseline/scripts.base.frameworks.storage.erase/out index 056ba93c85..7ca5354b61 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.erase/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.erase/out @@ -1,3 +1,3 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. erase result, T -get result, got empty result +get result, Failed to retrieve data: Failed to find row for key: no more rows available diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.expiration/.stderr b/testing/btest/Baseline/scripts.base.frameworks.storage.expiration/.stderr index 65a0a4d3ed..97188487be 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.expiration/.stderr +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.expiration/.stderr @@ -1,3 +1,2 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. -1627225025.686472 error in <...>/expiration.zeek, line 20: Failed to retrieve data: Failed to find row for key: no more rows available (Storage::get(backend, to_any_coerce key, F)) 1627225025.686472 received termination signal diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.expiration/out b/testing/btest/Baseline/scripts.base.frameworks.storage.expiration/out index f75c9fb9cc..08758e1bf2 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.expiration/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.expiration/out @@ -1,5 +1,5 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. put result, T -get result, value7890 +get result, [val=value7890, error=] get result same as inserted, T -get result, F +get result, Failed to retrieve data: Failed to find row for key: no more rows available diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.overwriting/out b/testing/btest/Baseline/scripts.base.frameworks.storage.overwriting/out index 809959c28f..d1338d7ba9 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.overwriting/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.overwriting/out @@ -1,4 +1,4 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. put result, T -get result, value7890 +get result, [val=value7890, error=] get result same as inserted, T diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.redis-async-reading-pcap/out b/testing/btest/Baseline/scripts.base.frameworks.storage.redis-async-reading-pcap/out index 5125c8494e..b32c104878 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.redis-async-reading-pcap/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.redis-async-reading-pcap/out @@ -1,4 +1,4 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. put result, T -get result, value5678 +get result, [val=value5678, error=] get result same as inserted, T diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.redis-async/out b/testing/btest/Baseline/scripts.base.frameworks.storage.redis-async/out index 5125c8494e..b32c104878 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.redis-async/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.redis-async/out @@ -1,4 +1,4 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. put result, T -get result, value5678 +get result, [val=value5678, error=] get result same as inserted, T diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.redis-expiration/out b/testing/btest/Baseline/scripts.base.frameworks.storage.redis-expiration/out index 88d16bb4e3..996cf2ebac 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.redis-expiration/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.redis-expiration/out @@ -1,5 +1,5 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. put result, T -get result, value7890 +get result, [val=value7890, error=] get result same as inserted, T -get result after expiration, F +get result after expiration, [val=, error=Failed to retrieve data: GET returned key didn't exist] diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.redis-sync/out b/testing/btest/Baseline/scripts.base.frameworks.storage.redis-sync/out index e7d5445bc7..b77104dcd8 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.redis-sync/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.redis-sync/out @@ -1,7 +1,7 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. put result, T -get result, value1234 +get result, [val=value1234, error=] get result same as inserted, T overwrite put result, T -get result, value5678 -get result same as inserted after overwrite, F +get result, [val=value5678, error=] +get result same as inserted, T diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-basic-reading-pcap/out b/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-basic-reading-pcap/out index 5125c8494e..b32c104878 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-basic-reading-pcap/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-basic-reading-pcap/out @@ -1,4 +1,4 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. put result, T -get result, value5678 +get result, [val=value5678, error=] get result same as inserted, T diff --git a/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-basic/out b/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-basic/out index e32898816e..1eca70373d 100644 --- a/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-basic/out +++ b/testing/btest/Baseline/scripts.base.frameworks.storage.sqlite-basic/out @@ -1,6 +1,6 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. open successful put result, T -get result, value5678 +get result, [val=value5678, error=] get result same as inserted, T closed succesfully diff --git a/testing/btest/plugins/storage.zeek b/testing/btest/plugins/storage.zeek index d759e4b7da..fd57797252 100644 --- a/testing/btest/plugins/storage.zeek +++ b/testing/btest/plugins/storage.zeek @@ -37,13 +37,16 @@ event zeek_init() { get_res = Storage::get(b, key, F); Storage::close_backend(b); + if ( get_res?$error ) + Reporter::error(get_res$error); + # Test attempting to use the closed handle. put_res = Storage::put(b, [$key="a", $value="b", $overwrite=F]); get_res = Storage::get(b, "a"); erase_res = Storage::erase(b, "a"); print(fmt("results of trying to use closed handle: get: %d, put: %d, erase: %d", - get_res, put_res, erase_res)); + get_res?$val, put_res, erase_res)); # Test failing to open the handle and test closing an invalid handle. opts$open_fail = T; diff --git a/testing/btest/scripts/base/frameworks/storage/erase.zeek b/testing/btest/scripts/base/frameworks/storage/erase.zeek index 515f2ab2ea..b6a3e540fc 100644 --- a/testing/btest/scripts/base/frameworks/storage/erase.zeek +++ b/testing/btest/scripts/base/frameworks/storage/erase.zeek @@ -26,9 +26,8 @@ event zeek_init() { print "erase result", res; local res2 = Storage::get(b, key, F); - if ( ! res2 as bool ) { - print "get result, got empty result"; - } + if ( res2?$error ) + print "get result", res2$error; Storage::close_backend(b); } diff --git a/testing/btest/scripts/base/frameworks/storage/expiration.zeek b/testing/btest/scripts/base/frameworks/storage/expiration.zeek index 46fb2be5a8..0913468004 100644 --- a/testing/btest/scripts/base/frameworks/storage/expiration.zeek +++ b/testing/btest/scripts/base/frameworks/storage/expiration.zeek @@ -17,8 +17,11 @@ global key: string = "key1234"; global value: string = "value7890"; event check_removed() { + # This should return an error from the sqlite backend that there aren't any more + # rows available. local res2 = Storage::get(backend, key, F); - print "get result", res2; + if ( res2?$error ) + print "get result", res2$error; Storage::close_backend(backend); terminate(); @@ -36,7 +39,8 @@ event setup_test() { local res2 = Storage::get(backend, key, F); print "get result", res2; - print "get result same as inserted", value == (res2 as string); + if ( res2?$val ) + print "get result same as inserted", value == (res2$val as string); schedule 5 secs { check_removed() }; } diff --git a/testing/btest/scripts/base/frameworks/storage/overwriting.zeek b/testing/btest/scripts/base/frameworks/storage/overwriting.zeek index 82b667b2b9..60211d1e2e 100644 --- a/testing/btest/scripts/base/frameworks/storage/overwriting.zeek +++ b/testing/btest/scripts/base/frameworks/storage/overwriting.zeek @@ -25,7 +25,8 @@ event zeek_init() { local res2 = Storage::get(b, key, F); print "get result", res2; - print "get result same as inserted", value == (res2 as string); + if ( res2?$val ) + print "get result same as inserted", value == (res2$val as string); Storage::close_backend(b); } diff --git a/testing/btest/scripts/base/frameworks/storage/redis-async-reading-pcap.zeek b/testing/btest/scripts/base/frameworks/storage/redis-async-reading-pcap.zeek index 0cd3dcc2b9..310e98fc4c 100644 --- a/testing/btest/scripts/base/frameworks/storage/redis-async-reading-pcap.zeek +++ b/testing/btest/scripts/base/frameworks/storage/redis-async-reading-pcap.zeek @@ -35,7 +35,8 @@ event zeek_init() { when [b, key, value] ( local res2 = Storage::get(b, key) ) { print "get result", res2; - print "get result same as inserted", value == (res2 as string); + if ( res2?$val ) + print "get result same as inserted", value == (res2$val as string); Storage::close_backend(b); } diff --git a/testing/btest/scripts/base/frameworks/storage/redis-async.zeek b/testing/btest/scripts/base/frameworks/storage/redis-async.zeek index 47f913e5ae..ae0da73906 100644 --- a/testing/btest/scripts/base/frameworks/storage/redis-async.zeek +++ b/testing/btest/scripts/base/frameworks/storage/redis-async.zeek @@ -37,7 +37,8 @@ event zeek_init() { when [b, key, value] ( local res2 = Storage::get(b, key) ) { print "get result", res2; - print "get result same as inserted", value == (res2 as string); + if ( res2?$val ) + print "get result same as inserted", value == (res2$val as string); Storage::close_backend(b); diff --git a/testing/btest/scripts/base/frameworks/storage/redis-expiration.zeek b/testing/btest/scripts/base/frameworks/storage/redis-expiration.zeek index 810bbb102d..dbc050debf 100644 --- a/testing/btest/scripts/base/frameworks/storage/redis-expiration.zeek +++ b/testing/btest/scripts/base/frameworks/storage/redis-expiration.zeek @@ -47,7 +47,8 @@ event setup_test() { local res2 = Storage::get(b, key, F); print "get result", res2; - print "get result same as inserted", value == (res2 as string); + if ( res2?$val ) + print "get result same as inserted", value == (res2$val as string); schedule 5 secs { check_removed() }; } diff --git a/testing/btest/scripts/base/frameworks/storage/redis-sync.zeek b/testing/btest/scripts/base/frameworks/storage/redis-sync.zeek index d21d5979a8..d9dd372544 100644 --- a/testing/btest/scripts/base/frameworks/storage/redis-sync.zeek +++ b/testing/btest/scripts/base/frameworks/storage/redis-sync.zeek @@ -35,7 +35,8 @@ event zeek_init() { local res2 = Storage::get(b, key, F); print "get result", res2; - print "get result same as inserted", value == (res2 as string); + if ( res2?$val ) + print "get result same as inserted", value == (res2$val as string); local value2 = "value5678"; res = Storage::put(b, [$key=key, $value=value2, $overwrite=T, $async_mode=F]); @@ -43,7 +44,8 @@ event zeek_init() { res2 = Storage::get(b, key, F); print "get result", res2; - print "get result same as inserted after overwrite", value == (res2 as string); + if ( res2?$val ) + print "get result same as inserted", value2 == (res2$val as string); Storage::close_backend(b); } diff --git a/testing/btest/scripts/base/frameworks/storage/sqlite-basic-reading-pcap.zeek b/testing/btest/scripts/base/frameworks/storage/sqlite-basic-reading-pcap.zeek index dbd00662b3..6f2bc2d3a5 100644 --- a/testing/btest/scripts/base/frameworks/storage/sqlite-basic-reading-pcap.zeek +++ b/testing/btest/scripts/base/frameworks/storage/sqlite-basic-reading-pcap.zeek @@ -29,7 +29,8 @@ event zeek_init() { when [b, key, value] ( local res2 = Storage::get(b, key) ) { print "get result", res2; - print "get result same as inserted", value == (res2 as string); + if ( res2?$val ) + print "get result same as inserted", value == (res2$val as string); Storage::close_backend(b); diff --git a/testing/btest/scripts/base/frameworks/storage/sqlite-basic.zeek b/testing/btest/scripts/base/frameworks/storage/sqlite-basic.zeek index 224eda3223..684c86bce0 100644 --- a/testing/btest/scripts/base/frameworks/storage/sqlite-basic.zeek +++ b/testing/btest/scripts/base/frameworks/storage/sqlite-basic.zeek @@ -30,7 +30,8 @@ event zeek_init() { when [b, key, value] ( local get_res = Storage::get(b, key) ) { print "get result", get_res; - print "get result same as inserted", value == (get_res as string); + if ( get_res?$val ) + print "get result same as inserted", value == (get_res$val as string); when [b] ( local close_res = Storage::close_backend(b, T) ) { print "closed succesfully";