On 5/20/2025 11:36 AM, Danilo Krummrich wrote:
>>> If you want a helper type with Options while parsing that's totally fine,
>>> but
>>> the final result can clearly be without Options. For instance:
>>>
>>> struct Data {
>>> image: KVec<u8>,
>>> }
>>>
>>> impl Data {
>>> fn new() -> Result<Self> {
>>> let parser = DataParser::new();
>>>
>>> Self { image: parser.parse()? }
>>> }
>>>
>>> fn load_image(&self) {
>>> ...
>>> }
>>> }
>>>
>>> struct DataParser {
>>> // Only some images have a checksum.
>>> checksum: Option<u64>,
>>> // Some images have an extra offset.
>>> offset: Option<u64>,
>>> // Some images need to be patched.
>>> patch: Option<KVec<u8>>,
>>> image: KVec<u8>,
>>> }
>>>
>>> impl DataParser {
>>> fn new() -> Self {
>>> Self {
>>> checksum: None,
>>> offset: None,
>>> patch: None,
>>> bytes: KVec::new(),
>>> }
>>> }
>>>
>>> fn parse(self) -> Result<KVec<u8>> {
>>> // Fetch all the required data.
>>> self.fetch_checksum()?;
>>> self.fetch_offset()?;
>>> self.fetch_patch()?;
>>> self.fetch_byes()?;
>>>
>>> // Doesn't do anything if `checksum == None`.
>>> self.validate_checksum()?;
>>>
>>> // Doesn't do anything if `offset == None`.
>>> self.apply_offset()?;
>>>
>>> // Doesn't do anything if `patch == None`.
>>> self.apply_patch()?;
>>>
>>> // Return the final image.
>>> self.image
>>> }
>>> }
>>>
>>> I think the pattern here is the same, but in this example you keep working
>>> with
>>> the DataParser, instead of a new instance of Data.
>> I think this would be a fundamental rewrite of the patch. I am Ok with
>> looking
>> into it as a future item, but right now I am not sure if it justifies not
>> using
>> Option for these few. There's a lot of immediate work we have to do for boot,
>> lets please not block the patch on just this if that's Ok with you. If you
>> want,
>> I could add a TODO here.
>
> Honestly, I don't think it'd be too bad to fix this up. It's "just" a bit of
> juggling fields and moving code around. The actual code should not change
> much.
>
> Having Option<T> where the corresponding value T isn't actually optional is
> extremely confusing and makes it hard for everyone, but especially new
> contributors, to understand the code and can easily trick people into taking
> wrong assumptions.
>
> Making the code reasonably accessible for (new) contributors is one of the
> objectives of nova and one of the learnings from nouveau.
I implemented the Data parsing pattern like the following, the final
FwSecBiosImage will not have optional fields as you suggested. It does get rid
of 2 additional fields as well which are not needed after vbios parsing
completes.
https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/commit/?h=nova/vbios&id=8cc852fe5573890596a91a2a935b3af24dcb9f04
Hope that looks Ok now! I am open to naming FwSecBiosPartial as FwSecBiosData if
that's better.
The full file after the change:
https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/tree/drivers/gpu/nova-core/vbios.rs?h=nova/vbios&id=8cc852fe5573890596a91a2a935b3af24dcb9f04
thanks,
- Joel
