fix review comments

Signed-off-by: Marin Veršić <marin.versic101@gmail.com>
hyperledger-iroha · Jun 7, 2022 · d35ecfc · d35ecfc
1 parent c28a1fb
commit d35ecfc
Show file tree

Hide file tree

Showing 3 changed files with 113 additions and 78 deletions.
diff --git a/data_model/src/lib.rs b/data_model/src/lib.rs
@@ -170,21 +170,30 @@ pub unsafe extern "C" fn Name__from_str(
     candidate_len: usize,
     output: *mut *mut Name,
 ) -> iroha_ffi::FfiResult {
-    let candidate = core::slice::from_raw_parts(candidate, candidate_len);
+    let res = std::panic::catch_unwind(|| {
+        let candidate = core::slice::from_raw_parts(candidate, candidate_len);
 
-    let method_res = match core::str::from_utf8(candidate) {
-        // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
-        Err(_error) => return iroha_ffi::FfiResult::Utf8Error,
-        Ok(candidate) => Name::from_str(candidate),
-    };
-    let method_res = match method_res {
-        Err(_error) => return iroha_ffi::FfiResult::ExecutionFail,
-        Ok(method_res) => method_res,
-    };
-    let method_res = Box::into_raw(Box::new(method_res));
-
-    output.write(method_res);
-    iroha_ffi::FfiResult::Ok
+        let method_res = match core::str::from_utf8(candidate) {
+            // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
+            Err(_error) => return iroha_ffi::FfiResult::Utf8Error,
+            Ok(candidate) => Name::from_str(candidate),
+        };
+        let method_res = Box::into_raw(Box::new(match method_res {
+            Err(_error) => return iroha_ffi::FfiResult::ExecutionFail,
+            Ok(method_res) => method_res,
+        }));
+
+        output.write(method_res);
+        iroha_ffi::FfiResult::Ok
+    });
+
+    match res {
+        Ok(res) => res,
+        Err(_) => {
+            // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
+            iroha_ffi::FfiResult::UnrecoverableError
+        }
+    }
 }
 
 impl Debug for Name {

diff --git a/ffi/derive/src/arg.rs b/ffi/derive/src/arg.rs
@@ -1,3 +1,5 @@
+//! Logic related to FFI function argument. Visitor implementation visits the given type
+//! and collects the FFI conversion information into the [`Arg`] struct
 #![allow(clippy::unimplemented)]
 
 use proc_macro2::Span;
@@ -99,7 +101,6 @@ impl Arg {
     }
 }
 
-/// Struct representing a method/function argument
 struct TypeVisitor<'ast> {
     self_ty: &'ast Path,
     name: &'ast Ident,
@@ -120,6 +121,7 @@ impl<'ast> TypeVisitor<'ast> {
             ffi_to_src: None,
         }
     }
+
     fn visit_item_binding(&mut self, seg: &'ast syn::PathSegment) {
         let bindings = generic_arg_bindings(seg);
 
@@ -352,13 +354,18 @@ impl<'ast> Visit<'ast> for TypeVisitor<'ast> {
                         }
                     };
                 });
