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

[Danilo Krummrich]

On Mon, Feb 17, 2025 at 12:08:22PM +0100, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@xxxxxxxxx> 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?