Re: [PATCH v8 2/2] rust: xarray: Add an abstraction for XArray

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/21/24 6:20 PM, Boqun Feng wrote:
On Thu, Mar 21, 2024 at 05:22:11PM -0700, John Hubbard wrote:
On 3/19/24 2:32 AM, Philipp Stanner wrote:
...

+        if ret < 0 {
+            Err(Error::from_errno(ret))
+        } else {
+            guard.dismiss();
+            Ok(id as usize)
+        }

You could make this easier to read using to_result.

to_result(ret)?;
guard.dismiss();
Ok(id as usize)

My 2 cents, I'd go for classic kernel style:


if ret < 0 {
      return Err(...);
}

guard.dismiss();
Ok(id as usize)


Yes.

As a "standard" C-based kernel person who is trying to move into Rust
for Linux, I hereby invoke my Rust-newbie powers to confirm that the
"clasic kernel style" above goes into my head much faster and easier,
yes. :)


Hope I'm still belong to a "standard C-based kernel person" ;-) My
problem on "if ret < 0 { ... }" is: what's the type of "ret", and is it
"negative means failure" or "non-zero means failure"? Now for this
particular code, the assignment of "ret" and the use of "ret" is pretty
close, so it doesn't matter. But in the code where "ret" is used for
multiple function calls and there is code in-between, then I'd prefer we
use `to_result` (i.e. `Result` type and question mark operator).

No argument there. I was more focused on the readability of: do a clean
check for failure, and returning directly in that case. And then
continuing on with non-failure-case code. The to_result() part makes
sense and can fit right into that.

I hope I'm not violating any "this is how Rust idioms must be"
conventions. But if not, then all other things being equal, it is of
course a nice touch to make the code more readable to the rest of the
kernel folks.


One more extra point from myself only: if one is using Rust for drivers
or subsystem they are going to take care of it in the future, it's
totally fine for them to pick coding styles that they feel comfortable,
I don't want to make people who do the real work feel frustrated because
"this is how Rust idioms must be", also I don't believe tools should
restrict people. But in the "kernel" crate (i.e. for core kernel part),
I want to make it "Rusty" since it's the point of the experiement ("you
asked for it ;-)).


OK, I understand that reasoning.
Hope we can find a balanced point when we learn more and more ;-)

Yes. And to continue with the metaphor, I'm attempting here to put a
light thumb on the scale. :)


thanks,
--
John Hubbard
NVIDIA





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux