Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Allow setting the panic hook with parity-clib #9292

Merged
merged 4 commits into from
Aug 10, 2018

Conversation

tomaka
Copy link
Contributor

@tomaka tomaka commented Aug 6, 2018

For example on Android, we want to present the panic message to the user with a dialog box, and put it in the logs (which are not the same as stderr).

This PR adds a parity_set_panic_hook function in the API of parity-as-a-library, so that we can choose what happens in case of a panic.

@tomaka tomaka added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 6, 2018
@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 6, 2018
@@ -162,3 +163,21 @@ pub extern fn parity_rpc(client: *mut c_void, query: *const char, len: usize, ou
}).unwrap_or(1)
}
}

#[no_mangle]
pub extern fn parity_set_panic_hook(callback: extern "C" fn(*mut c_void, *const c_char, usize), param: *mut c_void) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not be unsafe to invoke?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now I can call this from Rust code with crap data and it will register it as a thread-safe panic hook

@tomaka
Copy link
Contributor Author

tomaka commented Aug 6, 2018

I made all the other FFI functions unsafe as well.

/// Set the panic hook with a closure to be called. The closure receives the panic message.
///
/// Depending on how Parity was compiled, after the closure either returns the process aborts or
/// unwinding starts.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this, can you rephrase?

I guess it should be something like "after the closure has been executed it either returns the exit code when the process terminates or the unwinding starts"

impl Cb {
fn call(&self, new_chain: String) {
if let Some(ref cb) = self.0 {
cb(self.1, new_chain.as_bytes().as_ptr() as *const _, new_chain.len())
}
}
Copy link
Collaborator

@niklasad1 niklasad1 Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Duplicate struct definition", the only thing the differs is that this one is inside an Option and https://github.com/paritytech/parity-ethereum/blob/4ce97f9554d2353f225d8f47f615e85a019e0316/parity-clib/src/lib.rs#L159-#L167 is not!

Can you move the "local struct definitions" to one file scope definition (the Option variant) and rename it to Callback? I think the full name is worth the effort here!

Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, minor grumble on the Cb struct definition

@debris debris merged commit 1564fae into openethereum:master Aug 10, 2018
@tomaka tomaka deleted the panic-hook-clib branch August 10, 2018 10:05
ordian added a commit to ordian/parity that referenced this pull request Aug 10, 2018
…rp_sync_on_light_client

* 'master' of https://github.com/paritytech/parity:
  Allow setting the panic hook with parity-clib (openethereum#9292)
  Prevent blockchain & miner racing when accessing pending block. (openethereum#9310)
  Docker alpine: use multi-stage concept (openethereum#9269)
  Update `log` -> 0.4, `env_logger` -> 0.5. (openethereum#9294)
  Update tobalaba.json (openethereum#9313)
  Allow tx pool to be Send (openethereum#9315)
  Fix codecov.io badge in README (openethereum#9327)
  Move ethereum-specific H256FastMap type to own crate (openethereum#9307)
  ethcore sync decodes rlp less often (openethereum#9264)
@5chdn 5chdn added this to the 2.1 milestone Aug 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants