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

[Paolo Bonzini <pbonzini@redhat.com>]

On 2/7/25 14:33, Zhao Liu wrote:
>>> +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()".

Please keep init_full(); init() would be a version without some of the 
arguments (e.g. the TimerListGroup, or the attributes).

> 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).

Good point.

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

Yes, and we actually should do it sooner or later to add a PhantomPinned 
field, because timers can't move in memory!  But no need to do it now.

>>> +        scale: u32,

While at it, can you add constants for the scale, i.e.

     pub const NS: u32 = bindings::SCALE_NS;
     pub const US: u32 = bindings::SCALE_US;
     pub const MS: u32 = bindings::SCALE_MS;

? Using Timer::NS is clear enough and removes the need to import from 
"bindings".  At least in theory, bindings should not even have to be 
"pub" (or at least that's where the code should move towards).

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

Yeah, changing mod/del to modify/delete is fine!

Paolo