Invoke Testing extension through `ExtensionHandle` #2660

mariaschett · 2022-04-04T14:09:20Z

add ExtensionHandle and get_handle() for every extension
invoke testing extension with a handle

conradgrobler · 2022-04-04T15:57:50Z

oak_functions/loader/src/lookup.rs

@@ -109,7 +109,7 @@ where
        Ok(extension_result)
    }

-    fn get_metadata(&self) -> (String, wasmi::Signature) {
+    fn get_metadata(&self) -> (String, wasmi::Signature, ExtensionHandle) {


I think it might be worth defining a struct rather than using a tuple with 3 elements to improve clarity.

Thank you, that's true! Instead of adding a struct, I added a new get_handle function to the trait. I believe we'll remove get_metadata eventually, so that makes more sense.

conradgrobler · 2022-04-04T15:59:58Z

oak_functions/loader/src/server.rs

+        let handle: ExtensionHandle =
+            ExtensionHandle::from_i32(handle).expect("Fail to parse handle.");
+
+        // Quick solution following impelementation of invoking an extension in `invoke_index`.


I am unsure what the intention of this comment is. Does it mean this is a temporary solution that will be replaced with something better?

Yes, updated comment. Thank you!

conradgrobler · 2022-04-04T16:03:26Z

oak_functions/loader/src/server.rs

+        // Then we get the extension which has the given handle by looking at the values of the
+        // extension_indices.
+        let extension = extensions_indices
+            .iter_mut()


Why is a mutable iterator needed? It seems weird that this requires taking the extension indicices and then putting them back when nothing seems to be actually mutated.

Because extension.invoke(self, ...) in line 369 needs a mutable WasmState and extension_indices are part of WasmState.

With #2664 we can simplify this.

Ah, I see. I guess part of the weirdness it that an Option is used rather than a RefCell to implement interior mutability, but this is preexisting so can be fixed later as part of #2664 and other refactoring.

tiziano88 · 2022-04-04T16:08:53Z

oak_functions/loader/src/lookup.rs

@@ -109,7 +109,7 @@ where
        Ok(extension_result)
    }

-    fn get_metadata(&self) -> (String, wasmi::Signature) {
+    fn get_metadata(&self) -> (String, wasmi::Signature, ExtensionHandle) {


tiziano88 · 2022-04-04T16:13:47Z

oak_functions/loader/src/testing.rs

-        let request_len: AbiPointerOffset = args.nth_checked(1)?;
-        let response_ptr_ptr: AbiPointer = args.nth_checked(2)?;
-        let response_len_ptr: AbiPointer = args.nth_checked(3)?;
+        // We still read the args after the ExtensionHandle at position 0.


Maybe add let _handle = args.nth_checked(0); for consistency and future proof.

mariaschett

Thank you!

mariaschett · 2022-04-05T09:31:56Z

oak_functions/loader/src/lookup.rs

@@ -109,7 +109,7 @@ where
        Ok(extension_result)
    }

-    fn get_metadata(&self) -> (String, wasmi::Signature) {
+    fn get_metadata(&self) -> (String, wasmi::Signature, ExtensionHandle) {


Thank you, that's true! Instead of adding a struct, I added a new get_handle function to the trait. I believe we'll remove get_metadata eventually, so that makes more sense.

mariaschett · 2022-04-05T09:47:10Z

oak_functions/loader/src/server.rs

+        let handle: ExtensionHandle =
+            ExtensionHandle::from_i32(handle).expect("Fail to parse handle.");
+
+        // Quick solution following impelementation of invoking an extension in `invoke_index`.


Yes, updated comment. Thank you!

mariaschett · 2022-04-05T09:48:20Z

oak_functions/loader/src/server.rs

+        // Then we get the extension which has the given handle by looking at the values of the
+        // extension_indices.
+        let extension = extensions_indices
+            .iter_mut()


Because extension.invoke(self, ...) in line 369 needs a mutable WasmState and extension_indices are part of WasmState.

With #2664 we can simplify this.

mariaschett · 2022-04-05T09:48:27Z

oak_functions/loader/src/testing.rs

-        let request_len: AbiPointerOffset = args.nth_checked(1)?;
-        let response_ptr_ptr: AbiPointer = args.nth_checked(2)?;
-        let response_len_ptr: AbiPointer = args.nth_checked(3)?;
+        // We still read the args after the ExtensionHandle at position 0.


conradgrobler · 2022-04-05T10:05:27Z

oak_functions/loader/src/server.rs

+        let handle: ExtensionHandle =
+            ExtensionHandle::from_i32(handle).expect("Fail to parse handle.");
+
+        // TODO(#2664) Quick solution following impelementation of invoking an extension in


nit: I think a : is required after the TODO().

conradgrobler · 2022-04-05T10:06:44Z

oak_functions/loader/src/server.rs

+        // Then we get the extension which has the given handle by looking at the values of the
+        // extension_indices.
+        let extension = extensions_indices
+            .iter_mut()


Ah, I see. I guess part of the weirdness it that an Option is used rather than a RefCell to implement interior mutability, but this is preexisting so can be fixed later as part of #2664 and other refactoring.

github-actions · 2022-04-05T12:13:50Z

Reproducibility Index:

d4b7228fb9f2bdb5a85593815a44be6ac8ab9e745c8352869a8bd7e815b18a92  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
969ca18c94fbd6e9b852ede7819a893194228316b49d09dc315e02a35cb88d93  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe

Reproducibility Index diff:

diff --git a/reproducibility_index b/reproducibility_index
index f6358ab..a7663eb 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,2 +1,2 @@
-96fb21e31e8cf9b36cfa3a60d9755b30b0032f4e232e322341b9f731879c6471  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
-b9fd6e9df8799c1d2465bcfcaecb4c6e5df266e1488700135112f9080be138aa  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe
+d4b7228fb9f2bdb5a85593815a44be6ac8ab9e745c8352869a8bd7e815b18a92  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
+969ca18c94fbd6e9b852ede7819a893194228316b49d09dc315e02a35cb88d93  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe

mariaschett marked this pull request as ready for review April 4, 2022 15:45

mariaschett requested a review from tiziano88 April 4, 2022 15:45

tiziano88 requested review from a team, andrisaar and tiziano88 and removed request for a team and tiziano88 April 4, 2022 15:46

conradgrobler reviewed Apr 4, 2022

View reviewed changes

tiziano88 approved these changes Apr 4, 2022

View reviewed changes

mariaschett self-assigned this Apr 5, 2022

mariaschett commented Apr 5, 2022

View reviewed changes

mariaschett requested a review from conradgrobler April 5, 2022 10:03

conradgrobler approved these changes Apr 5, 2022

View reviewed changes

mariaschett added 4 commits April 5, 2022 10:25

Invoke testing extension through handle

40d3a2d

Fixes

183a1fa

Add get_handle in OakApiNativeExtension, update comments

ba079ba

Fix TODO

d294575

mariaschett force-pushed the add-handle-for-testing branch from 455299c to d294575 Compare April 5, 2022 10:26

Fix Wasm

143919b

andrisaar approved these changes Apr 5, 2022

View reviewed changes

mariaschett merged commit 20c4ca2 into project-oak:main Apr 5, 2022

mariaschett deleted the add-handle-for-testing branch April 5, 2022 11:56

mariaschett mentioned this pull request Apr 8, 2022

Refactor invoke in OakApiNativeExtension to not require WasmState #2664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invoke Testing extension through `ExtensionHandle` #2660

Invoke Testing extension through `ExtensionHandle` #2660

mariaschett commented Apr 4, 2022 •

edited

Loading

conradgrobler Apr 4, 2022

tiziano88 Apr 4, 2022

mariaschett Apr 5, 2022

conradgrobler Apr 4, 2022

mariaschett Apr 5, 2022

conradgrobler Apr 4, 2022

mariaschett Apr 5, 2022

conradgrobler Apr 5, 2022

tiziano88 Apr 4, 2022

tiziano88 Apr 4, 2022

mariaschett Apr 5, 2022

mariaschett left a comment

mariaschett Apr 5, 2022

mariaschett Apr 5, 2022

mariaschett Apr 5, 2022

mariaschett Apr 5, 2022

conradgrobler Apr 5, 2022

conradgrobler Apr 5, 2022

github-actions bot commented Apr 5, 2022

Invoke Testing extension through ExtensionHandle #2660

Invoke Testing extension through ExtensionHandle #2660

Conversation

mariaschett commented Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariaschett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2022

Invoke Testing extension through `ExtensionHandle` #2660

Invoke Testing extension through `ExtensionHandle` #2660

mariaschett commented Apr 4, 2022 •

edited

Loading