-                self.ffi_to_src = Some(parse_quote! { #ok_ffi_to_src });
+                self.ffi_to_src = Some(parse_quote! {
+                    let #arg_name = {
+                        #ok_ffi_to_src
+                        Ok(#arg_name)
+                    };
+                });
             }
             _ => {
                 if self.is_input {
                     self.ffi_type = Some(parse_quote! { *const #node });
                     self.src_to_ffi = Some(parse_quote! {
-                        let #arg_name: *const _ = #arg_name;
+                        let #arg_name: *const _ = &#arg_name;
                     });
                     self.ffi_to_src = Some(parse_quote! {
                         let #arg_name = Clone::clone(&*#arg_name);
@@ -459,16 +466,18 @@ impl<'ast> Visit<'ast> for TypeVisitor<'ast> {
 
 pub fn generic_arg_types(seg: &syn::PathSegment) -> Vec<&Type> {
     if let AngleBracketed(arguments) = &seg.arguments {
-        let mut args = vec![];
-
-        for arg in &arguments.args {
-            if let syn::GenericArgument::Type(ty) = &arg {
-                args.push(ty);
-            }
-        }
-
-        return args;
-    };
+        return arguments
+            .args
+            .iter()
+            .filter_map(|arg| {
+                if let syn::GenericArgument::Type(ty) = &arg {
+                    Some(ty)
+                } else {
+                    None
+                }
+            })
+            .collect();
+    }
 
     abort!(seg, "Type not found in the given path segment")
 }

diff --git a/ffi/src/lib.rs b/ffi/src/lib.rs
@@ -75,6 +75,15 @@ macro_rules! gen_ffi_impl {
             return $crate::FfiResult::ArgIsNull;
         } )+
     };
+    (@catch_unwind $block:block ) => {
+        match std::panic::catch_unwind(|| $block) {
+            Ok(res) => res,
+            Err(_) => {
+                // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
+                $crate::FfiResult::UnrecoverableError
+            },
+        }
+    };
     ( Clone: $( $other:ty ),+ $(,)? ) => {
         /// FFI function equivalent of [`Clone::clone`]
         ///
@@ -88,22 +97,24 @@ macro_rules! gen_ffi_impl {
             handle_ptr: *const core::ffi::c_void,
             output_ptr: *mut *mut core::ffi::c_void
         ) -> $crate::FfiResult {
-            gen_ffi_impl!{@null_check_stmts handle_ptr, output_ptr}
+            gen_ffi_impl!(@catch_unwind {
+                gen_ffi_impl!{@null_check_stmts handle_ptr, output_ptr}
 
-            match handle_id {
-                $( <$other as Handle>::ID => {
-                    let handle = &*handle_ptr.cast::<$other>();
+                match handle_id {
+                    $( <$other as Handle>::ID => {
+                        let handle = &*handle_ptr.cast::<$other>();
 
-                    let new_handle = Box::new(Clone::clone(handle));
-                    let new_handle = Box::into_raw(new_handle);
+                        let new_handle = Box::new(Clone::clone(handle));
+                        let new_handle = Box::into_raw(new_handle);
 
-                    output_ptr.write(new_handle.cast());
-                } )+
-                // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
-                _ => return $crate::FfiResult::UnknownHandle,
-            }
+                        output_ptr.write(new_handle.cast());
+                    } )+
+                    // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
+                    _ => return $crate::FfiResult::UnknownHandle,
+                }
 
-            $crate::FfiResult::Ok
+                $crate::FfiResult::Ok
+            })
         }
     };
     ( Eq: $( $other:ty ),+ $(,)? ) => {
@@ -120,20 +131,22 @@ macro_rules! gen_ffi_impl {
             right_handle_ptr: *const core::ffi::c_void,
             output_ptr: *mut bool,
         ) -> $crate::FfiResult {
-            gen_ffi_impl!{@null_check_stmts left_handle_ptr, right_handle_ptr, output_ptr}
-
-            match handle_id {
-                $( <$other as Handle>::ID => {
-                    let left_handle = &*left_handle_ptr.cast::<$other>();
-                    let right_handle = &*right_handle_ptr.cast::<$other>();
-
-                    output_ptr.write(left_handle == right_handle);
-                } )+
-                // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
-                _ => return $crate::FfiResult::UnknownHandle,
-            }
-
-            $crate::FfiResult::Ok
+            gen_ffi_impl!(@catch_unwind {
+                gen_ffi_impl!{@null_check_stmts left_handle_ptr, right_handle_ptr, output_ptr}
+
+                match handle_id {
+                    $( <$other as Handle>::ID => {
+                        let left_handle = &*left_handle_ptr.cast::<$other>();
+                        let right_handle = &*right_handle_ptr.cast::<$other>();
+
+                        output_ptr.write(left_handle == right_handle);
+                    } )+
+                    // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
+                    _ => return $crate::FfiResult::UnknownHandle,
+                }
+
+                $crate::FfiResult::Ok
+            })
         }
     };
     ( Ord: $( $other:ty ),+ $(,)? ) => {
@@ -150,20 +163,22 @@ macro_rules! gen_ffi_impl {
             right_handle_ptr: *const core::ffi::c_void,
             output_ptr: *mut core::cmp::Ordering,
         ) -> $crate::FfiResult {
-            gen_ffi_impl!{@null_check_stmts left_handle_ptr, right_handle_ptr, output_ptr}
-
-            match handle_id {
-                $( <$other as Handle>::ID => {
-                    let left_handle = &*left_handle_ptr.cast::<$other>();
-                    let right_handle = &*right_handle_ptr.cast::<$other>();
-
-                    output_ptr.write(left_handle.cmp(right_handle));
-                } )+
-                // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
-                _ => return $crate::FfiResult::UnknownHandle,
-            }
-
-            $crate::FfiResult::Ok
+            gen_ffi_impl!(@catch_unwind {
+                gen_ffi_impl!{@null_check_stmts left_handle_ptr, right_handle_ptr, output_ptr}
+
+                match handle_id {
+                    $( <$other as Handle>::ID => {
+                        let left_handle = &*left_handle_ptr.cast::<$other>();
+                        let right_handle = &*right_handle_ptr.cast::<$other>();
+
+                        output_ptr.write(left_handle.cmp(right_handle));
+                    } )+
+                    // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
+                    _ => return $crate::FfiResult::UnknownHandle,
+                }
+
+                $crate::FfiResult::Ok
+            })
         }
     };
     ( Drop: $( $other:ty ),+ $(,)? ) => {
@@ -178,17 +193,19 @@ macro_rules! gen_ffi_impl {
             handle_id: $crate::HandleId,
             handle_ptr: *mut core::ffi::c_void,
         ) -> $crate::FfiResult {
-            gen_ffi_impl!{@null_check_stmts handle_ptr}
-
-            match handle_id {
-                $( <$other as Handle>::ID => {
-                    Box::from_raw(handle_ptr.cast::<$other>());
-                } )+
-                // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
-                _ => return $crate::FfiResult::UnknownHandle,
-            }
-
-            $crate::FfiResult::Ok
+            gen_ffi_impl!(@catch_unwind {
+                gen_ffi_impl!{@null_check_stmts handle_ptr}
+
+                match handle_id {
+                    $( <$other as Handle>::ID => {
+                        Box::from_raw(handle_ptr.cast::<$other>());
+                    } )+
+                    // TODO: Implement error handling (https://github.com/hyperledger/iroha/issues/2252)
+                    _ => return $crate::FfiResult::UnknownHandle,
+                }
+
+                $crate::FfiResult::Ok
+            })
         }
     };
 }