draft PR for commenting on snitch implementation #1916
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 543 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 542 | 1 | 0 | 0 |
Click to see the invalid file list
- lib/snitch-core/src/lib.rs
| @@ -0,0 +1,781 @@ | |||
| //! Portable core data structures for the `snitch` error reporting task. | |||
There was a problem hiding this comment.
| //! Portable core data structures for the `snitch` error reporting task. | |
| // This Source Code Form is subject to the terms of the Mozilla Public | |
| // License, v. 2.0. If a copy of the MPL was not distributed with this | |
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | |
| //! Portable core data structures for the `snitch` error reporting task. |
There was a problem hiding this comment.
Overall, this is lovely, although --- as we discussed out of band --- I think the ereport ingestion protocol will be easier to implement if there's a cursor-style interface for reads, so that you can reply to a "give me everything you've got" message and then handle a single ack for every ereport you replied with. Essentially exposing slightly more of the ringbuffer guts to the protocol.
This implementation is lovely though. I picked a few inconsequential nits.
| if let Some(n) = lost_count { | ||
| // Set to `None` on overflow. | ||
| *lost_count = n.checked_add(1); | ||
| } else { | ||
| // Count is already saturated and will remain so. | ||
| } |
There was a problem hiding this comment.
this use of NonZero is very cute :)
| /// All data has been written to the queue. The writer should attempt to | ||
| /// write new data. | ||
| Writing, | ||
| /// }Data has been lost. The writer should attempt to recover but may wind up |
There was a problem hiding this comment.
typo
| /// }Data has been lost. The writer should attempt to recover but may wind up | |
| /// Data has been lost. The writer should attempt to recover but may wind up |
| enum WriterState { | ||
| /// All data has been written to the queue. The writer should attempt to | ||
| /// write new data. | ||
| Writing, |
There was a problem hiding this comment.
extremely unimportant nitpick: i feel a bit iffy about this naming; "writing" suggests to me that a write is currently in progress, which is not actually what this means. "writable" maybe? but the "...ing" ending is consistent with "losing", so it does kinda make sense.
| let lo = self.contents[(self.next_read + 1) % N]; | ||
| let hi = self.contents[(self.next_read + 2) % N]; | ||
| let len = usize::from(u16::from_le_bytes([lo, hi])); |
There was a problem hiding this comment.
this bit threw me briefly before i realized that it's because the length word itself may have been split if it wrapped around the buffer, not just the actual message region. which makes sense, it just took me a minute. maybe worth a comment?
| }) | ||
| } | ||
| _uhhhh => { | ||
| // Well, great, the queue is corrupt. We indicate this condition |
There was a problem hiding this comment.
"corrupt" here means that the next read pointer is pointed at a byte which is neither the start of a message nor a loss record, indicating that we have somehow ended up in the midst of a message frame, or someone has written over where a message/loss record is supposed to start?
|
|
||
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] | ||
| pub enum Record<'a> { | ||
| Valid(&'a [u8], &'a [u8]), |
There was a problem hiding this comment.
it seems like it would be nice if the valid record case provided a function for doing..."whatever the opposite of copy2 is"1 (i.e. letting you memcpy the possibly-split buffer into a normal contiguous slice you have lying around)? presumably we can just add that whenever and you haven't really started doing this kind of API polish yet?
Footnotes
-
calling it "uncopy2" would be a terrible idea but also makes me giggle ↩
|
|
||
| const MESSAGE: u8 = 1; | ||
| const LOSS: u8 = 0xFF; | ||
| const LOSS_LEN: usize = 5; |
There was a problem hiding this comment.
teensy nit: it seems like there could be a corresponding MESSAGE_LEN: usize = 3 rather than just saying 3 everywhere?
| fn copy2(src: &[u8], dest0: &mut [u8], dest1: &mut [u8]) { | ||
| assert_eq!(src.len(), dest0.len() + dest1.len()); | ||
| dest0.copy_from_slice(&src[..dest0.len()]); | ||
| dest1.copy_from_slice(&src[dest0.len()..]); | ||
| } |
There was a problem hiding this comment.
it seems like there could be a method on WriteReservation that you can just pass a slice to and have it copy2 on your behalf? so that code calling into this API doesn't have to rewrite this
No description provided.