Return generic result for get operations that includes error messages

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);
 }

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

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<zeek::RecordType>("val_result");
+    auto* result = new zeek::RecordVal(val_result_type);
 
-    if ( res ) {
+    if ( res )
-        result = res.value().get();
+        result->Assign(0, res.value());
-        Ref(result);
-    }
     else
-        result = new StringVal(res.error());
+        result->Assign(1, zeek::make_intrusive<StringVal>(res.error()));
 
     ValComplete(result);
 }

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<RecordType>("val_result");
+	auto val_result = make_intrusive<RecordVal>(val_result_type);
+
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
 	if ( ! b ) {
-		emit_builtin_error("Invalid storage handle", backend);
+		val_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
-		return val_mgr->Bool(false);
+		return val_result;
+	}
+	else if ( ! b->backend->IsOpen() ) {
+		val_result->Assign(1, make_intrusive<StringVal>("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 )
+		if ( ! trigger ) {
-			return val_mgr->Bool(false);
+			val_result->Assign(1, make_intrusive<StringVal>("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()));
+		val_result->Assign(1, make_intrusive<StringVal>(
-		return val_mgr->Bool(false);
+			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

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 41: Failed to retrieve data: Failed to find key
-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 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 51: Invalid storage handle (Storage::close_backend(b2, F) and F)
+error in <...>/storage.zeek, line 54: Invalid storage handle (Storage::close_backend(b2, F) and F)

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=<uninitialized>]

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))

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

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

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=<uninitialized>]
 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

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=<uninitialized>]
 get result same as inserted, T

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=<uninitialized>]
 get result same as inserted, T

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=<uninitialized>]
 get result same as inserted, T

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=<uninitialized>]
 get result same as inserted, T
-get result after expiration, F
+get result after expiration, [val=<uninitialized>, error=Failed to retrieve data: GET returned key didn't exist]

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=<uninitialized>]
 get result same as inserted, T
 overwrite put result, T
-get result, value5678
+get result, [val=value5678, error=<uninitialized>]
-get result same as inserted after overwrite, F
+get result same as inserted, T

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=<uninitialized>]
 get result same as inserted, T

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=<uninitialized>]
 get result same as inserted, T
 closed succesfully

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;

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 ) {
+	if ( res2?$error )
-		print "get result, got empty result";
+		print "get result", res2$error;
-	}
 
 	Storage::close_backend(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() };
 }

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);
 }

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);
 		}

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);
 

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() };
 }

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);
 }

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);
 

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";