> > > #[repr(C)] > > > -#[derive(Debug, qemu_api_macros::Object, qemu_api_macros::offsets)] > > > > This is the issue I also met, so why not drive "Debug" for BqlRefCell? > > > > Because it is not entirely possible to do it safely--there could be > outstanding borrows that break invariants and cause debug() to fail. Maybe > we could implement it on BqlRefCell<PL011Registers> with a custom derive > macro... > > RefCell doesn't implement Debug either for the same reason.
Thank you for the clarification, I understand now (I was indeed puzzled as to why RefCell didn't do this). > I tried to do this in [*]. Do we need to reconsider this? > > > > [*]: > > https://lore.kernel.org/qemu-devel/20241205060714.256270-3-zhao1....@intel.com/ > > > > > +#[derive(qemu_api_macros::Object, qemu_api_macros::offsets)] > > > /// PL011 Device Model in QEMU > > > pub struct PL011State { > > > pub parent_obj: ParentField<SysBusDevice>, > > > pub iomem: MemoryRegion, > > > #[doc(alias = "chr")] > > > pub char_backend: CharBackend, > > > - pub regs: PL011Registers, > > > + pub regs: BqlRefCell<PL011Registers>, > > > > This is a good example on the usage of BqlRefCell! > > > > //! `BqlRefCell` is best suited for data that is primarily accessed by the > > //! device's own methods, where multiple reads and writes can be grouped > > within > > //! a single borrow and a mutable reference can be passed around. " > > > > Yeah, the comment was inspired by this usage and not vice versa. :D > > > /// QEMU interrupts > > > /// > > > /// ```text > > > @@ -530,8 +530,8 @@ fn post_init(&self) { > > > } > > > } > > > > > > + #[allow(clippy::needless_pass_by_ref_mut)] > > > > How did you trigger this lint error? I switched to 1.84 and didn't get > > any errors (I noticed that 1.84 fixed the issue of ignoring `self` [*], > > but it still doesn't seem to work on my side). > > > > I will double check. But I do see that there is no mut access inside, at > least not until the qemu_chr_fe_accept_input() is moved here. Unfortunately > until all MemoryRegion and CharBackend bindings are available the uses of > &mut and the casts to *mut are really really wonky. yes, I agree here we should remove mut :-). (if needless_pass_by_ref_mut doesn't work on this place, I think we can drop it.) > (On the other hand it wouldn't be possible to have a grip on the qemu_api > code without users). > > Paolo > > > @@ -603,19 +603,19 @@ pub fn realize(&mut self) { > > > } > > > > > > pub fn reset(&mut self) { > > > > In principle, this place should also trigger `needless_pass_by_ref_mut`. > > > > Yes but clippy hides it because this function is assigned to a function > pointer const. At least I think so---the point is more generally that you > can't change &mut to & without breaking compilation. Make sense! > > > - self.regs.reset(); > > > + self.regs.borrow_mut().reset(); > > > } > > > > [snip] > > > > > @@ -657,10 +657,10 @@ pub fn post_load(&mut self, _version_id: u32) -> > > Result<(), ()> { > > > pub unsafe extern "C" fn pl011_receive(opaque: *mut c_void, buf: *const > > u8, size: c_int) { > > > unsafe { > > > debug_assert!(!opaque.is_null()); > > > - let mut state = > > NonNull::new_unchecked(opaque.cast::<PL011State>()); > > > + let state = NonNull::new_unchecked(opaque.cast::<PL011State>()); > > > > Perhaps we can use NonNull::new and unwrap()? Then debug_assert! is > > unnecessary. > > > > let state = unsafe { > > NonNull::new(opaque.cast::<PL011State>()).unwrap().as_ref() }; > > > > Yeah, though that's preexisting and it's code that will go away relatively > soon. I tried to minimize unrelated changes and changes to these temporary > unsafe functions, but in some cases there were some that sneaked in. > > Let me know what you prefer. > I prefer to use NonNull::new and unwrap(). Too much assert() pattern is not user-friendly. I also think it's unnecessary to change NonNull interface in this patch, we can see what's left when you're done with the most QAPI work. Thanks, Zhao
