linux-kernel - Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fad19413-8d58-4cf5-82e6-8d4410fd7e50@lunn.ch>
Date: Sat, 19 Oct 2024 20:41:21 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, hkallweit1@...il.com,
	tmgross@...ch.edu, ojeda@...nel.org, alex.gaynor@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...sung.com, aliceryhl@...gle.com,
	anna-maria@...utronix.de, frederic@...nel.org, tglx@...utronix.de,
	arnd@...db.de, jstultz@...gle.com, sboyd@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

On Sat, Oct 19, 2024 at 02:21:45PM +0200, Miguel Ojeda wrote:
> On Fri, Oct 18, 2024 at 6:55 PM Andrew Lunn <andrew@...n.ch> wrote:
> >
> > Did you see my other comment, about not actually using these helpers?
> > I _guess_ it was not used because it does not in fact round up. So at
> > least for this patchset, the helpers are useless. Should we be adding
> > useless helpers? Or should we be adding useful helpers? Maybe these
> > helpers need a different name, and do actually round up?
> 
> Yeah, I saw that -- if I understand you correctly, you were asking why
> `as_nanos()` is used and not `as_secs()` directly (did you mean
> `as_micros()`?) by adding rounding on `Delta`'s `as_*()` methods.
> 
> So my point here was that a method with a name like `as_*()` has
> nothing to do with rounding, so I wouldn't add rounding here (it would
> be misleading).
> 
> Now, we can of course have rounding methods in `Delta` for convenience
> if that is something commonly needed by `Delta`'s consumers like
> `fsleep()`, that is fine, but those would need to be called
> differently, e.g. `to_micros_ceil`: `to_` since it is not "free"
> (following e.g. `to_radians`) and + `_ceil` to follow `div_ceil` from
> the `int_roundings` feature (and shorter than something like
> `_rounded_up`).
> 
> In other words, I think you see these small `as_*()` functions as
> "helpers" to do something else, and thus why you say we should provide
> those that we need (only) and make them do what we need (the
> rounding). That is fair.
> 
> However, another way of viewing these is as typical conversion methods
> of `Delta`, i.e. the very basic interface for a type like this. Thus
> Tomonori implemented the very basic interface, and since there was no
> rounding, then he added it in `fsleep()`.
> 
> I agree having rounding methods here could be useful, but I am
> ambivalent as to whether following the "no unused code" rule that far
> as to remove very basic APIs for a type like this.

I would say ignoring the rule about an API always having a user has
led to badly designed code, which is actually going to lead to bugs.

It is clearly a philosophical point, what the base of the type is, and
what are helpers. For me, the base is a i64 representing nano seconds,
operations too add, subtract and compare, and a way to get the number
of nanoseconds in and out.

I see being able to create such a type from microseconds, millisecond,
seconds and decades as helpers on top of this base. Also, being able
to extract the number of nanoseconds from the type but expressed in
microseconds, milliseconds, seconds and months are lossy helpers on
top of the base.

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.

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.

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.

	Andrew