Re: [PATCH] rust: time: make Delta division and remainder fail consistently

[Miguel Ojeda]

On Mon, Sep 28, 2026 at 8:39 PM chenhan <chenhan0017.work@xxxxxxxxx> wrote:
>
> Tested on x86-64 and ARMv7 QEMU with CONFIG_SAMPLE_RUST_REPRO=y: zero
> divisors and i64::MIN / -1 panic for both APIs, while 10 / 3 returns 3
> and 10 % 3 returns 1 on both architectures.

This does not add much information, since we don't have the reproducer
here, i.e. what is `CONFIG_SAMPLE_RUST_REPRO`? I would instead explain
what the reproducer is, if it is important to do so.

> Fixes: 4521438fb076 ("rust: time: Implement basic arithmetic operations for Delta")

This points to something that could be backported, so if it is a fix,
then we probably want:

Cc: stable@xxxxxxxxxxxxxxx

However, this changes behavior -- did you check all callers etc.?

> Signed-off-by: chenhan <chenhan0017.work@xxxxxxxxx>

The kernel requires a "known identity", is "chenhan" one?

  https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

> +        // SAFETY: `divisor` is non-zero, and both operands are passed by value.

What "both operands are passed by value" is trying to say? i.e. what
precondition are you trying to satisfy?

> +        // SAFETY: `rem` points to a local variable and `divisor` is non-zero.
> +        unsafe { bindings::div_s64_rem(dividend, divisor, &mut rem) };

What happens in the `min / -1` case?

> +/// Panics if `rhs` is zero, or if the quotient overflows, i.e. if `self` is
> +/// `Delta::from_nanos(i64::MIN)` and `rhs` is `Delta::from_nanos(-1)`. Both
> +/// cases panic on 32-bit as well as on 64-bit; see `div_s64_or_panic()`.

I think the last sentence is not needed, i.e. if nothing is said, then
it should be understood apply to all cases. Perhaps we could keep the
reference to the function, if it were public, with an intra-doc link,
but it isn't, so I don't think we need it either.

> +    /// platforms, `div_s64_rem()`, takes an `s32` divisor. The dividend (`self`)

Perhaps `i32` with an intra-doc link instead?

> +    /// is a full `i64` on every architecture, so the width restriction applies

Please add intra-doc links where they work, e.g. [`i64`]

Thanks for the patch and welcome!

Cheers,
Miguel