Addressed reviewer and tool comments.

Signed-off-by: Holger Rapp <HolgerRapp@gmx.net>

everestrs/CMakeLists.txt

@@ -24,6 +24,8 @@ target_include_directories(everestrs_sys PRIVATE
   ${CMAKE_CURRENT_BINARY_DIR}/cxxbridge
 )
 
+# This is a requirement that linking works on systems enforcing PIE.
+set_property(TARGET everestrs_sys PROPERTY POSITION_INDEPENDENT_CODE ON)
 target_link_libraries(everestrs_sys
     PRIVATE
     everest::framework

everestrs/everestrs/README.md

@@ -2,29 +2,28 @@
 
 This is Rust support using cxx.rs to wrap the framework C++ library.
 
-# Trying it out
+## Trying it out
 
-- Install rust as outlined on <https://rustup.rs/>, which should just be this
-  one line: `curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh`
-- Built your workspace as outlined in `everest-core` README, make sure to tell
-  cMake to enable `EVEREST_ENABLE_RS_SUPPORT`. Note, that the Rust code relies
-  on being built in a workspace where `make install` was run once.
-- You can now try building the code, but it will not do anything: `cd everestrs
-  && cargo build --all`
-- If you want to play with a node, check out `https://github.com/EVerest/everest-core/pull/344` in your workspace and run make install.
-- Go to `everest-core/modules/RsSomkeTest` and run `cargo build` there.
-- There is no support for building or installing Rust modules with cMake
-  currently, so let's fake the installation:
-- Go to `everest-core/build/dist/libexec/everest/modules` and create the stuff needed
-for a module:
-    ~~~bash
-    mkdir RsSmokeTest
-    ln -s ../../../../../../modules/RsSmokeTest/target/debug/smoke_test RsSmokeTest
-    ln -s ../../../../../../modules/RsSmokeTest/manifest.yaml . 
-    ~~~
-- You should now be able to configure the `RsSmokeTest` module in your config
-  YAML.
+    - Install rust as outlined on <https://rustup.rs/>, which should just be this
+      one line: `curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh`
+    - Built your workspace as outlined in `everest-core` README, make sure to tell
+      cMake to enable `EVEREST_ENABLE_RS_SUPPORT`. Note, that the Rust code relies
+      on being built in a workspace where `make install` was run once.
+    - You can now try building the code, but it will not do anything: `cd everestrs
+      && cargo build --all`
+    - If you want to play with a node, check out `https://github.com/EVerest/everest-core/pull/344` in your workspace and run make install.
+    - Go to `everest-core/modules/RsSomkeTest` and run `cargo build` there.
+    - There is no support for building or installing Rust modules with cMake
+      currently, so let's fake the installation:
+    - Go to `everest-core/build/dist/libexec/everest/modules` and create the stuff needed for a module:
+        ~~~bash
+        mkdir RsSmokeTest
+        ln -s ../../../../../../modules/RsSmokeTest/target/debug/smoke_test RsSmokeTest
+        ln -s ../../../../../../modules/RsSmokeTest/manifest.yaml . 
+        ~~~
+    - You should now be able to configure the `RsSmokeTest` module in your config
+      YAML.
 
-# Status
+## Status
 
 This code is currently only supporting providing an interface to be implemented, i.e. no variables publish or receiving and no calling of other interfaces. Those features are straightforward, quick and easy to implement, but for now this is probably enough to iron out the integration questions.

everestrs/everestrs/build.rs

@@ -23,7 +23,7 @@ fn find_everest_workspace_root() -> PathBuf {
 
 /// Returns the Libraries path if this is a standalone build of everest-framework or None if it is
 /// not.
-fn find_libs_in_qwello_framework(root: &Path) -> Option<Libraries> {
+fn find_libs_in_everest_framework(root: &Path) -> Option<Libraries> {
     let everestrs_sys = root.join("everest-framework/build/everestrs/libeverestrs_sys.a");
     let framework = root.join("everest-framework/build/lib/libframework.so");
     if everestrs_sys.exists() && framework.exists() {
@@ -38,7 +38,7 @@ fn find_libs_in_qwello_framework(root: &Path) -> Option<Libraries> {
 
 /// Returns the Libraries path if this is an EVerest workspace where make install was run in
 /// everest-core/build or None if not.
-fn find_libs_in_qwello_core_build_dist(root: &Path) -> Option<Libraries> {
+fn find_libs_in_everest_core_build_dist(root: &Path) -> Option<Libraries> {
     let everestrs_sys = root.join("everest-core/build/dist/lib/libeverestrs_sys.a");
     let framework = root.join("everest-core/build/dist/lib/libframework.so");
     if everestrs_sys.exists() && framework.exists() {
@@ -54,13 +54,12 @@ fn find_libs_in_qwello_core_build_dist(root: &Path) -> Option<Libraries> {
 /// Takes a path to a library like `libframework.so` and returns the name for the linker, aka
 /// `framework`
 fn libname_from_path(p: &Path) -> String {
-    let mut name = p
-        .file_stem()
+    p.file_stem()
         .and_then(|os_str| os_str.to_str())
-        .unwrap_or("")
-        .to_string();
-    name.drain(..3); // remove 'lib' prefix.
-    name
+        .expect("'p' must be valid UTF-8 and have a .so extension.")
+        .strip_prefix("lib")
+        .expect("'p' should start with `lib`")
+        .to_string()
 }
 
 fn print_link_options(p: &Path) {
@@ -72,11 +71,11 @@ fn print_link_options(p: &Path) {
 }
 
 fn find_libs(root: &Path) -> Libraries {
-    let libs = find_libs_in_qwello_core_build_dist(&root);
+    let libs = find_libs_in_everest_core_build_dist(&root);
     if libs.is_some() {
         return libs.unwrap();
     }
-    find_libs_in_qwello_framework(&root)
+    find_libs_in_everest_framework(&root)
         .expect("everestrs is not build in a EVerest workspace that already ran cmake build")
 }
 

everestrs/everestrs/src/lib.rs

@@ -23,6 +23,9 @@ mod ffi {
     struct CommandMeta {
         implementation_id: String,
         name: String,
+        // cxx.rs does not expose a *const c_void type, so we use *const c_char everywhere.
+        // Wherever this shows it means "pointer to some random blob of memory that only Rust
+        // understand what it means and is of no concern to C".
         obj: *const c_char,
     }
 
@@ -47,6 +50,7 @@ mod ffi {
         /// `Everest::Module::signal_ready`.
         unsafe fn signal_ready(
             self: &Module,
+            // See comment in `CommandMeta::obj`.
             obj: *const c_char,
             on_ready: unsafe fn(obj: *const c_char) -> (),
         );
@@ -102,22 +106,26 @@ pub trait GenericModule: Sync {
     /// Handler for the command `name` on `implementation_id` with the given `parameters`. The return value
     /// will be returned as the result of the call.
     fn handle_command(
-        &mut self,
+        &self,
         implementation_id: &str,
         name: &str,
         parameters: HashMap<String, serde_json::Value>,
     ) -> Result<serde_json::Value>;
 
-    fn on_ready(&mut self) {}
+    fn on_ready(&self) {}
 }
 
 pub struct Runtime<T: GenericModule> {
-    // We are handing out pointers to `module_impl` to `cpp_module` for callbacks. The pointers must
-    // must stay valid for as long as `cpp_module` is alive. Hence `module_impl` must never move in
-    // memory. Rust can model this through the Pin concept which upholds this guarantee. We use a
-    // Box to put the object on the heap.
-    module_impl: Pin<Box<T>>,
+    // There are two subleties here:
+    // 1. We are handing out pointers to `module_impl` to `cpp_module` for callbacks. The pointers
+    //    must must stay valid for as long as `cpp_module` is alive. Hence `module_impl` must never
+    //    move in memory. Rust can model this through the Pin concept which upholds this guarantee.
+    //    We use a Box to put the object on the heap.
+    // 2. For the same reason, `module_impl` should outlive `cpp_module`, hence should be dropped
+    //    after it. Rust drops fields in declaration order, hence `cpp_module` should come before
+    //    `module_impl` in this struct.
     cpp_module: cxx::UniquePtr<ffi::Module>,
+    module_impl: Pin<Box<T>>,
 }
 
 impl<T: GenericModule> Runtime<T> {