Add logging support #107
Add logging support #107
Conversation
This uses name mangling to ensure we get our own isolated copy of the `log` crate, so we are free to set our own logger. The technique is described at https://github.com/jsha/rust-private-loggers. That logger will potentially be set each time rustls_connection_set_log_callback is called. Every time after the first, we will fail to register a log (but will ignore the error because we already registered one, and our log instances are all identical). Our log implementation uses the same thread-local userdata values we use for callbacks that implement customizable traits in rustls. The thread-local storage now includes a log_callback parameter as well as a userdata parameter.
src/log.rs
Outdated
| } | ||
| fn log(&self, record: &log::Record<'_>) { | ||
| if let Ok((Some(cb), userdata)) = log_callback_get() { | ||
| let message = format!("{} {} {}", record.target(), record.level(), record.args()); |
There was a problem hiding this comment.
Passing level separately as a defined int/enum is definitely useful to the application.
| pub static USERDATA: RefCell<Vec<Userdata>> = RefCell::new(Vec::new()); | ||
| } | ||
|
|
||
| pub struct Userdata { |
There was a problem hiding this comment.
One might consider simply storing the rustls_connection here. It seems to contain the application context which is exactly what we want to push/pop with this.
If my Rust were better, I'd also knew if a Arc<dyn AppContextTrait> could be used here.
There was a problem hiding this comment.
I considered this, but this would require us to violate the borrowing rules for Connection in ways we don't already. Specifically in the case where we are in a call that takes a *mut rustls_connection and turns it into a &mut Connection, we would then have to put a *mut Connection in Userdata and use unsafe to dereference it. I think this winds up being sound due to the threading properties of rustls (everything happens on the thread you called on), but it still seems like unnecessary risk.
Of course we are taking the same risk (having multiple mutable pointers) with userdata: *mut c_void, but that is unavoidable risk, if we want a general-purpose callback.
src/connection.rs
Outdated
| @@ -114,6 +118,16 @@ pub extern "C" fn rustls_connection_set_userdata( | |||
| conn.userdata = userdata; | |||
| } | |||
|
|
|||
| #[no_mangle] | |||
| pub extern "C" fn rustls_client_connection_set_log_callback( | |||
There was a problem hiding this comment.
Should this be rather rustls_connection_set_log_callback?
| struct rustls_str message; | ||
| } rustls_log_params; | ||
|
|
||
| typedef void (*rustls_log_callback)(void *userdata, const struct rustls_log_params *params); |
There was a problem hiding this comment.
Wrapping rustls_str in another struct seems like overkill to me. Is the idea here that it would be easier to add members to rustls_log_params than it would be to add arguments to rustls_log_callback? Also I think this merits documentation to explain which userdata is provided to the callback (since rustls_client_connection_set_log_callback doesn't take opaque userdata) (though discussion of doccomments is probably premature for a draft PR!)
There was a problem hiding this comment.
Yeah, the idea is to add other arguments, like log level. This follows a pattern for our other callbacks where they take a pointer to a struct rather than a list of params. One could argue is that that pattern is useful for large lists of params, but with small lists (like level and message), it's better to just pass the params directly. The flip side is that it's more consistent for our callbacks to always take a struct.
|
One other open question: the So what I'm asking is: How important is it to design the API so that, for instance, you can get I guess one answer is: The way we would do this is have a Also there are these compile time filters to entirely turn off logs below a certain level so they don't even get compiled into the binary: https://docs.rs/log/0.4.14/log/#compile-time-filters. Should we expose these? |
|
I vote for
|
+1 from me as well. |
This uses name mangling to ensure we get our own isolated copy of the
logcrate, so we are free to set our own logger. The technique is described at https://github.com/jsha/rust-private-loggers.That logger will be set each time rustls_connection_set_log_callback is called. Every time after the first, we will fail to register a log (but will ignore the error because we already registered one, and our log instances are all identical).
Our log implementation uses the same thread-local userdata values we use for callbacks that implement customizable traits in rustls. The thread-local storage now includes a log_callback parameter as well as a userdata parameter.
Still a draft because:
Levelenum fromlogso users can dispatch logs to different places by level.