Re: [tor-bugs] #23881 [Core Tor/Tor]: Implement a way to utilise tor's logging system from Rust code

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#23881: Implement a way to utilise tor's logging system from Rust code
------------------------------+------------------------------------
 Reporter:  isis              |          Owner:  chelseakomlo
     Type:  enhancement       |         Status:  needs_review
 Priority:  High              |      Milestone:  Tor: 0.3.3.x-final
Component:  Core Tor/Tor      |        Version:
 Severity:  Normal            |     Resolution:
 Keywords:  rust, rust-pilot  |  Actual Points:
Parent ID:                    |         Points:  3
 Reviewer:                    |        Sponsor:
------------------------------+------------------------------------

Comment (by manish.earth):

 This looks pretty good! I like how you've set up the testing code.

 Some review comments

 > `const int _LOG_WARN = LOG_WARN;`

 This is interesting. Usually I'd just mirror the constants and leave
 comments, and bindgen is useful for automirroring (but y'all aren't using
 bindgen and that's ok).

 I guess this works. I've never seen constants mirrored this way but I do
 feel it is more resilient to code changing.

 {{{#!rust
     /// Allow for default cases in case Rust and C log types get out of
 sync
     #[allow(unreachable_patterns)]
     pub unsafe fn translate_severity(severity: LogSeverity) -> c_int {
         match severity {
             LogSeverity::Warn => _LOG_WARN,
             LogSeverity::Notice => _LOG_NOTICE,
             _ => _LOG_NOTICE,
         }
     }
 }}}



 There's no need for that `unreachable_patterns`. The only way stuff will
 go out of sync is if `LogSeverity` gets a variant added, however that will
 trigger a compiler error in this match (if you leave off the `_` branch).
 It's nice to be able to modify enums and immediately have the compiler
 tell you where you need to update things


 {{{#!rust
             unsafe {
                 tor_log_string(translate_severity($severity),
                 translate_domain($domain),
                 func_ptr, msg_ptr
                 )
             }
 }}}

 These will fail to resolve if called outside of the correct module with
 the right imports.

 The best thing to do is to use `$crate::tor_log_string(..)` (see the docs
 on `$crate`, https://doc.rust-lang.org/1.7.0/book/macros.html#the-
 variable-crate) which will ensure these will resolve wherever you use
 them.


 > `const ERR_LOG_MSG: &'static str`

 Rust lets you omit the `'static` now (this is recent, so it works on the
 latest stable but may not work on the compiler you're using)

 -----

 One concern of mine is that the `tor_log_msg` macro contains unsafe code,
 and more problematically, it contains macro variables in that unsafe code.

 Now this is firmly in "why would anyone even _do_ that" territory, but for
 example someone could call `tor_log_msg!({some code}, ....)`, and that
 code would be expanded to be inside an unsafe block without the caller
 ever having to use unsafe.

 You should probably have severity/domain get bound to variables outside of
 the unsafe block.

 Ideally the entire macro body would be moved to a function, making the
 body just handle the `format!` stuff and calling the function. However,
 this means you can no longer do the testing trick where the test function
 itself returns what was printed.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23881#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs