Re: [PATCH] rust: add this_module macro

[Greg KH]

On Tue, Jan 31, 2023 at 01:07:28PM -0300, Martin Rodriguez Reboredo wrote:
> On Tue, Jan 31, 2023 at 04:15:51PM +0100, Greg KH wrote:
> >On Tue, Jan 31, 2023 at 12:07:45PM -0300, Martin Rodriguez Reboredo wrote:
> >> On Tue, Jan 31, 2023 at 02:42:08PM +0100, Greg KH wrote:
> >> >On Tue, Jan 31, 2023 at 10:08:41AM -0300, Martin Rodriguez Reboredo wrote:
> >> >> Adds a Rust equivalent to the handy THIS_MODULE macro from C.
> >> >> 
> >> >> Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@xxxxxxxxx>
> >> >> ---
> >> >>  rust/kernel/lib.rs | 12 ++++++++++++
> >> >>  1 file changed, 12 insertions(+)
> >> >> 
> >> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> >> >> index e0b0e953907d..afb6b0390426 100644
> >> >> --- a/rust/kernel/lib.rs
> >> >> +++ b/rust/kernel/lib.rs
> >> >> @@ -80,6 +80,18 @@ impl ThisModule {
> >> >>      }
> >> >>  }
> >> >>  
> >> >> +/// Returns the current module.
> >> >> +#[macro_export]
> >> >> +macro_rules! this_module {
> >> >> +    () => {
> >> >> +        if cfg!(MODULE) {
> >> >> +            Some(unsafe { $crate::ThisModule::from_ptr(&mut $crate::bindings::__this_module) })
> >> >> +        } else {
> >> >> +            None
> >> >> +        }
> >> >> +    };
> >> >> +}
> >> >
> >> >While this is handy, what exactly will it be used for?  The C
> >> >wrappers/shim/whatever should probably handle this for you already when
> >> >you save this pointer into a structure right?
> >> >
> >> >Surely you aren't trying to increment your own module's reference count,
> >> >right?  That just doesn't work :)
> >> >
> >> >thanks,
> >> >
> >> >greg k-h
> >> 
> >> This was meant for setting the owner field of a file_operations struct
> >> or the cra_owner field of crypto_alg and many other structs.
> >
> >But shouldn't the macro kernel::declare_file_operations() do this for
> >you automagically?  You should never have to manually say "this module!"
> >to any structure or function call if we do things right.
> >
> >Yes, many "old school" structures in the kernel do this, but we have
> >learned from the 1990's, see the fun wrappers around simple things like
> >usb_register_driver(); as an example of how the driver author themselves
> >should never see a module pointer anywhere.
> >
> >> I know that increfing a module without a good reason is dead dumb, so
> >> I'm not trying to send things in a downwards spiral. @@@
> >
> >That's good, but let's not add housekeeping requirements when we do not
> >have to do so if at all possible please.
> >
> >thanks,
> >
> >greg k-h
> 
> *kicks can*, at least I can take some ideas out of this, anyways, thanks
> for your reviews.

I don't mean to reject this out-of-hand, it's just that without a real
user, it's impossible to review this and say "this is ok" instead of
"perhaps you should do it this other way"?

Right now the rust framework is just that, a framework.  Perhaps we
should not be adding anything else to it until there is a real user of
it?  Otherwise this will keep coming up again and again.

Treat this like any other kernel feature/addition, you can't add apis
until you submit a user for it at the same time.  That's one way we have
been able to evolve and maintain the kernel source tree for so long.
Without an api user, we have no way to know how it's being used or if
it's even being used at all.

thanks,

greg k-h