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

[Zhao Liu <zhao1.liu@intel.com>]

On Thu, Dec 05, 2024 at 07:46:52PM +0100, Paolo Bonzini wrote:
> Date: Thu, 5 Dec 2024 19:46:52 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [RFC 07/13] rust: add bindings for timer
> 
> On 12/5/24 07:07, Zhao Liu wrote:
> > +impl QEMUTimer {
> > +    fn new() -> Self {
> > +        QEMUTimer {
> > +            expire_time: 0,
> > +            timer_list: ::core::ptr::null_mut(),
> > +            cb: None,
> > +            opaque: ::core::ptr::null_mut(),
> > +            next: ::core::ptr::null_mut(),
> > +            attributes: 0,
> > +            scale: 0,
> > +        }
> > +    }
> 
> Please implement Zeroable instead and make this just Self::ZERO.

Sure, will do.

> > +    // TODO: Consider how to avoid passing in C style callbacks directly.
> > +    fn timer_new_full<T: QEMUTimerImpl>(
> > +        timer_list_group: Option<&mut QEMUTimerListGroup>,
> > +        clk_type: QEMUClockType,
> > +        scale: u32,
> > +        attributes: u32,
> > +        opaque: &mut T::Opaque,
> 
> This gets tricky when you have more than one timer per device.  With the right
> infrastructure we can make this something like
> 
>     fn timer_init_full<'a, 'b: 'a, T, F: 'b Fn(&'b T)>(
>         &'a mut self,
>         timer_list_group: Option<&mut QEMUTimerListGroup>,
>         clk_type: QEMUClockType,
>         scale: u32,
>         attributes: u32,
>         f: &F,
>         opaque: &'b T) {
>         // SAFETY: the opaque outlives the timer
>         unsafe {
>             timer_init_full(...)
>         }
>     }
> 
> So I guess that's another thing I have to extract and post. :)

Thank you so much for your help! I'm happy to help test and review.

(if you feel you don't have enough time, I'm also willing to try this
way.. and if there's any issue, I'll let you know...)

whichever way you think is solid, I'm both happy with it.

> BTW don't bother with timer_new, only do the non-pointer timer_init.

Do you mean timer_init is enough, and timer_new and its variants are not
necessary?

> > +    pub fn timer_del(&mut self) {
> > +        unsafe { timer_del(self as *mut QEMUTimer) };
> > +    }
> > +}
> 
> Please also add a Drop implementation that calls timer_del.

Sure!

> > +pub fn qemu_clock_get_virtual_ns() -> u64 {
> > +    // SAFETY:
> > +    // Valid parameter.
> > +    (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as u64
> > +}
> 
> Maybe something like
> 
> pub struct Clock {
>     id: QEMUClockType
> }
> 
> impl Clock {
>     pub fn get_ns() -> u64 {
>         // SAFETY: cannot be created outside this module, therefore id
>         // is valid
>         (unsafe { qemu_clock_get_ns(self.id) }) as u64
>     }
> }
> 
> pub static CLOCK_VIRTUAL: Clock = Clock { id: QEMUClockType::QEMU_CLOCK_VIRTUAL };
> // etc.
> 
> and then the user can do timer::CLOCK_VIRTUAL::get_ns().

More idiomatic, thank you! (I feel like I need to switch my mindset even
more from current C style.)

Regards,
Zhao