Skip to content

snapshot: support ip address literals, more docs - #403

Open
rcgoodfellow wants to merge 1 commit into
mainfrom
ry/snapshot-improvements
Open

rcgoodfellow wants to merge 1 commit into
mainfrom
ry/snapshot-improvements

Conversation

@rcgoodfellow

Copy link
Copy Markdown
Collaborator
  • Add more documentation on how to use the snapshot command.
  • Add support for specifying literal IP addresses as triggers since doing this in a hex/mask was very tedious.

@rcgoodfellow
rcgoodfellow force-pushed the ry/snapshot-improvements branch 6 times, most recently from e4c911f to 92462de Compare October 14, 2026 03:57
@rcgoodfellow
rcgoodfellow force-pushed the ry/snapshot-improvements branch from 92462de to b28e1ab Compare October 14, 2026 04:01

@cfzimmerman cfzimmerman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gut reaction is to ask for an expectorate test for every swadm change, but this one would be pretty tricky to test in CI, right? No pressure if so. Overall looks great.

Comment thread swadm/src/snapshot.rs
Comment on lines +41 to +55
fn from(value: Trigger) -> Self {
let (val, mask) = match value.value {
TriggerValue::Ip { ip } => match ip {
IpAddr::V4(v4) => (
format!("0x{:08x}", u32::from(v4)),
format!("0x{:x}", u32::MAX),
),
IpAddr::V6(v6) => (
format!("0x{:032x}", u128::from(v6)),
format!("0x{:x}", u128::MAX),
),
},
TriggerValue::Hex { value, mask } => (value, mask),
};
types::SnapshotTrigger { field: value.field, mask, value: val }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Might be more readable to un-nest the matches.

    fn from(value: Trigger) -> Self {
        let (val, mask) = match value.value {
            TriggerValue::Ip { ip: IpAddr::V4(v4) } => (
                format!("0x{:08x}", u32::from(v4)),
                format!("0x{:x}", u32::MAX),
            ),
            TriggerValue::Ip { ip: IpAddr::V6(v6) } => (
                format!("0x{:032x}", u128::from(v6)),
                format!("0x{:x}", u128::MAX),
            ),
            TriggerValue::Hex { value, mask } => (value, mask),
        };
        types::SnapshotTrigger { field: value.field, mask, value: val }
    }

Comment thread swadm/src/snapshot.rs

/// Parse a trigger spec: "field=value/mask" where value and mask are hex.
fn parse_trigger(s: &str) -> Result<types::SnapshotTrigger> {
fn parse_trigger(s: &str) -> Result<Trigger> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth a unit test?

Comment thread swadm/src/snapshot.rs
Comment on lines +112 to +113
/// This flag may be specified multiple times to specify multiple
/// triggers. Multiple triggers are joined together as a logical AND.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we say "multiple unique triggers" or even enforce uniqueness? For example, I think

-t hdr.ethernet.dst_addr=A -t hdr.ethernet.dst_addr=B

only captures "B". Does that sound right to you? Might be confusing if left unstated. This is based on reading tofino sde code, so its very possible I'm incorrect.

https://github.com/oxidecomputer/tofino-sde/blob/9671e58c051b342d2031a005226caf00d951dcc5/pkgsrc/bf-drivers/src/pipe_mgr/pipe_mgr_mau_snapshot.c#L4186-L4196

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.

2 participants