Thread (64 messages) 64 messages, 6 authors, 2024-10-25

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help