> > +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
