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: <ZvqkAuxWZIMZshN_@pollux>
Date: Mon, 30 Sep 2024 15:13:38 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Guilherme Giacomo Simoes <trintaeoitogc@...il.com>
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

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`".

> ---
>  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()`.

>          // 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
>