Add OperationResult::MakeVal, use it to reduce some code duplication

src/storage/Backend.cc

@@ -9,15 +9,17 @@
 
 namespace zeek::storage {
 
-RecordValPtr OperationResult::BuildVal() {
+RecordValPtr OperationResult::BuildVal() { return MakeVal(code, err_str, value); }
+
+RecordValPtr OperationResult::MakeVal(EnumValPtr code, std::string_view err_str, ValPtr value) {
     static auto op_result_type = zeek::id::find_type<zeek::RecordType>("Storage::OperationResult");
 
     auto rec = zeek::make_intrusive<zeek::RecordVal>(op_result_type);
-    rec->Assign(0, code);
+    rec->Assign(0, std::move(code));
     if ( ! err_str.empty() )
-        rec->Assign(1, err_str);
+        rec->Assign(1, std::string{err_str});
     if ( value )
-        rec->Assign(2, value);
+        rec->Assign(2, std::move(value));
 
     return rec;
 }
@@ -28,12 +30,8 @@ ResultCallback::ResultCallback(zeek::detail::trigger::TriggerPtr trigger, const
 void ResultCallback::Timeout() {
     static const auto& op_result_type = zeek::id::find_type<zeek::RecordType>("Storage::OperationResult");
 
-    if ( ! IsSyncCallback() ) {
+    if ( ! IsSyncCallback() )
-        auto op_result = make_intrusive<RecordVal>(op_result_type);
+        trigger->Cache(assoc, OperationResult::MakeVal(ReturnCode::TIMEOUT).release());
-        op_result->Assign(0, ReturnCode::TIMEOUT);
-
-        trigger->Cache(assoc, op_result.release());
-    }
 }
 
 OperationResultCallback::OperationResultCallback(zeek::detail::trigger::TriggerPtr trigger, const void* assoc)
@@ -46,19 +44,9 @@ void OperationResultCallback::Complete(OperationResult res) {
         return;
     }
 
-    static auto op_result_type = zeek::id::find_type<zeek::RecordType>("Storage::OperationResult");
+    auto res_val = res.BuildVal();
-    auto* op_result = new zeek::RecordVal(op_result_type);
+    trigger->Cache(assoc, res_val.get());
-
-    op_result->Assign(0, res.code);
-    if ( res.code->Get() != 0 )
-        op_result->Assign(1, res.err_str);
-    else
-        op_result->Assign(2, res.value);
-
-    trigger->Cache(assoc, op_result);
     trigger->Release();
-
-    Unref(op_result);
 }
 
 OpenResultCallback::OpenResultCallback(IntrusivePtr<detail::BackendHandleVal> backend)
@@ -73,27 +61,19 @@ void OpenResultCallback::Complete(OperationResult res) {
         backend->backend->EnqueueBackendOpened();
     }
 
-    // If this is a sync callback, there isn't a trigger to process. Store the result and bail. Always
+    // Set the result's value to the backend so that it ends up in the result getting either
-    // set result's value to the backend pointer so that it comes across in the result. This ensures
+    // passed back to the trigger or the one stored for sync backends.
-    // the handle is always available in the result even on failures.
+    res.value = backend;
+
+    // If this is a sync callback, there isn't a trigger to process. Store the result and bail.
     if ( IsSyncCallback() ) {
         result = std::move(res);
-        result.value = backend;
         return;
     }
 
-    static auto op_result_type = zeek::id::find_type<zeek::RecordType>("Storage::OperationResult");
+    auto res_val = res.BuildVal();
-    auto* op_result = new zeek::RecordVal(op_result_type);
+    trigger->Cache(assoc, res_val.get());
-
-    op_result->Assign(0, res.code);
-    if ( res.code != ReturnCode::SUCCESS )
-        op_result->Assign(1, res.err_str);
-    op_result->Assign(2, backend);
-
-    trigger->Cache(assoc, op_result);
     trigger->Release();
-
-    Unref(op_result);
 }
 
 OperationResult Backend::Open(RecordValPtr options, TypePtr kt, TypePtr vt, OpenResultCallback* cb) {

src/storage/Backend.h

@@ -20,6 +20,7 @@ struct OperationResult {
     ValPtr value;
 
     RecordValPtr BuildVal();
+    static RecordValPtr MakeVal(EnumValPtr code, std::string_view err_str = "", ValPtr value = nullptr);
 };
 
 
@@ -38,7 +39,7 @@ public:
 protected:
     void CompleteWithVal(Val* result);
 
-    IntrusivePtr<zeek::detail::trigger::Trigger> trigger;
+    zeek::detail::trigger::TriggerPtr trigger;
     const void* assoc = nullptr;
 };
 

src/storage/storage.bif

@@ -96,24 +96,15 @@ function Storage::Async::__open_backend%(btype: Storage::Backend, options: any,
 
 function Storage::Async::__close_backend%(backend: opaque of Storage::BackendHandle%) : Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-	auto op_result = make_intrusive<RecordVal>(op_result_type);
-
 	auto trigger = init_trigger(frame);
 	if ( ! trigger )
 		return nullptr;
 
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
-	if ( ! b ) {
+	if ( ! b )
-		op_result->Assign(0, ReturnCode::OPERATION_FAILED);
+		return OperationResult::MakeVal(ReturnCode::OPERATION_FAILED, "Invalid storage handlle");
-		op_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
+	else if ( ! b->backend->IsOpen() )
-		return op_result;
+		return OperationResult::MakeVal(ReturnCode::NOT_CONNECTED, "Backend is closed");
-	}
-	else if ( ! b->backend->IsOpen() ) {
-		op_result->Assign(0, ReturnCode::NOT_CONNECTED);
-		op_result->Assign(1, make_intrusive<StringVal>("Backend is closed"));
-		return op_result;
-	}
 
 	auto cb = new OperationResultCallback(trigger, frame->GetTriggerAssoc());
 	auto close_res = storage_mgr->CloseBackend(b->backend, cb);
@@ -136,24 +127,15 @@ function Storage::Async::__close_backend%(backend: opaque of Storage::BackendHan
 function Storage::Async::__put%(backend: opaque of Storage::BackendHandle, key: any, value: any,
 			 overwrite: bool, expire_time: interval%): Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-	auto op_result = make_intrusive<RecordVal>(op_result_type);
-
 	auto trigger = init_trigger(frame);
 	if ( ! trigger )
 		return nullptr;
 
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
-	if ( ! b ) {
+	if ( ! b )
-		op_result->Assign(0, ReturnCode::OPERATION_FAILED);
+		return OperationResult::MakeVal(ReturnCode::OPERATION_FAILED, "Invalid storage handlle");
-		op_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
+	else if ( ! b->backend->IsOpen() )
-		return op_result;
+		return OperationResult::MakeVal(ReturnCode::NOT_CONNECTED, "Backend is closed");
-	}
-	else if ( ! b->backend->IsOpen() ) {
-		op_result->Assign(0, ReturnCode::NOT_CONNECTED);
-		op_result->Assign(1, make_intrusive<StringVal>("Backend is closed"));
-		return op_result;
-	}
 
 	if ( expire_time > 0.0 )
 		expire_time += run_state::network_time;
@@ -180,24 +162,15 @@ function Storage::Async::__put%(backend: opaque of Storage::BackendHandle, key:
 
 function Storage::Async::__get%(backend: opaque of Storage::BackendHandle, key: any%): Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-	auto op_result = make_intrusive<RecordVal>(op_result_type);
-
 	auto trigger = init_trigger(frame);
 	if ( ! trigger )
 		return nullptr;
 
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
-	if ( ! b ) {
+	if ( ! b )
-		op_result->Assign(0, ReturnCode::OPERATION_FAILED);
+		return OperationResult::MakeVal(ReturnCode::OPERATION_FAILED, "Invalid storage handlle");
-		op_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
+	else if ( ! b->backend->IsOpen() )
-		return op_result;
+		return OperationResult::MakeVal(ReturnCode::NOT_CONNECTED, "Backend is closed");
-	}
-	else if ( ! b->backend->IsOpen() ) {
-		op_result->Assign(0, ReturnCode::NOT_CONNECTED);
-		op_result->Assign(1, make_intrusive<StringVal>("Backend is closed"));
-		return op_result;
-	}
 
 	auto cb = new OperationResultCallback(trigger, frame->GetTriggerAssoc());
 	auto key_v = IntrusivePtr<Val>{NewRef{}, key};
@@ -220,24 +193,15 @@ function Storage::Async::__get%(backend: opaque of Storage::BackendHandle, key:
 
 function Storage::Async::__erase%(backend: opaque of Storage::BackendHandle, key: any%): Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-	auto op_result = make_intrusive<RecordVal>(op_result_type);
-
 	auto trigger = init_trigger(frame);
 	if ( ! trigger )
 		return nullptr;
 
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
-	if ( ! b ) {
+	if ( ! b )
-		op_result->Assign(0, ReturnCode::OPERATION_FAILED);
+		return OperationResult::MakeVal(ReturnCode::OPERATION_FAILED, "Invalid storage handlle");
-		op_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
+	else if ( ! b->backend->IsOpen() )
-		return op_result;
+		return OperationResult::MakeVal(ReturnCode::NOT_CONNECTED, "Backend is closed");
-	}
-	else if ( ! b->backend->IsOpen() ) {
-		op_result->Assign(0, ReturnCode::NOT_CONNECTED);
-		op_result->Assign(1, make_intrusive<StringVal>("Backend is closed"));
-		return op_result;
-	}
 
 	auto cb = new OperationResultCallback(trigger, frame->GetTriggerAssoc());
 	auto key_v = IntrusivePtr<Val>{NewRef{}, key};
@@ -262,8 +226,6 @@ module Storage::Sync;
 
 function Storage::Sync::__open_backend%(btype: Storage::Backend, options: any, key_type: any, val_type: any%): Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-
 	auto btype_val = IntrusivePtr<EnumVal>{NewRef{}, btype->AsEnumVal()};
 	Tag tag{btype_val};
 
@@ -296,21 +258,11 @@ function Storage::Sync::__open_backend%(btype: Storage::Backend, options: any, k
 
 function Storage::Sync::__close_backend%(backend: opaque of Storage::BackendHandle%) : Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
-	if ( ! b ) {
+	if ( ! b )
-		auto op_result = make_intrusive<RecordVal>(op_result_type);
+		return OperationResult::MakeVal(ReturnCode::OPERATION_FAILED, "Invalid storage handlle");
-		op_result->Assign(0, ReturnCode::OPERATION_FAILED);
+	else if ( ! b->backend->IsOpen() )
-		op_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
+		return OperationResult::MakeVal(ReturnCode::NOT_CONNECTED, "Backend is closed");
-		return op_result;
-	}
-	else if ( ! b->backend->IsOpen() ) {
-		auto op_result = make_intrusive<RecordVal>(op_result_type);
-		op_result->Assign(0, ReturnCode::NOT_CONNECTED);
-		op_result->Assign(1, make_intrusive<StringVal>("Backend is closed"));
-		return op_result;
-	}
 
 	auto cb = new OperationResultCallback();
 	auto close_res = storage_mgr->CloseBackend(b->backend, cb);
@@ -330,21 +282,11 @@ function Storage::Sync::__close_backend%(backend: opaque of Storage::BackendHand
 function Storage::Sync::__put%(backend: opaque of Storage::BackendHandle, key: any, value: any,
 			 overwrite: bool, expire_time: interval%): Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
-	if ( ! b ) {
+	if ( ! b )
-		auto op_result = make_intrusive<RecordVal>(op_result_type);
+		return OperationResult::MakeVal(ReturnCode::OPERATION_FAILED, "Invalid storage handlle");
-		op_result->Assign(0, ReturnCode::OPERATION_FAILED);
+	else if ( ! b->backend->IsOpen() )
-		op_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
+		return OperationResult::MakeVal(ReturnCode::NOT_CONNECTED, "Backend is closed");
-		return op_result;
-	}
-	else if ( ! b->backend->IsOpen() ) {
-		auto op_result = make_intrusive<RecordVal>(op_result_type);
-		op_result->Assign(0, ReturnCode::NOT_CONNECTED);
-		op_result->Assign(1, make_intrusive<StringVal>("Backend is closed"));
-		return op_result;
-	}
 
 	if ( expire_time > 0.0 )
 		expire_time += run_state::network_time;
@@ -368,21 +310,11 @@ function Storage::Sync::__put%(backend: opaque of Storage::BackendHandle, key: a
 
 function Storage::Sync::__get%(backend: opaque of Storage::BackendHandle, key: any%): Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
-	if ( ! b ) {
+	if ( ! b )
-		auto op_result = make_intrusive<RecordVal>(op_result_type);
+		return OperationResult::MakeVal(ReturnCode::OPERATION_FAILED, "Invalid storage handlle");
-		op_result->Assign(0, ReturnCode::OPERATION_FAILED);
+	else if ( ! b->backend->IsOpen() )
-		op_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
+		return OperationResult::MakeVal(ReturnCode::NOT_CONNECTED, "Backend is closed");
-		return op_result;
-	}
-	else if ( ! b->backend->IsOpen() ) {
-		auto op_result = make_intrusive<RecordVal>(op_result_type);
-		op_result->Assign(0, ReturnCode::NOT_CONNECTED);
-		op_result->Assign(1, make_intrusive<StringVal>("Backend is closed"));
-		return op_result;
-	}
 
 	auto key_v = IntrusivePtr<Val>{NewRef{}, key};
 	auto cb = new OperationResultCallback();
@@ -402,21 +334,11 @@ function Storage::Sync::__get%(backend: opaque of Storage::BackendHandle, key: a
 
 function Storage::Sync::__erase%(backend: opaque of Storage::BackendHandle, key: any%): Storage::OperationResult
 	%{
-	static auto op_result_type = id::find_type<RecordType>("Storage::OperationResult");
-
 	auto b = dynamic_cast<storage::detail::BackendHandleVal*>(backend);
-	if ( ! b ) {
+	if ( ! b )
-		auto op_result = make_intrusive<RecordVal>(op_result_type);
+		return OperationResult::MakeVal(ReturnCode::OPERATION_FAILED, "Invalid storage handlle");
-		op_result->Assign(0, ReturnCode::OPERATION_FAILED);
+	else if ( ! b->backend->IsOpen() )
-		op_result->Assign(1, make_intrusive<StringVal>("Invalid storage handlle"));
+		return OperationResult::MakeVal(ReturnCode::NOT_CONNECTED, "Backend is closed");
-		return op_result;
-	}
-	else if ( ! b->backend->IsOpen() ) {
-		auto op_result = make_intrusive<RecordVal>(op_result_type);
-		op_result->Assign(0, ReturnCode::NOT_CONNECTED);
-		op_result->Assign(1, make_intrusive<StringVal>("Backend is closed"));
-		return op_result;
-	}
 
 	auto cb = new OperationResultCallback();
 	auto key_v = IntrusivePtr<Val>{NewRef{}, key};