Skip to content

min_shark: add first-class eth.type ethertype clause AX-98261 - #7

Merged
talvaknin1 merged 13 commits into
dovreshef:mainfrom
talvaknin1:AX-98261-add-ethertype-filter
Sep 7, 2026
Merged

talvaknin1 merged 13 commits into
dovreshef:mainfrom
talvaknin1:AX-98261-add-ethertype-filter

Conversation

@talvaknin1

Copy link
Copy Markdown
Collaborator

Adds eth.type == 0x88a4 / in {...} filtering (hex or decimal), mirroring vlan.id. Needed to filter OT protocols like EtherCAT that are identified by ethertype rather than an IP/port. AX-98261.

Adds `eth.type == 0x88a4` / `in {...}` filtering (hex or decimal), mirroring
vlan.id. Needed to filter OT protocols like EtherCAT that are identified by
ethertype rather than an IP/port. AX-98261.
@talvaknin1 talvaknin1 changed the title min_shark: add first-class eth.type ethertype clause min_shark: add first-class eth.type ethertype clause AX-98261 Aug 17, 2026
Manual reformatting in the previous commit fought the pinned nightly
rustfmt and broke `cargo fmt --check`.
@dovreshef

Copy link
Copy Markdown
Owner

Hi!

Please fix the clippy / fmt issues.

In addition some comments:

Values larger than an EtherType are accepted

src/parser.rs:538-541 uses parse_u32_hex_or_dec, which accepts values through u32::MAX. But Matcher::eth_type only accepts u16 at src/expression.rs:710-713.

Consequences:

  eth.type == 0x10000   # parses, can never match
  eth.type < 0x10000    # parses, matches every possible supplied EtherType

I confirmed both 65536 and 0x10000 parse successfully.

The smallest fix is an EtherType parser that accepts decimal/hex but rejects values above u16::MAX, widening valid results into the existing ValOp representation.

Tests should cover:

  • 0
  • 0xffff succeeds
  • 65536 and 0x10000 fail
  • an out-of-range value inside a list fails

No public-path integration test

Current tests separately verify:

  • lexing: src/lexer.rs
  • AST construction: src/parser.rs:859-911
  • manually constructed matching: src/expression.rs:1049-1082

But none exercises the actual user flow:

  parse("eth.type in {0x0800, 0x86dd}")
      .unwrap()
      .matcher()
      .eth_type(0x0800)
      .is_match()

A single test in src/driver.rs, alongside the existing integration-style tests, should cover positive and negative matching.

New parser duplicates existing code

parse_u32_hex_or_dec at src/value_parsers.rs:38-53 duplicates nearly all of parse_u64 immediately above it: UTF-8 conversion, underscore allocation, prefix handling, radix parsing.

Reuse parse_u64, then checked-convert to the EtherType range. That fixes both duplication and the P1 bug with fewer LOC.

Underscore handling is overly permissive

The parser removes every underscore, accepting malformed literals such as:

  0x1_
  _1
  0x1__2

Additional edge cases

Worth covering or defining:

  • EtherType maximum: 0xffff.
  • Out-of-range values in every operation form, especially lists and inequalities.
  • Whether malformed underscore placement is legal.
  • Missing metadata: an eth.type clause returns false if the caller never calls .eth_type(...); negating that clause then returns true, consistent with existing fields but worth understanding.
  • VLAN/QinQ: clarify whether .eth_type(...) means the outer Ethernet TPID or encapsulated protocol. A single u16 cannot represent multiple nested protocol identifiers.
  • IEEE 802.3 length-field frames: clarify whether callers should omit eth_type when the field is a length rather than an EtherType.

- Replace parse_u32_hex_or_dec (duplicated parse_u64) with
  parse_ethertype, which rejects values above u16::MAX via
  checked conversion — 65536/0x10000 now fail at parse time
- Wire parse_ethertype into the eth.type parser path
- Add test_parse_ethertype covering boundary values
- Extend test_parse_eth_type_failure with out-of-range cases
- Add test_eth_type_e2e_parse_and_match exercising the full
  public parse → matcher → is_match path
- Add precheck.sh mirroring all three CI checks (nightly fmt,
  clippy -D warnings, cargo test)
- Add CLAUDE.md documenting nightly rustfmt requirement

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@talvaknin1
talvaknin1 force-pushed the AX-98261-add-ethertype-filter branch from 92a9b10 to d82d0c4 Compare September 2, 2026 13:09
talvaknin1 and others added 3 commits September 2, 2026 16:16
- Remove redundant & in writeln! format arg (useless_borrows_in_formatting)
- Convert late-initialized nibble to if-else expression (needless_late_init)
- Switch chunks_exact(3/2) to as_chunks::<N>() (chunks_exact_to_as_chunks)
- Replace [b':', b'-'] literal with b":-" (byte_char_slices)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cargo llvm-cov covers local coverage needs for free.
The Codecov upload was hitting rate limits (no token) and
depending on a Node 20 action that is now deprecated.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Keep cargo-llvm-cov to catch untested code in CI but remove
the Codecov upload step that was hitting rate limits and
using the deprecated codecov-action@v3 (Node 20).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread .github/workflows/rust.yml
talvaknin1 and others added 4 commits September 6, 2026 09:36
…cts leading (_1), trailing (1_), and consecutive 173 +

  (1__2) underscores, including adjacent to the hex prefix (0x_1, 0x1_). 7 new failure cases added to test_parse_u64.         174 +### Missing fields
                                                                                                                              175 +
  2. Generic bound (src/parser.rs) — parse_value_operations_with dropped the unused E type parameter; bound simplified to F:  176 +If the caller does not supply a field value on the `Matcher` (e.g. never calls `.e
  Fn(&BStr) -> Result<u32, &'static str>.                                                                                         +th_type(...)`),
                                                                                                                              177 +any clause that references that field evaluates to **false**. Negating such a clau
  3. Docs (docs/syntax.md, src/expression.rs) — three topics added:                                                               +se with `not`
  - Underscore placement rules documented in the number section                                                               178 +therefore evaluates to **true**. This is consistent across all fields — a clause c
  - ## Field semantics section at the end of syntax.md covering: missing-field → false / negation → true, VLAN/QinQ layer         +an only match
    disambiguation, IEEE 802.3 length-field convention                                                                        179 +data that was actually provided.
  - Matcher::eth_type() doc comment updated with the same caller guidance inline
… Codecov badge

- Revert as_chunks::<N>() back to chunks_exact (as_chunks requires
  Rust 1.88 which is undeclared; chunks_exact is stable everywhere)
- Scope underscore-separator docs to eth.type and payload byte-read
  expressions; note ports/vlan.id do not support them
- Fix VLAN-tagged detection example: eth.type == 0x8100 only works
  when the caller supplies the outer TPID; use the vlan field instead
- Replace Expression::not() in integration test with parsed
  "not eth.type == 0x0800" so the not syntax is actually exercised
- Add public-parser regression tests for malformed underscore
  placement (1__0, _1, 1_, 0x_1, 0x1_) which previously parsed
  silently and now correctly error
- Remove Codecov badge from README (upload removed from CI)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Stable Rust 1.98 clippy fires chunks_exact_to_as_chunks and
byte_char_slices on chunks_exact(N) with a constant and on
[b':', b'-'] literals. The previous revert was wrong — as_chunks
is stable and what CI requires.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@talvaknin1
talvaknin1 merged commit 34ff887 into dovreshef:main Sep 7, 2026
5 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.

2 participants