Re: [RFC 07/13] rust: add bindings for timer

[Paolo Bonzini]

On Tue, Jan 14, 2025 at 4:18 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> ...Now I have a draft for timer binding:
>
> * timer binding:
>
> impl QEMUTimer {
>     pub fn new() -> Self {
>         Zeroable::ZERO
>     }

Maybe Default too (not sure if you even need new())?

>     pub fn timer_init_full<'a, 'b: 'a, T, F>(

It's better to use longer names like 'timer and 'opaque. But it's also
always possible to pass a longer lifetime, so 'opaque: 'timer is
strictly speaking not needed: you can just use &'timer T which in turn
means that lifetime elision applies. That said, I think I like the
idea of using 'timer and 'opaque lifetimes here, for clarity.

>         &'a mut self,
>         timer_list_group: Option<&mut QEMUTimerListGroup>,

I think QEMUTimerListGroup can (should) be shared because it's thread safe.

>         clk_type: QEMUClockType,
>         scale: u32,
>         attributes: u32,
>         _f: &F,

Better: "_f: &'static F", or even "_f: F" if it works.

>         opaque: &'b T,
>     ) where
>         F: for<'c> FnCall<(&'c T,)> + 'b,

'b ('opaque) is not needed here because the opaque is passed _into_
the function (thus its lifetime is 'c). 'timer would make sense, but
in fact the function itself is always 'static (see FnCall declaration)
so it is unnecessary to add a lifetime to FnCall.

> fn timer_handler(timer_cell: &BqlRefCell<HPETTimer>) {
>     timer_cell.borrow_mut().callback()
> }
>
> impl HPETTimer {
>     ...
>
>     fn init_timer_with_state(&mut self) {
>         let index = self.index;
>         let cb = |cell: &BqlRefCell<HPETTimer>| {
>             timer_handler(cell);
>         };

Why is the anonymous function needed?  Can you just pass "timer_handler"?

> Is this timer binding as you expected? Hope I am on the right track. :-)

If the only correction is to the function declaration, that's as good
as it can be for Rust! ;)

Thanks,

Paolo