Skip to content

lib-dice: Replace START / END with Range in cert templates. #761

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

flihp
Copy link
Contributor

@flihp flihp commented Sep 8, 2022

This simplifies the get / set functions in the Cert trait. This resolves #747

@flihp flihp added the lpc55 Has specific implications for LPC55 processors label Sep 12, 2022
where
Self: Sized,
T: TryFrom<&'a [u8]>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing inherent about TryFrom<&[u8]> that says it can only return Ok if the entire slice is used. It happens that you use a pattern of let x: [u8; X] = self.read_range(R) which means T is TryFrom<&'a [u8]> for [u8; N] which does fail if the slice and array lengths are different (I had to go look that up: https://doc.rust-lang.org/src/core/array/mod.rs.html#177-186). I'm not sure if that was an intentional choice here or whether T should always consume the entire slice provided. This feels like an easy place to make a mistake but I can't think of any easy way to detect such an error.

Copy link
Contributor Author

@flihp flihp Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern was intentional. It was the best option I could come up with that didn't rely on an unstable feature (generic_const_exprs I think but my understanding of this is limited). If you're OK with enabling incomplete / unstable features this can be updated, or we can wait for the feature to stabilize. I've created an issue to track this: #778


self
fn write_range<T: AsBytes>(&mut self, r: Range<usize>, t: &T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn write_range<T: AsBytes>(&mut self, r: Range<usize>, t: &T) {
fn set_range<T: AsBytes>(&mut self, r: Range<usize>, t: &T) -> Self {

Name change to align with functions that use it. Returning self to avoid all of the set_* methods from needing to explicitly return self themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do the same for 'read' vs 'get'

This simplifies the get / set functions in the Cert trait. This resolves oxidecomputer#747
@flihp flihp enabled auto-merge (squash) September 14, 2022 18:51
@flihp flihp merged commit 07bfc33 into oxidecomputer:master Sep 14, 2022
FawazTirmizi pushed a commit to rivosinc/hubris that referenced this pull request Oct 3, 2022
…puter#761)

This simplifies the get / set functions in the Cert trait. This resolves oxidecomputer#747
FawazTirmizi pushed a commit to rivosinc/hubris that referenced this pull request Oct 3, 2022
…puter#761)

This simplifies the get / set functions in the Cert trait. This resolves oxidecomputer#747
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lpc55 Has specific implications for LPC55 processors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use core::ops::Range<usize> in place of start / end / length values in DICE tmpls
3 participants