linux-kernel - Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support

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: <878qvojj0o.fsf@kernel.org>
Date: Thu, 19 Sep 2024 07:43:19 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Anna-Maria Behnsen" <anna-maria@...utronix.de>,
  "Frederic Weisbecker" <frederic@...nel.org>,  "Thomas Gleixner"
 <tglx@...utronix.de>,  "Boqun Feng" <boqun.feng@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Alice
 Ryhl" <aliceryhl@...gle.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 02/14] rust: hrtimer: introduce hrtimer support

Hi Benno,

Thanks for the feedback. I will incorporate all the whitespace
suggestions you have =F0=9F=91=8D

"Benno Lossin" <benno.lossin@...ton.me> writes:

> On 18.09.24 00:27, Andreas Hindborg wrote:
[...]
>> +
>> +impl<T> Timer<T> {
>> +    /// Return an initializer for a new timer instance.
>> +    pub fn new() -> impl PinInit<Self>
>> +    where
>> +        T: TimerCallback,
>> +    {
>> +        pin_init!( Self {
>
> I would remove the space after the `(`.
> Would be great if we had rustfmt support for custom macros.

Yes, that would be great!

>
>> +            // INVARIANTS: We initialize `timer` with `hrtimer_init` be=
low.
>> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtim=
er| {
>> +                // SAFETY: By design of `pin_init!`, `place` is a point=
er live
>> +                // allocation. hrtimer_init will initialize `place` and=
 does not
>> +                // require `place` to be initialized prior to the call.
>> +                unsafe {
>> +                    bindings::hrtimer_init(
>> +                        place,
>> +                        bindings::CLOCK_MONOTONIC as i32,
>> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
>> +                    );
>> +                }
>> +
>> +                // SAFETY: `place` is pointing to a live allocation, so=
 the deref
>> +                // is safe.
>> +                let function: *mut Option<_> =3D
>
> Do you really need this type hint?

Apparently not!

[...]
>> +pub trait TimerPointer: Sync + Sized {
>> +    /// A handle representing a scheduled timer.
>> +    ///
>> +    /// If the timer is armed or if the timer callback is running when =
the
>> +    /// handle is dropped, the drop method of `TimerHandle` should not =
return
>> +    /// until the timer is unarmed and the callback has completed.
>> +    ///
>> +    /// Note: It must be safe to leak the handle.
>> +    type TimerHandle: TimerHandle;
>
> Why does this need to be an associated type? Couldn't we have a
> `TimerHandle<T>` struct? The schedule function below could then return
> `TimerHandle<Self>`.

At one point, I had some cycles in trait resolution. Moving generics to
associated types solved that issue. Maybe this can be changed to a
generic. But are generics preferred over associated types for some
reason?


Best regards,
Andreas