Various components ... #135
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements several improvements and extensions across multiple components of the system. It updates documentation for cross‐namespace verification, refactors route and prefix table handling (including revised cost constants and nexthop selection), adds a new repository command, and updates generated TLV encoding to use updated Length properties.
- Updated documentation and specification for CrossSchema.
- Modified best route suppression and nexthop selection in the forwarding strategy.
- Added a new “repo” command and refactored DV components (e.g. prefix table, fib updates, TLV encoding).
Reviewed Changes
Copilot reviewed 118 out of 119 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/cross-schema.md | Added specification for CrossSchema with minor typographical issues. |
| fw/fw/bestroute.go | Adjusted suppression time and updated nexthop selection logic. |
| cmd/cmd.go | Added cmdRepo() command for the NDND repo. |
| dv/SPEC.md | Renamed and reformatted advertisement naming for DV. |
| dv/dv/router.go | Updated memory store usage and trust config; modified prefix naming. |
| dv/table/neighbor_table.go | Removed certRoute function and modified route registration accordingly. |
| dv/table/fib.go | Changed cost handling logic for Fib entries. |
| dv/table/prefix_table.go | Refactored announcement and withdraw logic to include cost updates. |
| dv/config/config.go | Updated cost constants and removed unused prefix methods. |
| e2e/dv_util.py & test_001.py | Extended convergence testing to conditionally use nfdc. |
| fw/table/rib.go | Updated nexthop update with additional capture flag check. |
| fw/mgmt/nlsr_readvertiser.go | Revised logging and removed redundant advertisement accounting. |
| dv/dv/advert_sync.go | Changed sync interest implementation to use data name for signing. |
| fw/mgmt/thread.go | Updated storage instantiation in the management thread. |
| dv/tlv/zz_generated.go & | Replaced usage of “length” with “Length” in TLV encoding. |
| fw/defn/zz_generated.go |
Files not reviewed (1)
- dv/config/schema.trust: Language not supported
Comments suppressed due to low confidence (2)
docs/cross-schema.md:79
- There is a spelling mistake 'schmea' in the error message; it should be corrected to 'schema'.
1. Use trust schmea to check if the KeyLocator certificate is allowed to sign the data (`no`).
dv/table/fib.go:26
- For consistency with later cost comparisons (which use config.CostPfxInfinity), consider updating this condition to use config.CostPfxInfinity to avoid potential inconsistencies in cost handling.
if ns := nt.GetH(ribEntry.nextHop1); ns != nil && ribEntry.lowest1 < config.CostInfinity {
| } | ||
|
|
||
| // TODO: Trust config may be specific to application | ||
| // This may need us to make a client for each app |
There was a problem hiding this comment.
Does repo need to know the application trust config?
There was a problem hiding this comment.
Open question. I want to say no, but need closer inspection if that adds additional constraints on app trust model.
| enc.NewVersionComponent(3), | ||
| } | ||
|
|
||
| type RepoCmd struct { |
There was a problem hiding this comment.
Do you want to modify ndn-python-repo spec or create a new spec doc?
There was a problem hiding this comment.
Yes, good point. This spec is a bit ad-hoc at the moment because SVS-ALO itself isn't specified.
| @@ -35,10 +35,6 @@ func (f *StreamFace) String() string { | |||
| return fmt.Sprintf("stream-face (%s://%s)", f.network, f.addr) | |||
| } | |||
|
|
|||
| func (f *StreamFace) Trait() Face { | |||
There was a problem hiding this comment.
To clarify: this Trait is to prevent uncertainty of interface type checking in Go: when the implementation broke an interface, it is the caller rather than the implementation that raises errors, and the error message sometimes confusing.
There was a problem hiding this comment.
Yeah, it's really annoying Go doesn't have implements. I can't remember why I removed this, will likely restore it.
| SignatureValue enc.Wire `tlv:"0x17"` | ||
|
|
||
| //+field:wire | ||
| CrossSchemaV enc.Wire `tlv:"0x258"` |
There was a problem hiding this comment.
Did people agreed to modify the Data spec on this?
There was a problem hiding this comment.
Depends on whch people :P
This was discussed at length in IRL but not with the broader group yet.
UseDataNameFwHint