Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type
From: Miguel Ojeda <hidden>
Date: 2024-10-20 13:05:50
Also in:
lkml, rust-for-linux
On Sat, Oct 19, 2024 at 8:41 PM Andrew Lunn [off-list ref] wrote:
So far, we only have one use case for this type, holding a duration to be passed to fsleep(). Rounding down what you pass to fsleep() is generally not what the user wants to do, and we should try to design the code to avoid this. The current helpers actually encourage such bugs, because they round down. Because of this they are currently unused. But they are a trap waiting for somebody to fall into. What the current users of this type really want is lossy helpers which round up. And by reviewing the one user against the API, it is clear the current API is wrong.
If you are talking about Rust's `fsleep()`, then "users" are not the ones calling the rounding up operation (the implementation of `fsleep()` is -- just like in the C side). If you are talking about C's `fsleep()`, then users are not supposed to use `bindings::`.
So i say, throw away the round down helpers until somebody really needs them. That avoids a class of bugs, passing a too low value to sleep. Add the one helper which is actually needed right now.
Eventually this type will likely get other methods for one reason or another, including the non-rounding ones. Thus we should be careful about the names we pick, which is why I was saying a method like `as_micros()` should not be rounding up. That would be confusing, and thus potentially end creating bugs, even if it is the only method you add today. Again, if you want to throw away all the unused methods and only have the rounding up one, then that is reasonable, but please let's not add misleading methods that could add more bugs than the ones you are trying to avoid. Please use `as_micros_ceil()` or similar.
There is potentially a better option. Make the actual sleep operation part of the type. All the rounding up then becomes part of the core, and the developer gets core code which just works correctly, with an API which is hard to make do the wrong thing.
That is orthogonal: whether the sleeping function is in `Delta` or not, users would not be the ones calling the rounding up operation (please see above). Anyway, by moving more things into a type, you are increasing its complexity. And, depending how modules are organized, you could be also giving visibility into the internals of that type, thus increasing the possibility of "implementation-side bugs" compared to the other way around (it does not really matter here, though). If you want it as a method for user ergonomics though, that is a different topic. Cheers, Miguel