Re: [PATCH 2/2] rust: block/mq: replace mem::zeroed() with Zeroable trait

[Paolo Bonzini]

On 11/29/24 10:41, Alice Ryhl wrote:
> On Thu, Nov 28, 2024 at 3:13 PM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> > Isolate the unsafety in the declaration of the Zeroable trait, instead of having
> > to use "unsafe" just to declare a struct.  This is more similar to how you would
> > use "..Default::default()" (which is also a possibility here, but arguably
> > less efficient).
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> >   rust/kernel/block/mq/gen_disk.rs |  8 +++++---
> >   rust/kernel/block/mq/tag_set.rs  | 10 ++++++----
> >   2 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> > index 708125dce96a..65342d065296 100644
> > --- a/rust/kernel/block/mq/gen_disk.rs
> > +++ b/rust/kernel/block/mq/gen_disk.rs
> > @@ -6,7 +6,7 @@
> >   //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
> >
> >   use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
> > -use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
> > +use crate::{bindings, error::from_err_ptr, error::Result, init::Zeroable, sync::Arc};
> >   use crate::{error, static_lock_class};
> >   use core::fmt::{self, Write};
> >
> > @@ -31,6 +31,9 @@ fn default() -> Self {
> >       }
> >   }
> >
> > +// SAFETY: `bindings::queue_limits` contains only fields that are valid when zeroed.
> > +unsafe impl Zeroable for bindings::queue_limits {}
> > +
> >   impl GenDiskBuilder {
> >       /// Create a new instance.
> >       pub fn new() -> Self {
> > @@ -93,8 +96,7 @@ pub fn build<T: Operations>(
> >           name: fmt::Arguments<'_>,
> >           tagset: Arc<TagSet<T>>,
> >       ) -> Result<GenDisk<T>> {
> > -        // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
> > -        let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
> > +        let mut lim: bindings::queue_limits = Zeroable::ZERO;
> >
> >           lim.logical_block_size = self.logical_block_size;
> >           lim.physical_block_size = self.physical_block_size;
> > diff --git a/rust/kernel/block/mq/tag_set.rs b/rust/kernel/block/mq/tag_set.rs
> > index f9a1ca655a35..1ff7366ca549 100644
> > --- a/rust/kernel/block/mq/tag_set.rs
> > +++ b/rust/kernel/block/mq/tag_set.rs
> > @@ -10,6 +10,7 @@
> >       bindings,
> >       block::mq::{operations::OperationsVTable, request::RequestDataWrapper, Operations},
> >       error,
> > +    init::Zeroable,
> >       prelude::PinInit,
> >       try_pin_init,
> >       types::Opaque,
> > @@ -32,6 +33,10 @@ pub struct TagSet<T: Operations> {
> >       _p: PhantomData<T>,
> >   }
> >
> > +// SAFETY: `blk_mq_tag_set` only contains integers and pointers, which
> > +// all are allowed to be 0.
> > +unsafe impl Zeroable for bindings::blk_mq_tag_set {}
>
> This will have to be reverted if we want to split up the kernel crate
> due to the orphan rule.

Ok, I will look into using the bindgen doc comments then.  It looks like 
the idea is not dead on arrival at least!  Thanks for the review.

Paolo