Skip to content
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

attest-data messages #220

Merged
merged 2 commits into from
Aug 26, 2024
Merged

attest-data messages #220

merged 2 commits into from
Aug 26, 2024

Conversation

labbott
Copy link
Contributor

@labbott labbott commented Jul 24, 2024

No description provided.

@labbott labbott marked this pull request as draft July 24, 2024 18:20
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has checked 53 files.

Valid Invalid Ignored Fixed
13 1 39 0
Click to see the invalid file list
  • attest-data/src/messages.rs
Use this command to fix any missing license headers
```bash

docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix

</details>

attest-data/src/messages.rs Show resolved Hide resolved
@labbott labbott force-pushed the attest_messages branch 3 times, most recently from ec7a737 to 37d6ef5 Compare July 29, 2024 18:12
@labbott labbott marked this pull request as ready for review July 29, 2024 18:16
@labbott labbott changed the title messages WIP attest-data messages Jul 29, 2024
Copy link
Collaborator

@flihp flihp left a comment

Choose a reason for hiding this comment

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

I'm in transit for a bit but will try to get this reviewed ASAP

let header_len = hubpack::serialize(out, header)?;
let mut n = header_len;

let out_data_end = out.len();
Copy link

Choose a reason for hiding this comment

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

Nit: you can do a one-sided range out[n..] instead of out[n..out_data_end]

/// Unexpected command returned
UnexpectedCommand,
/// Error return from the sprot command
SprotError(SprotError),
Copy link

Choose a reason for hiding this comment

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

Design question: because we're already supporting nested errors, should we have an error hierarchy here that matches the one in Hubris (variants for Protocol, Spi, Attest, etc) instead of putting them all in SprotError(..)?

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 went back and forth on that. We really only care about errors from Protocol Spi and Attest so it didn't seem that helpful to have a hierarchy vs. a flat error structure.

@labbott labbott force-pushed the attest_messages branch 2 times, most recently from 60638a6 to 1df0cdb Compare August 26, 2024 13:17
These are designed to be sent over IPCC (communication channel
between Host OS and SP)
@labbott labbott merged commit 15c0270 into main Aug 26, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants