| Message ID | 20250125125137.1223277-4-zhao1.liu@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: Add HPET timer device | expand |
On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > This is useful to hanlde InterruptSource slice and pass it to C > bindings. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> This may be a bad suggestion, after all. clippy complains that you're casting &[*mut IRQState] to mutable (https://rust-lang.github.io/rust- clippy/master/#as_ptr_cast_mut). I can think of two solutions: 1) add #[allow(clippy::as_ptr_cast_mut)] and explain with a comment // Casting to *mut *mut IRQState is valid, because // the source slice `pins` uses interior mutability. 2) drop as_slice_of_qemu_irq() and replace it with something like pub(crate) const fn slice_as_ptr(slice: &[Self]) -> *mut *mut IRQState { slice[0].as_ptr() } You choose. Paolo
On Wed, Jan 29, 2025 at 11:51:02AM +0100, Paolo Bonzini wrote: > Date: Wed, 29 Jan 2025 11:51:02 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert > [InterruptSource] to [*mut IRQState] > > On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > > > This is useful to hanlde InterruptSource slice and pass it to C > > bindings. > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > This may be a bad suggestion, after all. clippy complains that you're > casting &[*mut IRQState] to mutable (https://rust-lang.github.io/rust- > clippy/master/#as_ptr_cast_mut). > > I can think of two solutions: > > 1) add #[allow(clippy::as_ptr_cast_mut)] and explain with a comment > > // Casting to *mut *mut IRQState is valid, because > // the source slice `pins` uses interior mutability. > > 2) drop as_slice_of_qemu_irq() and replace it with something like > > pub(crate) const fn slice_as_ptr(slice: &[Self]) -> *mut *mut IRQState { > slice[0].as_ptr() > } > > You choose. > :) I choose the second way, which goes back to the initial codes. Though an empty slice w/o any check would cause a runtime error like: "thread '<unnamed>' panicked at 'index out of bounds: the len is 0 but the index is 0'" Adding an assert to ensure a non-empty slice is better. Thanks, Zhao
On Fri, Feb 07, 2025 at 03:10:53PM +0800, Zhao Liu wrote: > Date: Fri, 7 Feb 2025 15:10:53 +0800 > From: Zhao Liu <zhao1.liu@intel.com> > Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert > [InterruptSource] to [*mut IRQState] > > On Wed, Jan 29, 2025 at 11:51:02AM +0100, Paolo Bonzini wrote: > > Date: Wed, 29 Jan 2025 11:51:02 +0100 > > From: Paolo Bonzini <pbonzini@redhat.com> > > Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert > > [InterruptSource] to [*mut IRQState] > > > > On Sat, Jan 25, 2025 at 1:32 PM Zhao Liu <zhao1.liu@intel.com> wrote: > > > > > > This is useful to hanlde InterruptSource slice and pass it to C > > > bindings. > > > > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > > > > This may be a bad suggestion, after all. clippy complains that you're > > casting &[*mut IRQState] to mutable (https://rust-lang.github.io/rust- > > clippy/master/#as_ptr_cast_mut). > > > > I can think of two solutions: > > > > 1) add #[allow(clippy::as_ptr_cast_mut)] and explain with a comment > > > > // Casting to *mut *mut IRQState is valid, because > > // the source slice `pins` uses interior mutability. > > I agree it's the best to use interior mutability. Just to confirm, I check with `cargo +nightly clippy` but it doesn't complain about this case. Should I switch to another version of clippy when I do such check? (currently I'm using v0.1.63 clippy as well, to match rustc.) Thanks, Zhao
Il ven 7 feb 2025, 08:25 Zhao Liu <zhao1.liu@intel.com> ha scritto: > Just to confirm, I check with `cargo +nightly clippy` but it doesn't > complain about this case. Should I switch to another version of clippy > when I do such check? (currently I'm using v0.1.63 clippy as well, to > match rustc.) > I don't remember exactly how I noticed it—I am pretty sure it broke in CI though. Maybe the change to add rust_version hid it. To answer your question, generally the idea is that we use the latest version of the developer tools (cargo, rustfmt, clippy). In particular old versions of cargo don't support retrieving clippy settings from Cargo.toml. Paolo > Thanks, > Zhao > >
On Fri, Feb 07, 2025 at 10:57:11AM +0100, Paolo Bonzini wrote: > Date: Fri, 7 Feb 2025 10:57:11 +0100 > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert > [InterruptSource] to [*mut IRQState] > > Il ven 7 feb 2025, 08:25 Zhao Liu <zhao1.liu@intel.com> ha scritto: > > > Just to confirm, I check with `cargo +nightly clippy` but it doesn't > > complain about this case. Should I switch to another version of clippy > > when I do such check? (currently I'm using v0.1.63 clippy as well, to > > match rustc.) > > > > I don't remember exactly how I noticed it—I am pretty sure it broke in CI > though. Maybe the change to add rust_version hid it. > > To answer your question, generally the idea is that we use the latest > version of the developer tools (cargo, rustfmt, clippy). In particular old > versions of cargo don't support retrieving clippy settings from Cargo.toml. This one inspired me. I'm thinking that even though we deleted the README of pl011, a gneral guide or doc section (at somewhere, e.g., docs/devel/style.rst or docs/devel/submitting-a-patch.rst) would be helpful. At least, that could remind contributor to check patches with latest toolchain instead of letting the maintainer's CI smoking fail. Thanks, Zhao
Il sab 8 feb 2025, 11:55 Zhao Liu <zhao1.liu@intel.com> ha scritto: > On Fri, Feb 07, 2025 at 10:57:11AM +0100, Paolo Bonzini wrote: > > Date: Fri, 7 Feb 2025 10:57:11 +0100 > > From: Paolo Bonzini <pbonzini@redhat.com> > > Subject: Re: [PATCH 03/10] rust/irq: Add a helper to convert > > [InterruptSource] to [*mut IRQState] > > > > Il ven 7 feb 2025, 08:25 Zhao Liu <zhao1.liu@intel.com> ha scritto: > > > > > Just to confirm, I check with `cargo +nightly clippy` but it doesn't > > > complain about this case. Should I switch to another version of clippy > > > when I do such check? (currently I'm using v0.1.63 clippy as well, to > > > match rustc.) > > > > > > > I don't remember exactly how I noticed it—I am pretty sure it broke in CI > > though. Maybe the change to add rust_version hid it. > > > > To answer your question, generally the idea is that we use the latest > > version of the developer tools (cargo, rustfmt, clippy). In particular > old > > versions of cargo don't support retrieving clippy settings from > Cargo.toml. > > This one inspired me. I'm thinking that even though we deleted the > README of pl011, a gneral guide or doc section (at somewhere, e.g., > docs/devel/style.rst or docs/devel/submitting-a-patch.rst) would be > helpful. > > At least, that could remind contributor to check patches with latest > toolchain instead of letting the maintainer's CI smoking fail. > I have sent a docs/devel/rust.rst sometime last week, it will be in the next pull request and then you can send a patch on top. Paolo Thanks, > Zhao > >
> I have sent a docs/devel/rust.rst sometime last week, it will be in the > next pull request and then you can send a patch on top. > Nice, I'll also have a look at that patch And I plan to post v2 tomorrow... as I'll leave at next week from Tuesday. Try to catch your pull request train if possible. :-) Thanks, Zhao
diff --git a/rust/qemu-api/src/irq.rs b/rust/qemu-api/src/irq.rs index 638545c3a649..7dca3b9ee5a8 100644 --- a/rust/qemu-api/src/irq.rs +++ b/rust/qemu-api/src/irq.rs @@ -5,12 +5,10 @@ //! Bindings for interrupt sources use core::ptr; -use std::{marker::PhantomData, os::raw::c_int}; +use std::{marker::PhantomData, os::raw::c_int, slice}; -use crate::{ - bindings::{qemu_set_irq, IRQState}, - prelude::*, -}; +pub(crate) use crate::bindings::IRQState; +use crate::{bindings::qemu_set_irq, prelude::*}; /// Interrupt sources are used by devices to pass changes to a value (typically /// a boolean). The interrupt sink is usually an interrupt controller or @@ -81,6 +79,16 @@ pub fn set(&self, level: T) { pub(crate) const fn as_ptr(&self) -> *mut *mut IRQState { self.cell.as_ptr() } + + #[allow(dead_code)] + pub(crate) fn as_slice_of_qemu_irq(slice: &[Self]) -> &[*mut IRQState] { + unsafe { + // SAFETY: InterruptSource has the same memory layout as *mut IRQState. + // Additionally, the slice parameter itself, as a reference to a slice type, + // can be converted into a valid pointer and has a valid length. + slice::from_raw_parts(slice.as_ptr().cast::<*mut IRQState>(), slice.len()) + } + } } impl Default for InterruptSource {
This is useful to hanlde InterruptSource slice and pass it to C bindings. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Zhao Liu <zhao1.liu@intel.com> --- Changes since RFC: * New commit. --- rust/qemu-api/src/irq.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)