-
Notifications
You must be signed in to change notification settings - Fork 207
Description
When a task delivers an ereport to packrat, the CBOR is encoded into a buffer in the reporting task which is then leased to Packrat. Since Hubris doesn't use dynamic allocation, these buffers must be fixed-size. This means that there is a possibility for an ereport to be too big to ever fit in the buffer size that was chosen by the programmer. That would be bad.
Presently, I've been dealing with this by making code that produces an ereport record whether or not it fit in the buffer in a ringbuf, along with the encoded length. For example:
hubris/drv/gimlet-seq-server/src/vcore.rs
Lines 278 to 292 in 43e13b9
match self | |
.packrat | |
.serialize_ereport(&ereport, &mut ereport_buf[..]) | |
{ | |
Ok(len) => ringbuf_entry!(Trace::EreportSent(len)), | |
Err(task_packrat_api::EreportSerializeError::Packrat { | |
len, | |
err, | |
}) => { | |
ringbuf_entry!(Trace::EreportLost(len, err)) | |
} | |
Err(task_packrat_api::EreportSerializeError::Serialize(_)) => { | |
ringbuf_entry!(Trace::EreportTooBig) | |
} | |
} |
Then, I'll trigger the condition that produces the ereport and check that it was recorded successfully, and adjust the size of the buffer if it's too big. This "sort of works" for now, but it's not an approach that will scale as we add more ereports to things, and as we add more data to existing ereports. And, it only works when the condition that generates an ereport is something we can reliably and safely reproduce in the lab, which may not always be the case.1 It would be much better to have a way to ensure that these buffers are right-sized at build time.
For hubpack-encoded messages, this would be easy. Since Hubpack doesn't support variable-length fields, it has a #[derive(SerializedSize)]
that can statically determine the maximum size of a message, and then the buffer could just be sized to whatever the max size of all the messages it will be used for. This doesn't work for ereports, which are encoded in CBOR,1 a format that permits variable-size fields.
Looking at the current ereport messages, though, the only variable-size fields are &'static str
s, for the ereport class string and stuff like a component's refdes or a power rail. For example:
hubris/drv/gimlet-seq-server/src/vcore.rs
Lines 269 to 277 in 43e13b9
let ereport = Ereport { | |
k: "hw.pwr.pmbus.alert", | |
v: 0, | |
refdes: self.device.i2c_device().component_id(), | |
rail: "VDD_VCORE", | |
time: now, | |
pwr_good, | |
pmbus_status: status, | |
}; |
Generally there is a relatively small set of such values that we actually use, rather than dynamically formatted arbitrary strings, but we don't currently have a way of showing that at compile-time. I don't think some kind of proc-macro for determining this at compile-time will be easy, though, at least without some way to say "this is actually coming from a fixed set of strings" (perhaps an enum with some more attributes, like
serde(rename = "...")
?).
Alternatively, we could do this using unit tests that just construct the ereports and assert their serialized form fits in the buffer. This would be the easy, pragmatic approach...except for one wrinkle: today, the structs defining ereport messages are defined in task code. We can't easily include tests that are intended to run on a development machine in a task crate, since the task code usually doesn't compile at all for Big Computer targets like x86_64-unknown-linux-gnu
or aarch64-apple-darwin
. We could work around that by having separate crate(s) for the ereport messages that can compile for #![no_std]
MCU targets but also run tests on dev machine targets, but that's kind of a pain. Perhaps there's some build system hack that could make this easier...
I would love other suggestions if anyone can think of something I'm overlooking here!