Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Logging improvements #94

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

bavshin-f5
Copy link
Collaborator

WIP, because I want to set up CI tests of the example modules before merging any significant changes.


Log macros now accept any object that implements LogTarget, i.e. requests, events, ngx_conf_t, etc. Default mask for the ngx_log_debug! is set according to the object type.
With all the complexity added, the code still makes 1-2 allocations less than the original variant and gets completely inlined in the release builds.

A few tricky moments:

  • LogTarget trait is intentionally kept object-safe, because one of the planned further steps is to offer integration with the log facade and that required type erasure via dyn LogTarget (to be reevaluated).
  • Log methods take pointers to make things work uniformly for pointer and reference arguments. I wish I knew a better way to handle that.
  • Log methods take const ptr to avoid creating a mutable borrow (or failing to compile because the target was already borrowed immutably). It may look suspicious, but in the end we have a pointer to ngx_log_t that comes from C, points to a writable memory, and doesn't have to satisfy the Rust memory model constraints (as described in the std::ptr::from_ref).

Fixes #47
Fixes #92

A new `LogTarget` trait can be implemented for any object that owns a
logger or requires a special log behavior.

The logger implementation can do a mut cast on the `ngx_log_t` pointer
to avoid borrowing the target mutably.  This looks highly suspicious,
but is a defined behavior and mostly safe.
@bavshin-f5
Copy link
Collaborator Author

After some research, I arrived to the following conclusions:

  1. The following code is sound because log: *mut ngx_log_t has interior mutability, similar to UnsafeCell:
    impl LogTarget for ngx_event_t {
        fn get_log(&self) -> *mut ngx_log_t {
            self.log
        }
    }
  2. The behavior of the following code is undefined, because *mut ngx_log_t obtained from a shared reference could be mutated in the ngx_log_error_core. Similar issue exists with impl LogTarget for ngx_conf_t {} and ngx_conf_log_error.
    impl LogTarget for ngx_log_t {
        fn get_log(&self) -> *mut ngx_log_t {
            ...
        }
    }
    Quoting the language reference,

    the bytes pointed to by a shared reference, including transitively through other references (both shared and mutable) and Boxes, are immutable

  3. All the cases of &something as *const _ as *mut _ we have in the code are potentially unsound and should be reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in Error Handling in upstream.rs in examples Provide more logging macros
1 participant