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
| ||
|
Message-Id: <20231004.084644.50784533959398755.fujita.tomonori@gmail.com> Date: Wed, 04 Oct 2023 08:46:44 +0900 (JST) From: FUJITA Tomonori <fujita.tomonori@...il.com> To: andrew@...n.ch Cc: fujita.tomonori@...il.com, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com Subject: Re: [PATCH v1 1/3] rust: core abstractions for network PHY drivers On Mon, 2 Oct 2023 17:24:17 +0200 Andrew Lunn <andrew@...n.ch> wrote: >> + /// Gets the id of the PHY. >> + pub fn id(&mut self) -> u32 { >> + let phydev = self.0.get(); >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + unsafe { (*phydev).phy_id } >> + } > > I somewhat agree with GregKH here. It will be easier to review and > maintain if the naming of well known things stay the same in the C and > Rust world. So phy_id. However.... phy_id() is fine by me. The complete type name is `net::phy::Device` so I guess that the method names usually don't start with `phy`. But we maintain both C and Rust so I think that we need a balance between them. >> + /// Gets the state of the PHY. >> + pub fn state(&mut self) -> DeviceState { >> + let phydev = self.0.get(); >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + let state = unsafe { (*phydev).state }; >> + match state { >> + bindings::phy_state::PHY_DOWN => DeviceState::Down, >> + bindings::phy_state::PHY_READY => DeviceState::Ready, >> + bindings::phy_state::PHY_HALTED => DeviceState::Halted, >> + bindings::phy_state::PHY_ERROR => DeviceState::Error, >> + bindings::phy_state::PHY_UP => DeviceState::Up, >> + bindings::phy_state::PHY_RUNNING => DeviceState::Running, >> + bindings::phy_state::PHY_NOLINK => DeviceState::NoLink, >> + bindings::phy_state::PHY_CABLETEST => DeviceState::CableTest, >> + } >> + } >> + >> + /// Returns true if the link is up. >> + pub fn get_link(&mut self) -> bool { >> + const LINK_IS_UP: u32 = 1; >> + let phydev = self.0.get(); >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + unsafe { (*phydev).link() == LINK_IS_UP } >> + } > > Naming is hard. > > This one is trickier and shows a difference between C and Rust. C just > does phydev->link and treats it as a boolean, setter/getters are not > needed. But Rust does seem to need setter/getters, and it is a lot > less clear what link() does. get_link() is a bit more > obvious. has_link() would also work. But as GregKH said, get_foo() and > put_foo() are often used to represent getting a reference on an object > and releasing it. I am however of the opinion that many driver writers > don't understand locking, so it is best to hide all the locking in the > core. I would not actually expect to see a PHY driver need to take a > reference on anything. > > Since we forced into a world of getter/setter, the previous one > probably should be get_phy_id() and we want consistent set_ and get_ > prefixes for plain accesses to members without further interpretation. get/set_something names aren't commonly used in Rust, I guess. Some examples follows in the standard library. https://doc.rust-lang.org/stable/std/net/struct.TcpStream.html there are set_linger(), set_nodelay(), set_read_timeout(), set_write_timeout(). correspondingly, linger(), nodelay(), read_timeout(), write_timeout() are provided. https://doc.rust-lang.org/stable/std/io/struct.Cursor.html There are set_position() and position(). As I wrote above, I don't think that we need to follow Rust naming practices strictly, as long as there are patterns in Rust bindings. >> + /// Returns true if auto-negotiation is enabled. >> + pub fn is_autoneg_enabled(&mut self) -> bool { >> + let phydev = self.0.get(); >> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >> + unsafe { (*phydev).autoneg() == bindings::AUTONEG_ENABLE } >> + } > > Should this maybe be get_autoneg_enabled()? I don't know. I think that we can leave this name alone since tis_something() names are used for OS related functions in Rust.
Powered by blists - more mailing lists