Skip to content

Safe accessors for the module configuration #105

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.

Sign up for GitHub

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

Jump to bottom
Closed
bavshin-f5 opened this issue Dec 24, 2024 · 2 comments · Fixed by #142
Closed

Safe accessors for the module configuration #105

bavshin-f5 opened this issue Dec 24, 2024 · 2 comments · Fixed by #142
Labels
enhancement

Comments

@bavshin-f5
Copy link
Member

There's a certain unsafe pattern that is repeated in most of the examples:

let co = unsafe { request.get_module_loc_conf::<ModuleConfig>(&*addr_of!(ngx_http_curl_module)) };
let co = co.expect("module config is none");

The problems here are:

  • ngx_http_curl_module is a global mutable static, requiring unsafe for any access.
  • Nothing guarantees that the type requested matches an actual type allocated for the module config.

Something we can optimize here is to tie config type to a context type and to a module ptr, and hide most of the unsafe details from end user.

E.g.

impl HttpLocationConf for ngx_http_core_module_t {
    type ConfType = ngx_http_core_loc_conf_t;

    fn module() -> &'static ngx_module_t { unsafe { &*addr_of!(ngx_http_core_module) }  }
}

fn get_loc_conf<T: HttpLocationConf>(req) -> <T as HttpLocationConf>::ConfType;

req.get_loc_conf::<ngx_http_core_module_t>().ok_or(...)?;

or with an opposite direction of the mapping

impl HttpModuleConf for ngx_http_core_loc_conf_t {
    const CONTEXT: &HttpModuleConfType = HttpModuleConfType::Location;

    fn module() -> &'static ngx_module_t { unsafe { &*addr_of!(ngx_http_core_module) }  }
}

fn get_conf<T: HttpModuleConf>(req) -> &T;

req.get_conf::<ngx_http_core_loc_conf_t>().ok_or(...)?;

Extending the ngx::http::module::HTTPModule trait is also a viable option.

An important thing to consider is that the module configs can be obtained from ngx_cycle_t, ngx_conf_t, ngx_http_request_t or ngx_http_upstream_t objects and all of those should provide safe accessors.
@ensh63
Copy link
Contributor

ensh63 commented Feb 27, 2025

Possible variation of this interface with functions bound to module itself and using request, event, etc as a parameter.

All names are just examples and may be changed as desired.

/// Trait to be implemented for request, upstream, event, and conf_t
trait HttpMainConf {
    fn get_main_conf<T>(&self) -> Option<&T>;
}

/// Trait with config types and data
/// Should be implemented for each module including standard ones
trait ModuleConf {
    type MainConf;
    /// . . .
    fn module() -> &'static ngx_module_t;
}

/// Trait for main config
/// Similar traits should be defined for server and location configs
/// Each module may implement only necessary traits
trait GetModuleConf<M> {
    fn get_main_conf(o: & impl HttpMainConf) -> Option<&M::MainConf>;
}

/// usage with request
let r: &ngx_http_request_t;

let mcf = Module::get_main_conf(r);

@xeioex
Copy link
Contributor

xeioex commented Mar 4, 2025

I do not have strong opinion regarding where to implement traits, on module vs an object.

If there are no compelling arguments for module variant. I would prefer the initial proposal (impl HttpLocationConf for ngx_http_core_module_t).

@ensh63 ensh63 mentioned this issue Mar 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Safer interface for module configuration access. ensh63/ngx-rust
3 participants
@xeioex @ensh63 @bavshin-f5