DemesneGH commented on code in PR #176:
URL:
https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/176#discussion_r2012064467
##########
optee-teec/macros/src/lib.rs:
##########
@@ -78,50 +111,64 @@ pub fn plugin_invoke(_args: TokenStream, input:
TokenStream) -> TokenStream {
let f = parse_macro_input!(input as syn::ItemFn);
let f_vis = &f.vis;
let f_block = &f.block;
- let f_decl = &f.decl;
- let f_inputs = &f_decl.inputs;
+ let f_sig = &f.sig;
+ let f_inputs = &f_sig.inputs;
+ let return_type = check_return_type(&f);
+ let valid_return_type = return_type == ReturnType::TeecEmptyResult;
// check the function signature
- let valid_signature = f.constness.is_none()
+ let valid_signature = f_sig.constness.is_none()
&& match f_vis {
syn::Visibility::Inherited => true,
_ => false,
}
- && f.abi.is_none()
+ && f_sig.abi.is_none()
&& f_inputs.len() == 1
- && f.decl.generics.where_clause.is_none()
- && f.decl.variadic.is_none();
+ && f_sig.generics.where_clause.is_none()
+ && f_sig.variadic.is_none()
+ && valid_return_type;
if !valid_signature {
return syn::parse::Error::new(
f.span(),
- "`#[plugin_invoke]` function must have signature `fn(params: &mut
PluginParamters)`",
+ concat!(
+ "`#[plugin_invoke]` function must have signature",
+ " `fn(params: &mut PluginParameters) ->
optee_teec::Result<()>`"
+ ),
)
.to_compile_error()
.into();
}
+ let params = f_inputs
+ .first()
+ .expect("we have already verified its len")
+ .into_token_stream();
+
quote!(
#[no_mangle]
pub fn _plugin_invoke(
cmd: u32,
sub_cmd: u32,
- data: *mut c_char,
+ data: *mut core::ffi::c_char,
in_len: u32,
out_len: *mut u32
- ) -> optee_teec::Result<()> {
+ ) -> u32 {
Review Comment:
u32 => raw::TEEC_Result?
##########
examples/supp_plugin-rs/ta/src/main.rs:
##########
@@ -54,19 +54,24 @@ fn invoke_command(cmd_id: u32, params: &mut Parameters) ->
Result<()> {
trace_println!("[+] TA invoke command");
let mut p0 = unsafe { params.0.as_memref().unwrap() };
let inbuf = p0.buffer().to_vec();
- trace_println!("[+] TA received value {:?} then send to plugin",
p0.buffer());
+ trace_println!(
+ "[+] TA received value {:?} then send to plugin",
+ p0.buffer()
+ );
let uuid = Uuid::parse_str(PLUGIN_UUID).unwrap();
match Command::from(cmd_id) {
Command::Ping => {
- let mut plugin = LoadablePlugin::new(&uuid);
- let outbuf = plugin.invoke(
- PluginCommand::Print as u32,
- PLUGIN_SUBCMD_NULL,
- &inbuf
- ).unwrap();
+ let plugin = LoadablePlugin::new(&uuid);
+ let outbuf = plugin
+ .invoke(PluginCommand::Print as u32, PLUGIN_SUBCMD_NULL,
&inbuf)
+ .unwrap();
Review Comment:
It's a legacy coding style issue that we use `unwrap()` in many examples.
Please help eliminate its usage here to prevent potential panics.
##########
optee-teec/macros/src/lib.rs:
##########
@@ -78,50 +111,64 @@ pub fn plugin_invoke(_args: TokenStream, input:
TokenStream) -> TokenStream {
let f = parse_macro_input!(input as syn::ItemFn);
let f_vis = &f.vis;
let f_block = &f.block;
- let f_decl = &f.decl;
- let f_inputs = &f_decl.inputs;
+ let f_sig = &f.sig;
+ let f_inputs = &f_sig.inputs;
+ let return_type = check_return_type(&f);
+ let valid_return_type = return_type == ReturnType::TeecEmptyResult;
Review Comment:
If we only accept the return type as TeecEmptyResult, why we need to define
the ReturnType::No and ReturnType::Other?
In line 129 we can just `&& check_return_type()` and
in check_return_type():
syn::ReturnType::Type(xxx) => checks in line92-95; true
_ => false
##########
optee-teec/macros/src/lib.rs:
##########
@@ -78,50 +111,64 @@ pub fn plugin_invoke(_args: TokenStream, input:
TokenStream) -> TokenStream {
let f = parse_macro_input!(input as syn::ItemFn);
let f_vis = &f.vis;
let f_block = &f.block;
- let f_decl = &f.decl;
- let f_inputs = &f_decl.inputs;
+ let f_sig = &f.sig;
+ let f_inputs = &f_sig.inputs;
+ let return_type = check_return_type(&f);
+ let valid_return_type = return_type == ReturnType::TeecEmptyResult;
// check the function signature
- let valid_signature = f.constness.is_none()
+ let valid_signature = f_sig.constness.is_none()
&& match f_vis {
syn::Visibility::Inherited => true,
_ => false,
}
- && f.abi.is_none()
+ && f_sig.abi.is_none()
&& f_inputs.len() == 1
- && f.decl.generics.where_clause.is_none()
- && f.decl.variadic.is_none();
+ && f_sig.generics.where_clause.is_none()
+ && f_sig.variadic.is_none()
+ && valid_return_type;
if !valid_signature {
return syn::parse::Error::new(
f.span(),
- "`#[plugin_invoke]` function must have signature `fn(params: &mut
PluginParamters)`",
+ concat!(
+ "`#[plugin_invoke]` function must have signature",
+ " `fn(params: &mut PluginParameters) ->
optee_teec::Result<()>`"
+ ),
)
.to_compile_error()
.into();
}
+ let params = f_inputs
+ .first()
+ .expect("we have already verified its len")
+ .into_token_stream();
+
quote!(
#[no_mangle]
pub fn _plugin_invoke(
cmd: u32,
sub_cmd: u32,
- data: *mut c_char,
+ data: *mut core::ffi::c_char,
in_len: u32,
out_len: *mut u32
- ) -> optee_teec::Result<()> {
+ ) -> u32 {
+ fn inner(#params) -> optee_teec::Result<()> {
+ #f_block
+ }
let mut inbuf = unsafe { std::slice::from_raw_parts_mut(data,
in_len as usize) };
let mut params = PluginParameters::new(cmd, sub_cmd, inbuf);
- #f_block
+ if let Err(err) = inner(&mut params) {
+ return err.raw_code();
+ };
let outslice = params.get_out_slice();
unsafe {
*out_len = outslice.len() as u32;
std::ptr::copy(outslice.as_ptr(), data, outslice.len());
- }
-
- Ok(())
+ };
+ return 0;
Review Comment:
0 => TEE_SUCCESS?
##########
optee-teec/macros/src/lib.rs:
##########
@@ -34,38 +34,71 @@ pub fn plugin_init(_args: TokenStream, input: TokenStream)
-> TokenStream {
let f = parse_macro_input!(input as syn::ItemFn);
let f_vis = &f.vis;
let f_block = &f.block;
- let f_decl = &f.decl;
- let f_inputs = &f_decl.inputs;
+ let f_sig = &f.sig;
+ let f_inputs = &f_sig.inputs;
+ let return_type = check_return_type(&f);
+ let valid_return_type = return_type == ReturnType::TeecEmptyResult;
// check the function signature
- let valid_signature = f.constness.is_none()
+ let valid_signature = f_sig.constness.is_none()
&& match f_vis {
syn::Visibility::Inherited => true,
_ => false,
}
- && f.abi.is_none()
+ && f_sig.abi.is_none()
&& f_inputs.len() == 0
- && f.decl.generics.where_clause.is_none()
- && f.decl.variadic.is_none();
+ && f_sig.generics.where_clause.is_none()
+ && f_sig.variadic.is_none()
+ && valid_return_type;
if !valid_signature {
return syn::parse::Error::new(
f.span(),
- "`#[plugin_init]` function must have signature `fn()`",
+ "`#[plugin_init]` function must have signature `fn() ->
optee_teec::Result<()>`",
)
.to_compile_error()
.into();
}
quote!(
#[no_mangle]
- pub fn _plugin_init() -> optee_teec::Result<()> {
- #f_block
- Ok(())
+ pub fn _plugin_init() -> u32 {
+ fn inner() -> optee_teec::Result<()> {
+ #f_block
+ }
+ match inner() {
+ Ok(()) => 0,
Review Comment:
Prefer `TEE_SUCCESS` instead of `0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@teaclave.apache.org
For additional commands, e-mail: dev-h...@teaclave.apache.org
