linux-kernel - Re: [PATCH v2 2/3] rust: configfs: introduce rust support for configfs

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: <Z7SEeBFJT7fRtvVN@pollux>
Date: Tue, 18 Feb 2025 14:00:40 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Benno Lossin <benno.lossin@...ton.me>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Joel Becker <jlbec@...lplan.org>, Christoph Hellwig <hch@....de>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
	Waiman Long <longman@...hat.com>, Fiona Behrens <me@...enk.dev>,
	Charalampos Mitrodimas <charmitro@...teo.net>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] rust: configfs: introduce rust support for
 configfs

On Mon, Feb 17, 2025 at 12:08:22PM +0100, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@...ton.me> writes:
> 
> > On 07.02.25 15:41, Andreas Hindborg wrote:
> >> +//!
> >> +//! C header: [`include/linux/configfs.h`](srctree/include/linux/configfs.h)
> >> +//!
> >> +//! [C documentation]: srctree/Documentation/filesystems/configfs.rst
> >> +//! [rust_configfs.rs]: srctree/samples/rust/rust_configfs.rs
> >> +
> >> +use crate::alloc::flags;
> >> +use crate::container_of;
> >> +use crate::page::PAGE_SIZE;
> >> +use crate::prelude::*;
> >> +use crate::str::CString;
> >> +use crate::sync::Arc;
> >> +use crate::types::ForeignOwnable;
> >> +use crate::types::Opaque;
> >> +use core::cell::UnsafeCell;
> >> +use core::marker::PhantomData;
> >> +use core::ptr::addr_of;
> >> +use core::ptr::addr_of_mut;
> >
> > I usually would import this like so:
> >
> >     use crate::{
> >         alloc::flags,
> >         container_of,
> >         page::PAGE_SIZE,
> >         prelude::*,
> >         str::CString,
> >         sync::Arc,
> >         types::{ForeignOwnable, Opaque},
> >     };
> >     use core::{
> >         cell::UnsafeCell,
> >         marker::PhantomData,
> >         ptr::{addr_of, addr_of_mut},
> >     };
> >
> > To me this is more readable.
> 
> I disagree with that. I don't think what you suggest is easier to read,
> and it is much more difficult to work with when rebasing and merging
> things.

I have to agree that it is more difficult to work with. So far I used the style
as proposed by Benno, but it creates unncessary big and difficult to review
diffs when rustfmt moves things from a horizontal list to a vertical one and
vice versa.

> >> +    /// Implementations can use this method to do house keeping before
> >> +    /// `configfs` drops its reference to `Child`.
> >> +    fn drop_item(
> >
> > `drop` doesn't really fit here, I think something like `unlink_item`
> > fits better, since the child isn't actually dropped after this function
> > returns.
> 
> Yea, I know. But the function is called `drop_item` on the C side of
> things. Usually we keep the C names.

I agree C names should be kept as possible.

To me it seems obvious from the context, but maybe it'd still makes sense to add
a brief note that this callback's name is not related to 'drop' in the sense of
Rust?