Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Tue, 19 Aug 2025 09:49:38 +0900
"Alexandre Courbot" <acourbot@xxxxxxxxxx> wrote:

> On Sun Aug 17, 2025 at 1:47 PM JST, FUJITA Tomonori wrote:
>> Add read_poll_timeout function which poll periodically until a
>> condition is met or a timeout is reached.
>>
>> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
>> macro and a simple wrapper for Rust doesn't work. So this implements
>> the same functionality in Rust.
>>
>> The C version uses usleep_range() while the Rust version uses
>> fsleep(), which uses the best sleep method so it works with spans that
>> usleep_range() doesn't work nicely with.
>>
>> The sleep_before_read argument isn't supported since there is no user
>> for now. It's rarely used in the C version.
>>
>> Reviewed-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
>> Reviewed-by: Fiona Behrens <me@xxxxxxxxxx>
>> Tested-by: Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@xxxxxxxxx>
> 
> Tested this with nova-core, and it seems to work fine!
> 
> Reviewed-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> Tested-by: Alexandre Courbot <acourbot@xxxxxxxxxx>

Thanks!

> Just one last comment about the documentation below.
> 
> <snip>
>> +/// Polls periodically until a condition is met or a timeout is reached.
>> +///
>> +/// The function repeatedly executes the given operation `op` closure and
>> +/// checks its result using the condition closure `cond`.
>> +///
>> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
>> +/// Otherwise, it waits for a duration specified by `sleep_delta`
>> +/// before executing `op` again.
>> +///
>> +/// This process continues until either `cond` returns `true` or the timeout,
>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
> 
> For precision: "This process continues until either `op` returns and
> error, `cond` returns `true`, or the timeout specified by
> `timeout_delta` is reached."

Indeed, `op` should be mentioned here.

I think it should be "op returns an error", not "op returns and error",
right?

There are three alternatives, so Iʼd remove "either". The sentence
would be:

This process continues until either `op` returns an error, `cond`
returns `true`, or the timeout specified by `timeout_delta` is
reached.

I just realized that "If `timeout_delta` is `None`," comment is outdated. I'll update.


>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>> +///
>> +/// This function can only be used in a nonatomic context.
> 
> Here I'd add an errors section:
> 
> # Errors
> 
> If `op` returns an error, then that error is returned directly.
> 
> If the timeout specified by `timeout_delta` is reached, then
> `Err(ETIMEDOUT)` is returned.

Thanks, looks nice. I'll add it.


>> +    loop {
>> +        let val = op()?;
>> +        if cond(&val) {
>> +            // Unlike the C version, we immediately return.
>> +            // We know the condition is met so we don't need to check again.
> 
> nit: this comment looks superfluous to me, this is a different
> implementation from the C version anyway.

A previous review mentioned that, since this function shares the same
name as the C version, it's good to make the different behavior
explicit. It may be a bit redundant, but I'd prefer to keep it.