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: <120631ca-3da3-441c-9536-43ee9dbe3b01@proton.me> Date: Sat, 21 Oct 2023 07:36:31 +0000 From: Benno Lossin <benno.lossin@...ton.me> To: Andrew Lunn <andrew@...n.ch> Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers On 20.10.23 20:42, Andrew Lunn wrote: >>> +//! All the PHYLIB helper functions for `phy_device` modify some members in `phy_device`. Except for >>> +//! getter functions, [`Device`] methods take `&mut self`. This also applied to `read()`, which reads >>> +//! a hardware register and updates the stats. >> >> I would use [`Device`] instead of `phy_device`, since the Rust reader >> might not be aware what wraps `phy_device`. > > We don't want to hide phy_device too much, since at the moment, the > abstraction is very minimal. Anybody writing a driver is going to need > a good understanding of the C code in order to find the helpers they > need, and then add them to the abstraction. So i would say we need to > explain the relationship between the C structure and the Rust > structure, to aid developers. There is a comment on `Device` that explains that it wraps `phy_device`. Since [`Device`] is a link, readers who do not know what it means can immediately click the link and find out. This is not possible with `phy_device`, since you have to search the web for it, so I would prefer to use the link. >>> + /// Returns true if the link is up. >>> + pub fn get_link(&self) -> bool { >> >> I still think this name should be changed. My response at [1] has not yet >> been replied to. This has already been discussed before: >> - https://lore.kernel.org/rust-for-linux/2023100237-satirical-prance-bd57@gregkh/ >> - https://lore.kernel.org/rust-for-linux/20231004.084644.50784533959398755.fujita.tomonori@gmail.com/ >> - https://lore.kernel.org/rust-for-linux/CALNs47syMxiZBUwKLk3vKxzmCbX0FS5A37FjwUzZO9Fn-iPaoA@mail.gmail.com/ >> >> And I want to suggest to change it to `is_link_up`. >> >> Reasons why I do not like the name: >> - `get_`/`put_` are used for ref counting on the C side, I would like to >> avoid confusion. >> - `get` in Rust is often used to fetch a value from e.g. a datastructure >> such as a hashmap, so I expect the call to do some computation. >> - getters in Rust usually are not prefixed with `get_`, but rather are >> just the name of the field. >> - in this case I like the name `is_link_up` much better, since code becomes >> a lot more readable with that. >> - I do not want this pattern as an example for other drivers. >> >> [1]: https://lore.kernel.org/rust-for-linux/f5878806-5ba2-d932-858d-dda3f55ceb67@proton.me/ >> >>> + const LINK_IS_UP: u32 = 1; >>> + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. >>> + let phydev = unsafe { *self.0.get() }; >>> + phydev.link() == LINK_IS_UP >>> + } > > During the reviews we have had a lot of misunderstanding what this > actually does, given its name. Some thought it poked around in > registers to get the current state of the link. Some thought it > triggered the PHY to establish a link. When in fact its just a dumb > getter. And we have a few other dumb getters and setters. IMO `is_link_up` would indicate that it is a dumb getter. > So i would prefer something which indicates its a dumb getter. If the > norm of Rust is just the field name, lets just use the field name. But > we should do that for all the getters and setters. Is there a naming > convention for things which take real actions? For bool getters it would be the norm to use `is_` as the prefix, see [1]. In this case I would say it is also natural to not use `is_link`, but rather `is_link_up`, since the former sounds weird. [1]: https://doc.rust-lang.org/std/?search=is_ > And maybe we need to add a comment: Get the current link state, as > stored in the [`Device`]. Set the duplex value in [`Device`], etc. Sure, we can document dumb getters explicitly, I would prefer to do: Getter for the current link state. Setter for the duplex value. I don't think that we need to link to `Device`, since the functions are defined on that type. -- Cheers, Benno
Powered by blists - more mailing lists