netdev - Re: [PATCH net-next v4 1/4] rust: core abstractions for network PHY drivers

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: <ZSq5aY9R66vwcd6k@Boquns-Mac-mini.home>
Date: Sat, 14 Oct 2023 08:53:13 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, andrew@...n.ch,
	miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu,
	wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v4 1/4] rust: core abstractions for network PHY
 drivers

On Sat, Oct 14, 2023 at 02:54:30PM +0000, Benno Lossin wrote:
[...]
> >>>
> >>> Boqun asked me to drop mut on v3 review and then you ask why on v4?
> >>> Trying to find a way to discourage developpers to write Rust
> >>> abstractions? :)
> >>>
> >>> I would recommend the Rust reviewers to make sure that such would
> >>> not happen. I really appreciate comments but inconsistent reviewing is
> >>> painful.
> >>
> >> I agree with Boqun. Before Boqun's suggestion all functions were
> >> `&mut self`. Now all functions are `&self`. Both are incorrect. A
> >> function that takes `&mut self` can modify the state of `Self`,
> >> but it is weird for it to not modify anything at all. Such a
> >> function also can only be called by a single thread (per instance
> >> of `Self`) at a time. Functions with `&self` cannot modify the
> >> state of `Self`, except of course with interior mutability. If
> >> they do modify state with interior mutability, then they should
> >> have a good reason to do that.
> >>
> >> What I want you to do here is think about which functions should
> >> be `&mut self` and which should be `&self`, since clearly just
> >> one or the other is wrong here.
> > 
> > https://lore.kernel.org/netdev/20231011.231607.1747074555988728415.fujita.tomonori@gmail.com/T/#mb7d219b2e17d3f3e31a0d05697d91eb8205c5c6e
> > 
> > Hmm, I undertood that he suggested all mut.
> 

To be clear, I was only talking about phy_id() at the email thread,

My original reply:

> >>> >> +    pub fn phy_id(&mut self) -> u32 {
> >>> > 
> >>> > This function doesn't modify the `self`, why does this need to be a
> >>> > `&mut self` function? Ditto for a few functions in this impl block.

so Tomo, I wasn't suggesting dropping `mut` for all functions (I used
the words "a few" not "all"), just dropping them accordingly.

Actually this is an excellent example for the fragile of relying on
implicit requirements ;-) The original intent got lost track in a few
email exchanges. 

API soundness is not the sliver bullet, but it at least tries to bring
a common base for handling such problems. Again maybe you now can
understand why we push so hard on "tiny" things again and again.

Also of course Rust is still somehow immature, so if you have an idea
but cannot express in Rust, feel free to call it out, we can work
together to 1) find a temp solution and 2) push Rust to improve. That's
one of the points of the experiment.

Regards,
Boqun

> That remark seems to me to only apply to the return type of
> `assume_locked` in that thread.
>