-
Notifications
You must be signed in to change notification settings - Fork 62
[mgs] API for ingesting ereports from SPs #7903
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
Changes from 1 commit
ba42796
cb673ec
8d66c09
df3a493
731966a
e324481
4cf9246
49973ae
20863c6
f941a08
d1cd98b
a45ff03
ff56b84
e69fc91
f250ded
7d53fdb
d1738a7
7cc88ea
d481e21
619ccf4
914dcd4
539b0ce
46c3612
a7905e0
02b65b2
64fd8f3
24b009b
fea3438
69a6c0a
4dd8878
7ebf1d6
7ea4ef3
9af73b6
0ff4977
b71609c
f245a3f
286bf61
542ea63
645c4de
8eccf7b
451a12e
dd179e4
19b7cd0
178dcef
2d09c6f
9cb27a8
aaa3c37
2ecfb47
fe5d3ca
4bf8937
04d967e
368f84f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,15 +111,6 @@ impl EreportState { | |
| let EreportRequest::V0(req) = request; | ||
| slog::info!(self.log, "ereport request: {req:?}"); | ||
|
|
||
| let mut pos = gateway_messages::serialize( | ||
| buf, | ||
| &EreportResponseHeader::V0(ResponseHeaderV0 { | ||
| request_id: req.request_id, | ||
| restart_id: self.restart_id, | ||
| }), | ||
| ) | ||
| .expect("header must serialize"); | ||
|
|
||
| // If we "restarted", encode the current metadata map, and start at ENA | ||
| // 0. | ||
| let (meta_map, start_ena) = if req.restart_id != self.restart_id { | ||
|
|
@@ -158,6 +149,30 @@ impl EreportState { | |
|
|
||
| (&Default::default(), req.start_ena) | ||
| }; | ||
|
|
||
| let mut respondant_ereports = self | ||
| .ereports | ||
| .iter() | ||
| .filter(|ereport| ereport.ena >= start_ena) | ||
| .take(req.limit as usize) | ||
| .peekable(); | ||
| let start_ena = respondant_ereports | ||
| .peek() | ||
| .map(|ereport| ereport.ena) | ||
| .unwrap_or(Ena(start_ena.0 + 1)); | ||
hawkw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Serialize the header. | ||
| let mut pos = gateway_messages::serialize( | ||
| buf, | ||
| &EreportResponseHeader::V0(ResponseHeaderV0 { | ||
| request_id: req.request_id, | ||
| restart_id: self.restart_id, | ||
| start_ena, | ||
| }), | ||
| ) | ||
| .expect("header must serialize"); | ||
|
|
||
| // Serialize the metadata map. | ||
| pos += { | ||
| use serde::ser::SerializeMap; | ||
|
|
||
|
|
@@ -179,59 +194,36 @@ impl EreportState { | |
| cursor.position() as usize | ||
| }; | ||
|
|
||
| // Is there enough remaining space for ereports? We need at least 10 | ||
| // bytes (8 for the ENA, and at least two bytes to encode an empty CBOR | ||
| // list) | ||
| if buf[pos..].len() < 10 { | ||
| // Is there enough remaining space for ereports? We need at least two | ||
| // bytes to encode an empty CBOR list | ||
| if buf[pos..].len() <= 2 { | ||
| return &buf[..pos]; | ||
| } | ||
|
|
||
| let mut respondant_ereports = self | ||
| .ereports | ||
| .iter() | ||
| .filter(|ereport| ereport.ena >= start_ena) | ||
| .take(req.limit as usize); | ||
| if let Some(EreportListEntry { ena, ereport, bytes }) = | ||
| respondant_ereports.next() | ||
| { | ||
| pos += gateway_messages::serialize(&mut buf[pos..], ena) | ||
| .expect("serialing ena shouldn't fail"); | ||
| buf[pos] = 0x9f; // start list | ||
| pos += 1; | ||
| buf[pos] = 0x9f; // start list | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't need to block this PR, but: are there any cbor crates we could use here (we're already using
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, the reason that this part is a bit weird is because we want a particular behavior: we would like to put as many ereports as possible into the packet until the packet is full, and not write any bytes for any ereports that don't fit in the packet. This is why we build the list manually: just trying to use Unfortunately, none of the CBOR crates I looked at seemed to have constants for these bytes that were publicly exposed; per the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth changing this code and the MGS code to use |
||
| pos += 1; | ||
|
|
||
| // try to fill the rest of the packet with ereports | ||
| for EreportListEntry { ena, ereport, bytes } in respondant_ereports { | ||
| // packet full! | ||
| if buf[pos..].len() < (bytes.len() + 1) { | ||
| break; | ||
| } | ||
|
|
||
| buf[pos..pos + bytes.len()].copy_from_slice(&bytes[..]); | ||
| pos += bytes.len(); | ||
| slog::debug!( | ||
| self.log, | ||
| "wrote initial ereport: {ereport:#?}"; | ||
| "wrote ereport: {ereport:#?}"; | ||
| "ena" => ?ena, | ||
| "packet_bytes" => pos, | ||
| "ereport_bytes" => bytes.len(), | ||
| ); | ||
|
|
||
| // try to fill the rest of the packet | ||
| for EreportListEntry { ena, ereport, bytes } in respondant_ereports | ||
| { | ||
| // packet full! | ||
| if buf[pos..].len() < (bytes.len() + 1) { | ||
| break; | ||
| } | ||
|
|
||
| buf[pos..pos + bytes.len()].copy_from_slice(&bytes[..]); | ||
| pos += bytes.len(); | ||
| slog::debug!( | ||
| self.log, | ||
| "wrote subsequent ereport: {ereport:#?}"; | ||
| "ena" => ?ena, | ||
| "packet_bytes" => pos, | ||
| "ereport_bytes" => bytes.len(), | ||
| ); | ||
| } | ||
|
|
||
| buf[pos] = 0xff; // break list; | ||
| pos += 1; | ||
| } | ||
|
|
||
| buf[pos] = 0xff; // break list; | ||
| pos += 1; | ||
|
|
||
| &buf[..pos] | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less important on this commit than the upcoming one, but we should probably do what this comment says and update
package-manifest.tomltoo.