On Wed Oct 7, 2026 at 1:51 PM JST, Alexandre Courbot wrote:
>> +#![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
>> +
>> +use kernel::prelude::*;
>> +
>> +use crate::{
>> + gsp::nvkv::{
>> + Index,
>> + KeyId,
>> + Op,
>> + Opcode, //
>> + },
>> + num, //
>> +};
>> +
>> +/// A decoded NVKV value.
>> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
>> +pub(crate) enum DecoderValue<'a> {
>> + Scalar32(u32),
>> + Scalar64(u64),
>> + Array8(&'a [u8]),
>> + Array32(&'a [u32]),
>> + Array64(&'a [u64]),
>> +}
>> +
>> +/// Implements `TryFrom` from the given `DecoderValue` variant to the given
>> type.
>> +///
>> +/// `TryFrom` is used by the `Schema` implementations in this file to
>> convert from the
>> +/// `DecoderValue`s into the types to store. Provide the implementations
>> for basic types here.
>
> nit: proper doclinks for referenced types (recurring problem throughout
> the file).
Fixed (here and other locations)
>
>> +macro_rules! impl_try_from_decoder_value {
>> + ($ty:ty, $variant:ident) => {
>> + impl<'a> TryFrom<DecoderValue<'a>> for $ty {
>> + type Error = Error;
>> +
>> + fn try_from(value: DecoderValue<'a>) -> Result<Self> {
>> + if let DecoderValue::$variant(v) = value {
>> + Ok(v)
>> + } else {
>> + Err(EINVAL)
>> + }
>> + }
>> + }
>> + };
>> +}
>> +
>> +impl_try_from_decoder_value!(u32, Scalar32);
>> +impl_try_from_decoder_value!(u64, Scalar64);
>> +impl_try_from_decoder_value!(&'a [u8], Array8);
>> +impl_try_from_decoder_value!(&'a [u32], Array32);
>> +impl_try_from_decoder_value!(&'a [u64], Array64);
>> +
>> +/// A visitor that consumes decoded NVKV and produces a `Target`.
>> +pub(crate) trait Schema {
>> + type Target;
>> +
>> + /// Returns an initializer that creates an empty schema in place.
>> + ///
>> + /// Use [`KBox::init`] for the heap or `stack_pin_init!` for the stack
>> (if sure that the value
>> + /// is small enough to fit).
>> + fn init() -> impl Init<Self>
>> + where
>> + Self: Sized;
>> +
>> + /// Returns an initializer that makes the decoded `Target`.
>> + ///
>> + /// After the returned initializer runs, the schema should be empty
>> again.
>> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_;
>> +}
>> +
>> +/// A visitor that consumes decoded NVKV from a stream.
>
> Note that `Schema` is also defined as a "visitor that consumes decoded
> NVKV", which makes things a bit confusing. I guess that's because both
> traits implement different aspects of what a typical visitor does, but
> clarifying their relationship (and the reason for splitting) would be
> helpful.
Right, in order to avoid noisy types when you use a Schema type in a
struct, I split the trait here. So for Schemas that don't need it, you
can avoid having to put a lifetime on the struct that you plumb to the
scehma. Otherwise you'd be writing struct SomeSchema<'stream>
every time.
Let me add that info.
>
>> +///
>> +/// A schema that doesn't need to borrow data from the stream can implement
>> this for all `'data`
>> +/// lifetimes, avoiding having to carry the lifetime parameter. A schema
>> that borrows from the
>> +/// stream directly should implements it for its own lifetime only.
>> +pub(crate) trait Visit<'data> {
>
> Should this be named `Visitor` instead of `Visit`? Trait names should be
> nouns rather than verbs, and this would make the `Decoder::visit`
> method's prototype read more clearly imho. If you intended to use
> `Visit` as a noun, I don't think that works either since the `visit`
> method is potentially called many times (so it is not a single visit).
According to [1], traits should preferably be named as verbs, and it in
particular calls out the case where there's one fn in the trait:
```
This RFC proposes to amend the convention to further say: if there
is a single method that is the dominant functionality of the trait,
consider using the same name for the trait itself.
```
That said in the kernel there are more noun/noun phrase traits. So no
worries to rename it to Visitor. But I wouldn't say trait names should
be nouns as a rule, it seems that the prescriptive answer is to prefer
the opposite.
[1]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#trait-naming
>
>> + /// Visits one decoded pair. Returns `Ok(true)` if the schema consumed
>> it.
>> + fn visit(&mut self, key: KeyId, index: Index, value:
>> DecoderValue<'data>) -> Result<bool>;
>> +}
>> +
>> +/// A read position in an NVKV stream.
>> +struct Cursor<'a> {
>> + data: &'a [u64],
>> +}
>> +
>> +impl<'a> Cursor<'a> {
>> + /// Creates a cursor at the start of `data`.
>> + fn new(data: &'a [u64]) -> Self {
>> + Self { data }
>> + }
>> +
>> + /// Returns `true` if no `u64` values remain.
>> + fn is_empty(&self) -> bool {
>> + self.data.is_empty()
>> + }
>> +
>> + /// Takes the next `u64`.
>> + fn take_u64(&mut self) -> Result<u64> {
>> + // PANIC: `take_u64s(1)` returns exactly one element on success.
>> + Ok(self.take_u64s(1)?[0])
>> + }
>> +
>> + /// Takes `count` bytes. If `count` is not a multiple of 8 (`u64`
>> size), bytes are discarded up
>> + /// to the next multiple.
>> + fn take_u8s(&mut self, count: usize) -> Result<&'a [u8]> {
>> + let values = self.take_u64s(count.div_ceil(8))?;
>
> s/8/size_of::<u64>()
>
>> + values.as_bytes().get(..count).ok_or(EINVAL)
>> + }
>> +
>> + /// Takes `count` 32-bit values. If `count` is not a multiple of 2
>> (`u64` size), bytes are
>> + /// discarded up to the next multiple.
>> + fn take_u32s(&mut self, count: usize) -> Result<&'a [u32]> {
>> + let values = self.take_u64s(count.div_ceil(2))?;
>
> ... and maybe `size_of::<u64>() / size_of::<u32>()` here?
Done
[...] (applied review comments)
>> + // Tests that basic decoding into a manually implemented `Schema` works
>> correctly.
>> + #[test]
>> + fn decode_raw_schema() -> Result {
>> + // Decodes an IMM32 pair and a SEQ64 pair (the encoder emits a u64
>> as a single-element
>> + // SEQ64) with a hand written `Schema`. Keys and value constants
>> chosen to distinguish e.g.
>> + // saving the wrong value to the wrong location.
>> + const SCALAR32_KEY: KeyId = 0x1001;
>> + const SCALAR64_KEY: KeyId = 0x1002;
>> + const UNKNOWN_KEY: KeyId = 0x2001;
>> +
>> + const SCALAR32_VALUE: u32 = 0x1111_2222;
>> + const SCALAR64_VALUE: u64 = 0x3333_4444_5555_6666;
>> +
>> + // The output type of the hand written `Schema`. In this case, we
>> can have it also implement
>> + // `Schema` on itself rather than having a separate carrier type,
>> since the `Schema`
>> + // implementation is completely stateless.
>> + #[derive(Default)]
>> + struct RawSchema {
>> + scalar32: u32,
>> + scalar64: u64,
>> + }
>> +
>> + impl Schema for RawSchema {
>> + type Target = Self;
>> +
>> + fn init() -> impl Init<Self> {
>> + Self::default()
>> + }
>> +
>> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>> + Ok(core::mem::take(self))
>> + }
>> + }
>> +
>> + impl<'d> Visit<'d> for RawSchema {
>> + fn visit(&mut self, key: KeyId, index: Index, value:
>> DecoderValue<'d>) -> Result<bool> {
>> + if index != Index::new::<0>() {
>> + return Err(EINVAL);
>> + }
>> + match key {
>> + SCALAR32_KEY => self.scalar32 = value.try_into()?,
>> + SCALAR64_KEY => self.scalar64 = value.try_into()?,
>> + _ => return Ok(false),
>> + }
>> + Ok(true)
>> + }
>> + }
>> +
>> + let mut encoder = Encoder::new();
>> + encoder.encode_u32(SCALAR32_KEY, Index::new::<0>(),
>> SCALAR32_VALUE)?;
>> + encoder.encode_u64(SCALAR64_KEY, Index::new::<0>(),
>> SCALAR64_VALUE)?;
>> + let serialized = encoder.finish();
>> +
>> + let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
>> + let mut schema = KBox::init(RawSchema::init(), GFP_KERNEL)?;
>> + let decoded = KBox::try_init(decoder.decode(&mut *schema)?,
>> GFP_KERNEL)?;
>> +
>> + assert_eq!(decoded.scalar32, SCALAR32_VALUE);
>> + assert_eq!(decoded.scalar64, SCALAR64_VALUE);
>
> Nice test!
thanks~~
>
>> +
>> + // An unknown key should fail with under `UnknownKeyPolicy::Error`
>> and be skipped under
>> + // `UnknownKeyPolicy::Ignore`.
>> + let mut encoder = Encoder::new();
>> + encoder.encode_u32(UNKNOWN_KEY, Index::new::<0>(), 1)?;
>> +
>> + let serialized = encoder.finish();
>> + let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
>> + let mut schema = KBox::init(RawSchema::init(), GFP_KERNEL)?;
>> + assert!(decoder.decode(&mut *schema).is_err());
>> +
>> + let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Ignore);
>> + let mut schema = KBox::init(RawSchema::init(), GFP_KERNEL)?;
>> + let decoded = KBox::try_init(decoder.decode(&mut *schema)?,
>> GFP_KERNEL)?;
>> + assert_eq!(decoded.scalar32, 0);
>
> ... this part looks like it should be its own test though. That's
> trivial to achieve if you declare `RawSchema` at the module-level
> instead of inside the method.
>
> (also nit: let's assert `scalar64` as well, or none of the values at all
> - I'd lean towards none since that part is already covered by the
> regular decoding test).
Done
>
>> +
>> + Ok(())
>> + }
>> +
>> + /// Records each visit as (key, index, value), for tests on hand-built
>> streams.
>
> nit: `(key, index, value)`
>
>> + #[derive(Default)]
>> + struct Recorder<'d> {
>> + visits: KVVec<(KeyId, u64, DecoderValue<'d>)>,
>> + }
>> +
>> + impl<'d> Schema for Recorder<'d> {
>> + type Target = KVVec<(KeyId, u64, DecoderValue<'d>)>;
>> +
>> + fn init() -> impl Init<Self> {
>> + Self::default()
>> + }
>> +
>> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>> + Ok(core::mem::take(&mut self.visits))
>> + }
>> + }
>> +
>> + impl<'d> Visit<'d> for Recorder<'d> {
>> + fn visit(&mut self, key: KeyId, index: Index, value:
>> DecoderValue<'d>) -> Result<bool> {
>> + self.visits.push((key, index.get(), value), GFP_KERNEL)?;
>> + Ok(true)
>> + }
>> + }
>> +
>> + // Tests the decoder on hand-built `u64` values that the encoder does
>> not produce: SEQ32,
>> + // multi-value SEQ64, zero counts, a non-zero index and padded arrays.
>> + #[test]
>> + fn decode_raw_u64s() -> Result {
>> + const SEQ32_KEY: KeyId = 0x2000;
>> + const SEQ64_KEY: KeyId = 0x2010;
>> + const EMPTY_SEQ64_KEY: KeyId = 0x2020;
>> + const EMPTY_SEQ32_KEY: KeyId = 0x2021;
>> + const EMPTY_ARRAY8_KEY: KeyId = 0x2030;
>> + const EMPTY_ARRAY32_KEY: KeyId = 0x2031;
>> + const EMPTY_ARRAY64_KEY: KeyId = 0x2032;
>> + const ARRAY8_KEY: KeyId = 0x2040;
>> + const ARRAY32_KEY: KeyId = 0x2041;
>> +
>> + let index3 = Index::new::<3>();
>
> nit: variable only used once, can be created inline.
Done
>
> I really like how testing coverage has improved in this revision!
thanks for your suggestion~!
