Re: [PATCH 06/10] rust: add bindings for timer

[Zhao Liu]

> > +pub use bindings::QEMUTimer;
> > +
> > +use crate::{
> > +    bindings::{
> > +        self, qemu_clock_get_ns, timer_del, timer_init_full, timer_mod, 
> > QEMUClockType,
> > +        QEMUTimerListGroup,
> > +    },
> > +    callbacks::FnCall,
> > +};
> > +
> > +impl QEMUTimer {
> > +    pub fn new() -> Self {
> > +        Default::default()
> > +    }
> > +
> > +    pub fn timer_init_full<'timer, 'opaque: 'timer, T, F>(
> 
> General question - should the names:
> 
> - include the "timer" part, matching QEMU C code, or exclude it to avoid
> repetition? I would say remove it, 

I agree and I would name it "init()" instead of "init_full()".

> but I'm open to suggestions and other
> opinions
>
> - include the "QEMU" part? I'd say remove it, similar to ClockType below
> that is:
> 
> -pub use bindings::QEMUTimer;
> +pub use bindings::QEMUTimer as Timer;
> +pub use bindings::QEMUTimerList as TimerList;
> +pub use bindings::QEMUTimerListGroup as TimerListGroup;

I notice you've picked another way for IRQState, so I could follow that
like:

pub type Timer = bindings::QEMUTimer;

This style make it easy to add doc (timer binding currently lacks
doc, but I will add it as much as possible).

Another option may be to wrap QEMUTimer as what MemoryRegionOps did, but
timer has only 1 callback so I think it's not necessary.

> > +        &'timer mut self,
> > +        timer_list_group: Option<&QEMUTimerListGroup>,
> > +        clk_type: QEMUClockType,
> 
> Please take a ClockType instead.

Sure.

> > +        scale: u32,
> > +        attributes: u32,
> > +        _f: F,
> > +        opaque: &'opaque T,
> > +    ) where
> > +        F: for<'a> FnCall<(&'a T,)>,
> > +    {
> > +        /// timer expiration callback
> > +        unsafe extern "C" fn rust_timer_handler<T, F: for<'a> FnCall<(&'a 
> > T,)>>(
> > +            opaque: *mut c_void,
> > +        ) {
> > +            // SAFETY: the opaque was passed as a reference to `T`.
> > +            F::call((unsafe { &*(opaque.cast::<T>()) },))
> > +        }
> 
> Please add "let _: () = F::ASSERT_IS_SOME;", which is added by the
> qdev_init_clock_in() patch.

Sure. Added it to the beginning of this function.

> > +        let timer_cb: unsafe extern "C" fn(*mut c_void) = 
> > rust_timer_handler::<T, F>;
> > +
> > +        // SAFETY: the opaque outlives the timer
> > +        unsafe {
> > +            timer_init_full(
> > +                self,
> > +                if let Some(g) = timer_list_group {
> > +                    g as *const QEMUTimerListGroup as *mut 
> > QEMUTimerListGroup
> > +                } else {
> > +                    ::core::ptr::null_mut()
> > +                },
> > +                clk_type,
> > +                scale as c_int,
> > +                attributes as c_int,
> > +                Some(timer_cb),
> > +                (opaque as *const T).cast::<c_void>() as *mut c_void,
> > +            )
> > +        }
> > +    }
> > +
> > +    pub fn timer_mod(&mut self, expire_time: u64) {
> > +        unsafe { timer_mod(self as *mut QEMUTimer, expire_time as i64) }
> > +    }
> 
> This can take &self, because timers are thread-safe:
> 
>     pub fn timer_mod(&self, expire_time: u64) {
>         unsafe { timer_mod(self.as_mut_ptr(), expire_time as i64) }
>     }

timer_mod means "modify a timer", so I'd rename this method to "modify"

>     const fn as_mut_ptr(&self) -> *mut Self {
>         self as *const QEMUTimer as *mut _
>     }

Thanks!

> > +}
> > +
> > +impl Drop for QEMUTimer {
> > +    fn drop(&mut self) {
> > +        unsafe { timer_del(self as *mut QEMUTimer) }
> > +    }
> 
> timer_del() can be useful even outside Drop, so
> 
>     pub fn timer_del(&self) {
>         unsafe { timer_del(self.as_mut_ptr()) }
>     }
> 
> and just use self.timer_del() here.

OK, will also rename "timer_del" to "delete".

> > +}
> > +
> > +pub fn qemu_clock_get_virtual_ns() -> u64 {
> > +    // SAFETY:
> > +    // Valid parameter.
> > +    (unsafe { qemu_clock_get_ns(QEMUClockType::QEMU_CLOCK_VIRTUAL) }) as 
> > u64
> > +}
> 
> Not needed.

Yes!

> > +pub struct ClockType {
> > +    pub id: QEMUClockType,
> > +}
> 
> The field does not have to be "pub" (maybe "pub(self)", I'm not sure
> offhand).
> 

You're right! Making it private is enough, since I should also make
timer_init_full accept ClockType instead of QEMUClockType.

Thanks,
Zhao