linux-kernel - Re: [PATCH v2 1/1] device: rust: change the name function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_RzfYXWJ1ePZk7-DFR4P--F1pyzWg8bnC40mbLWaHpx_aNJA@mail.gmail.com>
Date: Mon, 30 Sep 2024 10:52:27 -0300
From: Guilherme Giácomo Simões <trintaeoitogc@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org, 
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net, 
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, aliceryhl@...gle.com, 
	mcgrof@...nel.org, russ.weight@...ux.dev, dakr@...hat.com, 
	a.hindborg@...nel.org, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] device: rust: change the name function

Danilo Krummrich <dakr@...nel.org> writes:
>
> On Mon, Sep 30, 2024 at 09:39:56AM -0300, Guilherme Giacomo Simoes wrote:
> > This function increment the refcount by this command "bindings::get_device(prt)".
>
> 'increments', 'bindings::get_device(ptr)'
>
> I'd also say 'by a call to' instead of 'by this command'.
>
> > This can be confuse becuase, the function Arc::from_raw() from standard library, don't increment the refcount.
>
> Neither the stdlib one, nor the kernel one.
>
> > Then, this function "Device::from_raw()" will be renamed for don't make confusing in the future.
>
> Prefer the imperative, e.g. "Hence, rename `Device::from_raw` to avoid
> confusion with other `from_raw` semantics."
>
> >
> > Signed-off-by: Guilherme Giacomo Simoes <trintaeoitogc@...il.com>
>
> Please make sure to run ./scripts/checkpatch.pl before submitting patches.
>
> I see the following two warnings:
>
> WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
> #6:
> This function increment the refcount by this command "bindings::get_device(prt)".
>
> WARNING: 'becuase' may be misspelled - perhaps 'because'?
> #7:
> This can be confuse becuase, the function Arc::from_raw() from standard library, don't increment the refcount.
>
> Please also use a more descriptive commit summary, e.g.
> "rust: device: rename `Device::from_raw`".
>

Thanks for your suggestions...  I will make this changes.

> > ---
> >  rust/kernel/device.rs   | 2 +-
> >  rust/kernel/firmware.rs | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 851018eef885..ecffaff041e0 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -51,7 +51,7 @@ impl Device {
> >      ///
> >      /// It must also be ensured that `bindings::device::release` can be called from any thread.
> >      /// While not officially documented, this should be the case for any `struct device`.
> > -    pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef<Self> {
> > +    pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
>
> As a follow-up, it probably makes sense to also change the function body to
> just: `unsafe { Self::as_ref(ptr) }.into()`.

But if we change the function body that is suggested for you, the
function will not have your own refcount. If I don't wrong, the
Device::as_ref() expects the caller to have its own reference counter.
And if we change the behavior of function, your name don't need to be
changed, because your behavior will be equal the from_raw() from
standard library.

>
> >          // SAFETY: By the safety requirements, ptr is valid.
> >          // Initially increase the reference count by one to compensate for the final decrement once
> >          // this newly created `ARef<Device>` instance is dropped.
> > diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs
> > index dee5b4b18aec..13a374a5cdb7 100644
> > --- a/rust/kernel/firmware.rs
> > +++ b/rust/kernel/firmware.rs
> > @@ -44,7 +44,7 @@ fn request_nowarn() -> Self {
> >  ///
> >  /// # fn no_run() -> Result<(), Error> {
> >  /// # // SAFETY: *NOT* safe, just for the example to get an `ARef<Device>` instance
> > -/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) };
> > +/// # let dev = unsafe { Device::get_device(core::ptr::null_mut()) };
> >  ///
> >  /// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev)?;
> >  /// let blob = fw.data();
> > --
> > 2.46.2
> >