Re: [PATCH v2 2/2] rust: impl_flags: add bitwise operations with the underlying type

[Andreas Hindborg <a.hindborg@kernel.org>]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Fri, Jun 5, 2026 at 3:04 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Add bitwise or operations between the flag value enum and the underlying
>> type. This is useful when manipulating flags from C API without round
>> tripping into the Rust flag container type:
>>
>>   let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>
> Can this use `zeroable` nowadays?

Yes, probably.

>
>>   if self.write_cache {
>>       lim.features |= request::Feature::WriteCache;
>>   }
>>
>> The above code would be needlessly verbose without this direct assignment
>> option.
>
> v2 looks better overall, but I am wondering about Gary's question in
> v1 -- not sure if you saw it:
>
>   https://lore.kernel.org/rust-for-linux/DGPT710WN25X.1B9P21BE6X8P4@garyguo.net/
>
> The commit message shows a converted case, but is it that much
> verbose? Perhaps you can point to the use case / code that made you
> think about adding these `impl`s?

I feel like we already discussed this. I don't understand the objection.
I have a C type integer that is a set of flags. In bindings, I
manipulate the flags (I will send this code within the next few days):

    /// Build a new `GenDisk` and add it to the VFS.
    pub fn build(
        self,
        name: fmt::Arguments<'_>,
        tagset: Arc<TagSet<T>>,
        queue_data: T::QueueData,
    ) -> Result<Arc<GenDisk<T>>> {
        let data = queue_data.into_foreign();
        let recover_data = ScopeGuard::new(|| {
            // SAFETY: T::QueueData was created by the call to `into_foreign()` above
            drop(unsafe { T::QueueData::from_foreign(data) });
        });

        let mut lim: bindings::queue_limits = pin_init::zeroed();

        lim.logical_block_size = self.logical_block_size;
        lim.physical_block_size = self.physical_block_size;
        lim.max_hw_discard_sectors = self.max_hw_discard_sectors;
        lim.max_sectors = self.max_sectors;
        lim.virt_boundary_mask = self.virt_boundary_mask;
        if self.rotational {
            lim.features = Feature::Rotational.into();
        }

        #[cfg(CONFIG_BLK_DEV_ZONED)]
        if self.zoned {
            if !T::HAS_REPORT_ZONES {
                return Err(error::code::EINVAL);
            }

            lim.features |= Feature::Zoned;
            lim.chunk_sectors = self.zone_size_sectors;
            lim.max_hw_zone_append_sectors = self.zone_append_max_sectors;
        }

        if self.write_cache {
            lim.features |= Feature::WriteCache;
        }

        if self.forced_unit_access {
            lim.features |= Feature::ForcedUnitAccess;
        }

        // SAFETY: `tagset.raw_tag_set()` points to a valid and initialized tag set
        let gendisk = from_err_ptr(unsafe {
            bindings::__blk_mq_alloc_disk(
                tagset.raw_tag_set(),
                &mut lim,
                data,
                static_lock_class!().as_ptr(),
            )
        })?;

        ...

I don't understand the rationale for writing these flags into a
temporary to then assigning later.


Best regards,
Andreas Hindborg