[PWCI] "lldp: Add ovs-appctl lldp/neighbor command."#18
[PWCI] "lldp: Add ovs-appctl lldp/neighbor command."#18
Conversation
Moving Russell to emeritus status at his request. Thanks, Russell, for all your contributions and support! Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
OVS adds matches on the DF/CSUM/etc bits of the tunnel info flags, but the DF bit is never actually extracted from the outer IP header during the tunnel decapsulation. This is not a huge problem, as we'll only match on what was parsed out, since matching on DF flag is not exposed through OpenFlow to the users. And since the bit is never extracted, we'll just always match on -df, which sort of "works", because the bit is never extracted and so it is never set. However, this causes misleading -df matches in the datapath flow dumps even if the packets actually have the DF bit set, which it is by default. Fix that by actually extracting the bit from the outer header while decapsulating tunneled traffic. Acked-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Changliang Wu <changliang.wu@smartx.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
New appctl 'lldp/neighbor' displays lldp neighbor information. Support json output with --format json --pretty Signed-off-by: Changliang Wu <changliang.wu@smartx.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
The 802.1 and 802.3 standards list ways for communicating important PoE, Maximum Frame Size, and other important link-level features that peers may want to look at. This commit provides a port of the LLDPD project's protocol decode for 802.1 and 802.3 TLVs of interest, preserving the implementation as closely as possible to the source materials. The supported decodes will allow decoding and displaying: * IEEE 802.1-2005 (Port VLAN and Protocol Configuration) * IEEE 802.3at (Power over Ethernet plus) * IEEE 802.3bt (4-pair PoE) The commit supports displaying this data in human readable and json formats. Signed-off-by: Changliang Wu <changliang.wu@smartx.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Signed-off-by: Changliang Wu <changliang.wu@smartx.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideAdds a new “ovs-appctl lldp/neighbor” command to expose LLDP neighbor details in text or JSON, extends the LLDP data model and decoder to support 802.1 (VLAN, PVID, PPVID, PI) and 802.3 (MAC/PHY auto-negotiation, MFS, power) TLVs, enhances neighbor-print routines to surface this information, and includes minor utility tweaks and a new test. Sequence diagram for ovs-appctl lldp/neighbor command executionsequenceDiagram
actor User
participant "ovs-appctl"
participant "ovs-vswitchd"
participant "lldp_unixctl_show_neighbor()"
participant "lldp_print_neighbor_json() / lldp_print_neighbor()"
User->>"ovs-appctl": run lldp/neighbor [--format=json]
"ovs-appctl"->>"ovs-vswitchd": send unixctl command
"ovs-vswitchd"->>"lldp_unixctl_show_neighbor()": dispatch command
"lldp_unixctl_show_neighbor()"->>"lldp_print_neighbor_json() / lldp_print_neighbor()": gather LLDP neighbor info
"lldp_print_neighbor_json() / lldp_print_neighbor()"-->>"lldp_unixctl_show_neighbor()": return formatted neighbor data
"lldp_unixctl_show_neighbor()"-->>"ovs-vswitchd": reply with neighbor info
"ovs-vswitchd"-->>"ovs-appctl": send output
"ovs-appctl"-->>User: display neighbor info
Class diagram for extended LLDP data model (lldpd_port and related structs)classDiagram
class lldpd_port {
char *p_descr
uint16_t p_mfs
uint32_t p_aggregid
lldpd_dot3_macphy p_macphy
lldpd_dot3_power p_power
uint16_t p_pvid
ovs_list p_vlans
ovs_list p_ppvids
ovs_list p_pids
ovs_list p_isid_vlan_maps
}
class lldpd_vlan {
ovs_list v_entries
char *v_name
uint16_t v_vid
}
class lldpd_ppvid {
ovs_list p_entries
uint8_t p_cap_status
uint16_t p_ppvid
}
class lldpd_pi {
ovs_list p_entries
char *p_pi
int p_pi_len
}
class lldpd_dot3_macphy {
uint8_t autoneg_support
uint8_t autoneg_enabled
uint16_t autoneg_advertised
uint16_t mau_type
}
class lldpd_dot3_power {
uint8_t devicetype
uint8_t supported
uint8_t enabled
uint8_t paircontrol
uint8_t pairs
uint8_t class
uint8_t powertype
uint8_t source
uint8_t priority
uint16_t requested
uint16_t allocated
uint8_t pd_4pid
uint16_t requested_a
uint16_t requested_b
uint16_t allocated_a
uint16_t allocated_b
uint16_t pse_status
uint8_t pd_status
uint8_t pse_pairs_ext
uint8_t class_a
uint8_t class_b
uint8_t class_ext
uint8_t type_ext
uint8_t pd_load
uint16_t pse_max
}
lldpd_port "1" o-- "*" lldpd_vlan : contains
lldpd_port "1" o-- "*" lldpd_ppvid : contains
lldpd_port "1" o-- "*" lldpd_pi : contains
lldpd_port "1" o-- lldpd_dot3_macphy : has
lldpd_port "1" o-- lldpd_dot3_power : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughUpdates maintainer status. Adds LLDP neighbor display (text/JSON), LLDP TLV decoding (dot1/dot3 incl. 802.3bt), new LLDP data structures and cleanup, JSON route/show note, utility helper for non-empty strings, color parser loop tweak, tunnel DF/CSUM flag handling, docs/manpage, and tests including new LLDP tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant OVC as ovs-vsctl
participant OVS as ovs-vswitchd (unixctl)
participant LLDP as LLDP subsystem
U->>OVC: ovs-vsctl lldp/neighbor [iface] [--format json]
OVC->>OVS: unixctl: lldp/neighbor [iface]
OVS->>LLDP: Query neighbors for iface/all
LLDP-->>OVS: Neighbor data (dot1, dot3, power, VLANs, PPVIDs, PIs)
OVS-->>OVC: Formatted response (text/JSON)
OVC-->>U: Display output
rect rgba(230,245,255,0.5)
note over OVS,LLDP: New/changed: decoding dot1/dot3 TLVs and JSON/text rendering
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/lldp/lldp.c:603` </location>
<code_context>
+
+ case LLDP_TLV_DOT1_PPVID:
+ CHECK_TLV_SIZE(7, "PPVID");
+ /* validation needed */
+ /* PPVID has to be unique if more than
+ one PPVID TLVs are received -
+ discard if duplicate */
+ /* if support bit is not set and
+ enabled bit is set - PPVID TLV is
+ considered error and discarded */
+ /* if PPVID > 4096 - bad and discard */
+ ppvid = xzalloc(sizeof *ppvid);
+ ppvid->p_cap_status = PEEK_UINT8;
+ ppvid->p_ppvid = PEEK_UINT16;
</code_context>
<issue_to_address>
Add validation for PPVID TLV as noted in comments.
Please add checks for duplicate PPVID TLVs and for invalid support/enabled bit combinations to ensure only valid PPVIDs are accepted.
</issue_to_address>
### Comment 2
<location> `lib/lldp/lldpd-structs.c:122` </location>
<code_context>
+ struct lldpd_ppvid *ppvid;
+ struct lldpd_vlan *vlan;
+ struct lldpd_pi *pid;
+ if (port->p_vlans.next && !ovs_list_is_empty(&port->p_vlans)) {
+ LIST_FOR_EACH_SAFE (vlan, v_entries, &port->p_vlans) {
+ ovs_list_remove(&vlan->v_entries);
</code_context>
<issue_to_address>
The check for .next may be redundant with ovs_list_is_empty.
You can remove the port->p_vlans.next check and rely solely on ovs_list_is_empty for this condition.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /* validation needed */ | ||
| /* PPVID has to be unique if more than | ||
| one PPVID TLVs are received - | ||
| discard if duplicate */ | ||
| /* if support bit is not set and | ||
| enabled bit is set - PPVID TLV is | ||
| considered error and discarded */ | ||
| /* if PPVID > 4096 - bad and discard */ | ||
| ppvid = xzalloc(sizeof *ppvid); |
There was a problem hiding this comment.
issue: Add validation for PPVID TLV as noted in comments.
Please add checks for duplicate PPVID TLVs and for invalid support/enabled bit combinations to ensure only valid PPVIDs are accepted.
| struct lldpd_ppvid *ppvid; | ||
| struct lldpd_vlan *vlan; | ||
| struct lldpd_pi *pid; | ||
| if (port->p_vlans.next && !ovs_list_is_empty(&port->p_vlans)) { |
There was a problem hiding this comment.
suggestion: The check for .next may be redundant with ovs_list_is_empty.
You can remove the port->p_vlans.next check and rely solely on ovs_list_is_empty for this condition.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
vswitchd/ovs-vswitchd.8.in (1)
152-155: Document JSON output hint.This command supports JSON output (via ovs-appctl -f json). Consider noting that for usability, mirroring other commands that mention alternate formats.
Apply this minimal addition after the description:
- If \fIinterface\fR is not specified, then it displays detailed information - about all interfaces with LLDP enabled. + If \fIinterface\fR is not specified, then it displays detailed information + about all interfaces with LLDP enabled. Use \fBovs-appctl -f json\fR for + machine-readable JSON output.lib/colors.c (1)
120-127: Loop guard refactor—LGTM; tiny nit on digit check.The nullable_string_not_empty() guard keeps semantics intact. Optionally use isdigit((unsigned char)*ptr) for readability and locale safety.
- if (*ptr == ';' || (*ptr >= '0' && *ptr <= '9')) { + if (*ptr == ';' || isdigit((unsigned char)*ptr)) { continue; }lib/lldp/lldpd-structs.c (1)
116-145: Ensure lists are always initialized; simplify cleanup.Initialization is present in lib/lldp/lldp.c (≈lines 394–398: ovs_list_init(&port->p_vlans), ovs_list_init(&port->p_ppvids), ovs_list_init(&port->p_pids)). In lib/lldp/lldpd-structs.c (snippet lines 116–145) remove the fragile "port->p_.next &&" guards and unconditionally use LIST_FOR_EACH_SAFE to free each list then call ovs_list_init(&port->p_).
lib/ovs-lldp.c (2)
352-433: JSON schema stability: prefer arrays consistently.Currently “vlan”, “ppvid”, and “pi” emit an object for single entries and an array for multiple. Returning arrays consistently simplifies clients.
Example pattern:
- if (vlan_json_array || vlan_json) { - json_object_put(interface_item_json, "vlan", - vlan_json_array ? vlan_json_array : vlan_json); - } + if (!vlan_json_array) vlan_json_array = json_array_create_empty(); + if (vlan_json) json_array_add(vlan_json_array, vlan_json); + json_object_put(interface_item_json, "vlan", vlan_json_array);
752-944: JSON structure uses dynamic keys; consider stable fields.Using chassis name as a key under "chassis" (and interface name as a top-level key inside each wrapped item) complicates parsing and risks key collisions. Prefer fixed keys like {"name": "..."} and always return arrays for "interface" regardless of count.
- json_object_put(chassis_json, port->p_chassis->c_name, chassis_sys_json); + json_object_put(chassis_json, "name", json_string_create(port->p_chassis->c_name)); + json_object_put(chassis_json, "sys", chassis_sys_json);- json_object_put(interface_json, "interface", - interface_array_json == NULL ? interface_item_warp_json_last - : interface_array_json); + struct json *arr = interface_array_json ? interface_array_json : json_array_create_empty(); + if (!interface_array_json) json_array_add(arr, interface_item_warp_json_last); + json_object_put(interface_json, "interface", arr);tests/ovs-lldp.at (1)
36-37: Fix PPVID text formattingPrinted line lacks a comma/label: “PPVID: supported: yes,enabled no”. Should be “PPVID: supported: yes, enabled: no”.
If printer is fixed, reflect here:
- PPVID: supported: yes,enabled no + PPVID: supported: yes, enabled: noAlso applies to: 109-110
lib/lldp/lldpd-structs.h (1)
66-72: Struct additions look good; prefer fixed-width stdint types for portabilityConsider switching u_intX_t → uintX_t for cross-platform headers and consistent ABI.
Example:
- u_int16_t autoneg_advertised; + uint16_t autoneg_advertised;Also applies to: 73-103, 159-169
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
MAINTAINERS.rst(1 hunks)NEWS(1 hunks)lib/colors.c(1 hunks)lib/lldp/lldp-const.h(1 hunks)lib/lldp/lldp.c(7 hunks)lib/lldp/lldpd-structs.c(2 hunks)lib/lldp/lldpd-structs.h(2 hunks)lib/netdev-native-tnl.c(2 hunks)lib/ovs-lldp.c(5 hunks)lib/util.h(1 hunks)tests/automake.mk(1 hunks)tests/nsh.at(2 hunks)tests/ovs-lldp.at(1 hunks)tests/packet-type-aware.at(8 hunks)tests/testsuite.at(1 hunks)tests/tunnel-push-pop.at(1 hunks)vswitchd/ovs-vswitchd.8.in(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
lib/netdev-native-tnl.c (1)
include/sparse/netinet/in.h (1)
htons(158-161)
lib/colors.c (1)
lib/util.h (1)
nullable_string_not_empty(200-204)
lib/lldp/lldp.c (4)
lib/util.h (1)
nullable_string_not_empty(200-204)lib/dp-packet.c (1)
dp_packet_put(367-373)include/openvswitch/list.h (2)
ovs_list_init(147-151)ovs_list_push_back(202-206)lib/util.c (1)
xzalloc(165-170)
lib/lldp/lldpd-structs.c (1)
include/openvswitch/list.h (3)
ovs_list_is_empty(318-322)ovs_list_remove(249-255)ovs_list_init(147-151)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Sourcery review
- GitHub Check: freebsd_build COMPILER:gcc freebsd_instance:family/freebsd-14-3-snap
- GitHub Check: freebsd_build COMPILER:clang freebsd_instance:family/freebsd-13-5-snap
- GitHub Check: linux rpm fedora
🔇 Additional comments (21)
lib/netdev-native-tnl.c (2)
155-157: DF flag propagation into tunnel metadata looks correct.Using |= FLOW_TNL_F_DONT_FRAGMENT based on IPv4 DF is the right behavior and aligns with later header construction.
468-475: Do not clobber tunnel flags when GRE CSUM present — good fix.Switching to “|= FLOW_TNL_F_CSUM” preserves KEY/other bits previously set.
MAINTAINERS.rst (1)
94-95: Maintainer status update reads well.Moving Russell Bryant to Emeritus Maintainers is correctly reflected.
tests/tunnel-push-pop.at (1)
688-689: Expectation updated to flags(+df-csum+key) — consistent with DF propagation.Matches the code change to carry DF and CSUM flags from outer headers.
lib/lldp/lldp-const.h (1)
101-105: Adds 802.3bt power type macros — verify mapping and usage.Looks fine, but please confirm these constants align with 802.3bt TLV encoding and are used in distinct fields from 802.3at types to avoid ambiguity (both sets use 0/1/2 values).
Would you like me to scan the LLDP DOT3 power parsing/printing code paths to ensure these constants are wired correctly and documented?
tests/nsh.at (2)
722-724: NSH test updated for flags(+df-csum+key) after decap — consistent.The dpctl flow dump now reflecting +df matches GRE/UDP handling changes.
776-779: Indirect path expectations updated to +df-csum+key — consistent.Aligns with per-tunnel DF semantics across stages.
tests/packet-type-aware.at (1)
327-327: DF/CSUM flag expectations look correct.The updated flow-dump expectations reflecting tunnel(..., flags(+df-csum)) in recirculation paths are consistent with per-tunnel DF semantics and GRE CSUM preservation. No issues spotted.
Also applies to: 345-345, 363-363, 381-382, 400-402, 419-420, 505-505, 1018-1020
tests/testsuite.at (1)
69-69: Wire LLDP tests into the suite—LGTM.Including tests/ovs-lldp.at here is correct and keeps ordering coherent.
tests/automake.mk (1)
71-71: Add ovs-lldp.at to TESTSUITE_AT—LGTM.It’s covered by EXTRA_DIST via $(TESTSUITE_AT), so dist is fine.
lib/util.h (1)
200-205: Helper is simple and broadly useful—LGTM.Name aligns with existing nullable_* helpers and avoids macro pitfalls.
lib/lldp/lldpd-structs.c (1)
161-161: Call cleanup is in the right place—LGTM.Integrating dot1/dot3 list cleanup into lldpd_port_cleanup avoids leaks.
lib/ovs-lldp.c (3)
316-350: Dot1 text output—LGTM.Covers VLANs, PVID, PPVID caps, and PIs as expected.
1003-1044: Unixctl handler: text/JSON split—LGTM.Scope/locking look correct; optional arg handling is clear.
1312-1314: Command registration—LGTM.Name, arity, and handler match the man page.
tests/ovs-lldp.at (1)
15-37: Consider asserting deterministic neighbor ordering or sorting in outputTwo neighbors on same iface appear in insertion order. If ordering matters for UIs/automation, add sort (e.g., by PortID) or make test tolerant to order.
Also applies to: 88-131
lib/lldp/lldp.c (3)
197-201: Good: skip empty optional strings when emitting TLVsAvoids sending zero-length SystemName/Descr/PortDescr.
Also applies to: 204-208, 237-241
396-399: Good: initialize new per-port DOT1 listsPrevents list misuse when populating VLANs/PPVIDs/PIs.
651-656: Confirm DOT3 Link Aggregation parsingStandard has a “capability/status” octet before aggregID; you discard 1 byte then read 32-bit ID. Ensure endianness/width matches spec and that the skipped status isn’t needed for output. If not used, add a comment.
lib/lldp/lldpd-structs.h (2)
73-103: Unused field pd_4pidpd_4pid is defined but never populated. Either parse/set it or drop it to avoid confusion.
164-169: Nice: document list element typesThe list annotations help prevent misuse. LGTM.
| switch (tlv_subtype) { | ||
| case LLDP_TLV_DOT1_VLANNAME: | ||
| CHECK_TLV_SIZE(7, "VLAN"); | ||
| vlan = xzalloc(sizeof *vlan); | ||
| vlan->v_vid = PEEK_UINT16; | ||
| vlan_len = PEEK_UINT8; | ||
| CHECK_TLV_SIZE(7 + vlan_len, "VLAN"); | ||
| vlan->v_name = xzalloc(vlan_len + 1); | ||
| PEEK_BYTES(vlan->v_name, vlan_len); | ||
| ovs_list_push_back(&port->p_vlans, &vlan->v_entries); | ||
| vlan = NULL; | ||
| break; | ||
|
|
There was a problem hiding this comment.
Leak on malformed VLANNAME TLV (allocate after size check)
If “CHECK_TLV_SIZE(7 + vlan_len, ...)” fails after allocating vlan, it leaks. Allocate after validating sizes.
Apply:
- case LLDP_TLV_DOT1_VLANNAME:
- CHECK_TLV_SIZE(7, "VLAN");
- vlan = xzalloc(sizeof *vlan);
- vlan->v_vid = PEEK_UINT16;
- vlan_len = PEEK_UINT8;
- CHECK_TLV_SIZE(7 + vlan_len, "VLAN");
- vlan->v_name = xzalloc(vlan_len + 1);
- PEEK_BYTES(vlan->v_name, vlan_len);
- ovs_list_push_back(&port->p_vlans, &vlan->v_entries);
- vlan = NULL;
- break;
+ case LLDP_TLV_DOT1_VLANNAME: {
+ u_int16_t vid;
+ CHECK_TLV_SIZE(7, "VLAN");
+ vid = PEEK_UINT16;
+ vlan_len = PEEK_UINT8;
+ CHECK_TLV_SIZE(7 + vlan_len, "VLAN");
+ vlan = xzalloc(sizeof *vlan);
+ vlan->v_vid = vid;
+ vlan->v_name = xzalloc(vlan_len + 1);
+ PEEK_BYTES(vlan->v_name, vlan_len);
+ ovs_list_push_back(&port->p_vlans, &vlan->v_entries);
+ vlan = NULL;
+ break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (tlv_subtype) { | |
| case LLDP_TLV_DOT1_VLANNAME: | |
| CHECK_TLV_SIZE(7, "VLAN"); | |
| vlan = xzalloc(sizeof *vlan); | |
| vlan->v_vid = PEEK_UINT16; | |
| vlan_len = PEEK_UINT8; | |
| CHECK_TLV_SIZE(7 + vlan_len, "VLAN"); | |
| vlan->v_name = xzalloc(vlan_len + 1); | |
| PEEK_BYTES(vlan->v_name, vlan_len); | |
| ovs_list_push_back(&port->p_vlans, &vlan->v_entries); | |
| vlan = NULL; | |
| break; | |
| switch (tlv_subtype) { | |
| case LLDP_TLV_DOT1_VLANNAME: { | |
| u_int16_t vid; | |
| CHECK_TLV_SIZE(7, "VLAN"); | |
| vid = PEEK_UINT16; | |
| vlan_len = PEEK_UINT8; | |
| CHECK_TLV_SIZE(7 + vlan_len, "VLAN"); | |
| vlan = xzalloc(sizeof *vlan); | |
| vlan->v_vid = vid; | |
| vlan->v_name = xzalloc(vlan_len + 1); | |
| PEEK_BYTES(vlan->v_name, vlan_len); | |
| ovs_list_push_back(&port->p_vlans, &vlan->v_entries); | |
| vlan = NULL; | |
| break; | |
| } |
🤖 Prompt for AI Agents
In lib/lldp/lldp.c around lines 583 to 595, the code allocates vlan before the
second CHECK_TLV_SIZE so a failed size check leaks; reorder to read v_vid and
vlan_len, perform CHECK_TLV_SIZE(7 + vlan_len, "VLAN") first, then allocate the
vlan struct and vlan->v_name (and only after successful size validation call
PEEK_BYTES into vlan->v_name) and push to list; ensure no allocations happen
before the final size validation so malformed TLVs do not leak.
| case LLDP_TLV_DOT1_PPVID: | ||
| CHECK_TLV_SIZE(7, "PPVID"); | ||
| /* validation needed */ | ||
| /* PPVID has to be unique if more than | ||
| one PPVID TLVs are received - | ||
| discard if duplicate */ | ||
| /* if support bit is not set and | ||
| enabled bit is set - PPVID TLV is | ||
| considered error and discarded */ | ||
| /* if PPVID > 4096 - bad and discard */ | ||
| ppvid = xzalloc(sizeof *ppvid); | ||
| ppvid->p_cap_status = PEEK_UINT8; | ||
| ppvid->p_ppvid = PEEK_UINT16; | ||
| ovs_list_push_back(&port->p_ppvids, &ppvid->p_entries); | ||
| break; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add PPVID validation (spec notes are commented but not enforced)
Enforce: unique PPVIDs, enabled ⇒ supported, PPVID range (1–4094). Reject/skip invalid TLVs with a warning.
I can send a patch adding a small helper to validate and dedupe before push_back—want me to draft it?
🤖 Prompt for AI Agents
In lib/lldp/lldp.c around lines 601-616, the PPVID TLV handling creates and
unconditionally pushes a ppvid structure but the commented spec validations are
not enforced; implement validation to (1) reject PPVID values outside 1–4094,
(2) ensure the enabled bit implies the support bit is set (if enabled and not
supported, skip), and (3) skip duplicate PPVID values already present in
port->p_ppvids while logging a warning for each rejected TLV. Add a small helper
(e.g., validate_and_add_ppvid(port, ppvid)) that checks the parsed p_ppvid and
p_cap_status against the rules, logs a warning with context on failure, frees
the allocated ppvid when skipping, and only calls ovs_list_push_back when
validation passes so duplicates and invalid entries are not added.
| case LLDP_TLV_DOT1_PI: | ||
| /* validation needed */ | ||
| /* PI has to be unique if more than | ||
| one PI TLVs are received - discard | ||
| if duplicate ?? */ | ||
| pi = xzalloc(sizeof *pi); | ||
| pi->p_pi_len = PEEK_UINT8; | ||
| CHECK_TLV_SIZE(5 + pi->p_pi_len, "PI"); | ||
| pi->p_pi = xzalloc(pi->p_pi_len); | ||
| PEEK_BYTES(pi->p_pi, pi->p_pi_len); | ||
| ovs_list_push_back(&port->p_pids, &pi->p_entries); | ||
| pi = NULL; | ||
| break; | ||
|
|
There was a problem hiding this comment.
Leak on malformed PI TLV (allocate after size check)
Same pattern: allocation before length validation leaks on malformed PI.
Apply:
- case LLDP_TLV_DOT1_PI:
- /* validation needed */
- /* PI has to be unique if more than
- one PI TLVs are received - discard
- if duplicate ?? */
- pi = xzalloc(sizeof *pi);
- pi->p_pi_len = PEEK_UINT8;
- CHECK_TLV_SIZE(5 + pi->p_pi_len, "PI");
- pi->p_pi = xzalloc(pi->p_pi_len);
- PEEK_BYTES(pi->p_pi, pi->p_pi_len);
- ovs_list_push_back(&port->p_pids, &pi->p_entries);
- pi = NULL;
- break;
+ case LLDP_TLV_DOT1_PI: {
+ /* validation needed */
+ u_int8_t len = PEEK_UINT8;
+ CHECK_TLV_SIZE(5 + len, "PI");
+ pi = xzalloc(sizeof *pi);
+ pi->p_pi_len = len;
+ pi->p_pi = xzalloc(len);
+ PEEK_BYTES(pi->p_pi, len);
+ ovs_list_push_back(&port->p_pids, &pi->p_entries);
+ pi = NULL;
+ break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case LLDP_TLV_DOT1_PI: | |
| /* validation needed */ | |
| /* PI has to be unique if more than | |
| one PI TLVs are received - discard | |
| if duplicate ?? */ | |
| pi = xzalloc(sizeof *pi); | |
| pi->p_pi_len = PEEK_UINT8; | |
| CHECK_TLV_SIZE(5 + pi->p_pi_len, "PI"); | |
| pi->p_pi = xzalloc(pi->p_pi_len); | |
| PEEK_BYTES(pi->p_pi, pi->p_pi_len); | |
| ovs_list_push_back(&port->p_pids, &pi->p_entries); | |
| pi = NULL; | |
| break; | |
| case LLDP_TLV_DOT1_PI: { | |
| /* validation needed */ | |
| u_int8_t len = PEEK_UINT8; | |
| CHECK_TLV_SIZE(5 + len, "PI"); | |
| pi = xzalloc(sizeof *pi); | |
| pi->p_pi_len = len; | |
| pi->p_pi = xzalloc(len); | |
| PEEK_BYTES(pi->p_pi, len); | |
| ovs_list_push_back(&port->p_pids, &pi->p_entries); | |
| pi = NULL; | |
| break; | |
| } |
🤖 Prompt for AI Agents
In lib/lldp/lldp.c around lines 617-630, the code allocates the PI structure
before validating the length which can leak memory on malformed TLVs; fix by
first read the PI length into a local variable (without allocating), run
CHECK_TLV_SIZE(5 + len, "PI"), then allocate the pi struct and its p_pi buffer
using the validated length, populate fields and push to the list; alternatively,
if you must allocate earlier ensure you free pi and any allocated buffers before
any early-return/error path triggered by size validation.
| port->p_power.type_ext = PEEK_UINT8; | ||
| port->p_power.pd_load = (port->p_power.type_ext & 0x1); | ||
| port->p_power.type_ext = | ||
| ((port->p_power.type_ext & | ||
| (1 << 3 | 1 << 2 | 1 << 1)) + | ||
| 1); | ||
| port->p_power.pse_max = PEEK_UINT16; | ||
| } else { |
There was a problem hiding this comment.
Bit-extraction bug: DOT3 power type_ext not shifted
Currently masks bits 3..1 but doesn’t right-shift; values will be 0/2/4/6/… then +1. Should shift >>1 and mask 0x7 before +1.
Apply:
- port->p_power.type_ext = PEEK_UINT8;
- port->p_power.pd_load = (port->p_power.type_ext & 0x1);
- port->p_power.type_ext =
- ((port->p_power.type_ext &
- (1 << 3 | 1 << 2 | 1 << 1)) +
- 1);
+ {
+ u_int8_t type_ext = PEEK_UINT8;
+ port->p_power.pd_load = type_ext & 0x1;
+ port->p_power.type_ext = (((type_ext >> 1) & 0x7) + 1);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| port->p_power.type_ext = PEEK_UINT8; | |
| port->p_power.pd_load = (port->p_power.type_ext & 0x1); | |
| port->p_power.type_ext = | |
| ((port->p_power.type_ext & | |
| (1 << 3 | 1 << 2 | 1 << 1)) + | |
| 1); | |
| port->p_power.pse_max = PEEK_UINT16; | |
| } else { | |
| { | |
| u_int8_t type_ext = PEEK_UINT8; | |
| port->p_power.pd_load = type_ext & 0x1; | |
| port->p_power.type_ext = (((type_ext >> 1) & 0x7) + 1); | |
| } | |
| port->p_power.pse_max = PEEK_UINT16; | |
| } else { |
🤖 Prompt for AI Agents
In lib/lldp/lldp.c around lines 716 to 723, the DOT3 power
port->p_power.type_ext extraction masks bits 3..1 but does not right-shift,
producing values like 0/2/4/... then adding one; change the expression to first
mask with 0x0E or 0x7 and then right-shift by 1 before adding 1 (e.g.
((port->p_power.type_ext & 0x0E) >> 1) + 1 or ((port->p_power.type_ext >> 1) &
0x7) + 1) so the extracted field is correctly aligned before incrementing.
| static void | ||
| lldp_print_neighbor_port_dot3_json(struct json *port_json, | ||
| struct lldpd_port *port) | ||
| { | ||
| if (port->p_mfs) { | ||
| json_object_put(port_json, "mfs", json_integer_create(port->p_mfs)); | ||
| } | ||
|
|
||
| uint16_t autoneg_advertised = port->p_macphy.autoneg_advertised; | ||
| uint16_t autoneg_support = port->p_macphy.autoneg_support; | ||
| uint16_t autoneg_enabled = port->p_macphy.autoneg_enabled; | ||
| uint16_t mautype = port->p_macphy.mau_type; | ||
| if (autoneg_support || autoneg_enabled || mautype) { | ||
| struct json *auto_nego_json = json_object_create(); | ||
| struct json *advertised_json = NULL; | ||
|
|
||
| json_object_put(auto_nego_json, "supported", | ||
| json_boolean_create(autoneg_support)); | ||
| json_object_put(auto_nego_json, "enabled", | ||
| json_boolean_create(autoneg_enabled)); | ||
|
|
||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_10BASE_T, | ||
| LLDP_DOT3_LINK_AUTONEG_10BASET_FD, | ||
| "10Base-T"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_100BASE_T4, | ||
| LLDP_DOT3_LINK_AUTONEG_100BASE_T4, | ||
| "100Base-T4"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_100BASE_TX, | ||
| LLDP_DOT3_LINK_AUTONEG_100BASE_TXFD, | ||
| "100Base-TX"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_100BASE_T2, | ||
| LLDP_DOT3_LINK_AUTONEG_100BASE_T2FD, | ||
| "100Base-T2"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_1000BASE_X, | ||
| LLDP_DOT3_LINK_AUTONEG_1000BASE_XFD, | ||
| "1000Base-X"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_1000BASE_T, | ||
| LLDP_DOT3_LINK_AUTONEG_1000BASE_TFD, | ||
| "1000Base-T"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE, | ||
| LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE, | ||
| "FDX_PAUSE"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE, | ||
| LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE, | ||
| "FDX_APAUSE"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE, | ||
| LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE, | ||
| "FDX_SPAUSE"); | ||
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | ||
| LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE, | ||
| LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE, | ||
| "FDX_BPAUSE"); | ||
| json_object_put(auto_nego_json, "current", | ||
| json_integer_create(mautype)); | ||
|
|
||
| json_object_put(port_json, "auto-negotiation", auto_nego_json); | ||
| } | ||
|
|
||
| if (port->p_power.devicetype) { | ||
| struct json *power_json = json_object_create(); | ||
|
|
||
| json_object_put(power_json, "supported", | ||
| json_boolean_create(port->p_power.supported)); | ||
| json_object_put(power_json, "enabled", | ||
| json_boolean_create(port->p_power.enabled)); | ||
| json_object_put(power_json, "paircontrol", | ||
| json_boolean_create(port->p_power.paircontrol)); | ||
| json_object_put(port_json, "power", power_json); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing “advertised” in JSON; data lost and potential leak.
You build advertised_json but never attach it to auto_nego_json, so autoneg advertised modes are absent in output.
Apply:
@@ static void lldp_print_neighbor_port_dot3_json(struct json *port_json, struct lldpd_port *port)
- json_object_put(auto_nego_json, "current",
- json_integer_create(mautype));
+ if (advertised_json) {
+ json_object_put(auto_nego_json, "advertised", advertised_json);
+ }
+ json_object_put(auto_nego_json, "current",
+ json_integer_create(mautype));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void | |
| lldp_print_neighbor_port_dot3_json(struct json *port_json, | |
| struct lldpd_port *port) | |
| { | |
| if (port->p_mfs) { | |
| json_object_put(port_json, "mfs", json_integer_create(port->p_mfs)); | |
| } | |
| uint16_t autoneg_advertised = port->p_macphy.autoneg_advertised; | |
| uint16_t autoneg_support = port->p_macphy.autoneg_support; | |
| uint16_t autoneg_enabled = port->p_macphy.autoneg_enabled; | |
| uint16_t mautype = port->p_macphy.mau_type; | |
| if (autoneg_support || autoneg_enabled || mautype) { | |
| struct json *auto_nego_json = json_object_create(); | |
| struct json *advertised_json = NULL; | |
| json_object_put(auto_nego_json, "supported", | |
| json_boolean_create(autoneg_support)); | |
| json_object_put(auto_nego_json, "enabled", | |
| json_boolean_create(autoneg_enabled)); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_10BASE_T, | |
| LLDP_DOT3_LINK_AUTONEG_10BASET_FD, | |
| "10Base-T"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_T4, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_T4, | |
| "100Base-T4"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_TX, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_TXFD, | |
| "100Base-TX"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_T2, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_T2FD, | |
| "100Base-T2"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_1000BASE_X, | |
| LLDP_DOT3_LINK_AUTONEG_1000BASE_XFD, | |
| "1000Base-X"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_1000BASE_T, | |
| LLDP_DOT3_LINK_AUTONEG_1000BASE_TFD, | |
| "1000Base-T"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE, | |
| "FDX_PAUSE"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE, | |
| "FDX_APAUSE"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE, | |
| "FDX_SPAUSE"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE, | |
| "FDX_BPAUSE"); | |
| json_object_put(auto_nego_json, "current", | |
| json_integer_create(mautype)); | |
| json_object_put(port_json, "auto-negotiation", auto_nego_json); | |
| } | |
| if (port->p_power.devicetype) { | |
| struct json *power_json = json_object_create(); | |
| json_object_put(power_json, "supported", | |
| json_boolean_create(port->p_power.supported)); | |
| json_object_put(power_json, "enabled", | |
| json_boolean_create(port->p_power.enabled)); | |
| json_object_put(power_json, "paircontrol", | |
| json_boolean_create(port->p_power.paircontrol)); | |
| json_object_put(port_json, "power", power_json); | |
| } | |
| } | |
| static void | |
| lldp_print_neighbor_port_dot3_json(struct json *port_json, | |
| struct lldpd_port *port) | |
| { | |
| if (port->p_mfs) { | |
| json_object_put(port_json, "mfs", json_integer_create(port->p_mfs)); | |
| } | |
| uint16_t autoneg_advertised = port->p_macphy.autoneg_advertised; | |
| uint16_t autoneg_support = port->p_macphy.autoneg_support; | |
| uint16_t autoneg_enabled = port->p_macphy.autoneg_enabled; | |
| uint16_t mautype = port->p_macphy.mau_type; | |
| if (autoneg_support || autoneg_enabled || mautype) { | |
| struct json *auto_nego_json = json_object_create(); | |
| struct json *advertised_json = NULL; | |
| json_object_put(auto_nego_json, "supported", | |
| json_boolean_create(autoneg_support)); | |
| json_object_put(auto_nego_json, "enabled", | |
| json_boolean_create(autoneg_enabled)); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_10BASE_T, | |
| LLDP_DOT3_LINK_AUTONEG_10BASET_FD, | |
| "10Base-T"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_T4, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_T4, | |
| "100Base-T4"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_TX, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_TXFD, | |
| "100Base-TX"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_T2, | |
| LLDP_DOT3_LINK_AUTONEG_100BASE_T2FD, | |
| "100Base-T2"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_1000BASE_X, | |
| LLDP_DOT3_LINK_AUTONEG_1000BASE_XFD, | |
| "1000Base-X"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_1000BASE_T, | |
| LLDP_DOT3_LINK_AUTONEG_1000BASE_TFD, | |
| "1000Base-T"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE, | |
| "FDX_PAUSE"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE, | |
| "FDX_APAUSE"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE, | |
| "FDX_SPAUSE"); | |
| lldp_dot3_autoneg_advertised_json(&advertised_json, autoneg_advertised, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE, | |
| LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE, | |
| "FDX_BPAUSE"); | |
| if (advertised_json) { | |
| json_object_put(auto_nego_json, "advertised", advertised_json); | |
| } | |
| json_object_put(auto_nego_json, "current", | |
| json_integer_create(mautype)); | |
| json_object_put(port_json, "auto-negotiation", auto_nego_json); | |
| } | |
| if (port->p_power.devicetype) { | |
| struct json *power_json = json_object_create(); | |
| json_object_put(power_json, "supported", | |
| json_boolean_create(port->p_power.supported)); | |
| json_object_put(power_json, "enabled", | |
| json_boolean_create(port->p_power.enabled)); | |
| json_object_put(power_json, "paircontrol", | |
| json_boolean_create(port->p_power.paircontrol)); | |
| json_object_put(port_json, "power", power_json); | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/ovs-lldp.c around lines 545-623, the function builds advertised_json but
never attaches it to auto_nego_json, losing advertised autoneg data and leaking
the allocated JSON; after the series of lldp_dot3_autoneg_advertised_json calls,
check if advertised_json is non-NULL and then call
json_object_put(auto_nego_json, "advertised", advertised_json) before putting
auto_nego_json into port_json (i.e., before json_object_put(port_json,
"auto-negotiation", auto_nego_json)); this ensures the advertised data is
present in the output and ownership is transferred to auto_nego_json to avoid
leaking the advertised_json object.
| - ovs-vsctl: | ||
| * Added JSON output support to the 'ovs/route/show' command. | ||
| * Added 'lldp/neighbor' command that displays lldp neighbor information. | ||
| - lldp: | ||
| * Added support for decoding dot1 and dot3 TLVs. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
NEWS: Wrong utility name and potential duplication.
“ovs/route/show” and the new “lldp/neighbor” are appctl commands, not ovs-vsctl. Also, JSON support for ovs/route/show is already listed under “v3.6.0 – ovs-appctl”. Suggest fixing the heading and avoiding duplication.
Apply this diff to the Post-v3.6.0 section:
- - ovs-vsctl:
- * Added JSON output support to the 'ovs/route/show' command.
- * Added 'lldp/neighbor' command that displays lldp neighbor information.
+ - ovs-appctl:
+ * Added 'lldp/neighbor' command that displays LLDP neighbor information.Optionally remove the redundant JSON bullet (already documented under v3.6.0), or clarify what’s new if this is an additional enhancement.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - ovs-vsctl: | |
| * Added JSON output support to the 'ovs/route/show' command. | |
| * Added 'lldp/neighbor' command that displays lldp neighbor information. | |
| - lldp: | |
| * Added support for decoding dot1 and dot3 TLVs. | |
| - ovs-appctl: | |
| * Added 'lldp/neighbor' command that displays LLDP neighbor information. | |
| - lldp: | |
| * Added support for decoding dot1 and dot3 TLVs. |
| AT_CHECK([ovs-appctl --format json --pretty lldp/neighbor p1 ], [0], [dnl | ||
| [{ | ||
| "lldp": { | ||
| "interface": { | ||
| "p1": { | ||
| "chassis": { | ||
| "FakeSystemName": { | ||
| "capability": [ | ||
| { | ||
| "enabled": true, | ||
| "type": "Bridge"}, | ||
| { | ||
| "enabled": true, | ||
| "type": "Router"}], | ||
| "descr": "FakeSystemDesc", | ||
| "id": { | ||
| "type": "mac", | ||
| "value": "aa:aa:aa:aa:aa:aa"}, | ||
| "mgmt-iface": [ | ||
| 635], | ||
| "mgmt-ip": [ | ||
| "192.168.0.1"]}}, | ||
| "port": { | ||
| "auto-negotiation": { | ||
| "current": 30, | ||
| "enabled": true, | ||
| "supported": true}, | ||
| "desc": "GigabitEthernet1/0/15 Interface", | ||
| "id": { | ||
| "type": "ifname", | ||
| "value": "GigabitEthernet1/0/15"}, | ||
| "mfs": 10240, | ||
| "power": { | ||
| "enabled": false, | ||
| "paircontrol": false, | ||
| "supported": true}, | ||
| "ttl": 121}, | ||
| "ppvid": { | ||
| "enabled": false, | ||
| "supported": true}, | ||
| "vlan": { | ||
| "pvid": true, | ||
| "value": "VLAN 0001", | ||
| "vlan-id": 1}}}}}] | ||
| ]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize JSON schema: “interface” flips object↔array depending on neighbor count
Single-neighbor JSON uses "interface": { "p1": {...} } while multi-neighbor uses "interface": [ { "p1": {...}}, { "p1": {...}} ]. This is brittle for consumers. Recommend: always emit "interface": { "p1": [ ] }. Update printer and tests accordingly.
Apply shape change in tests (illustrative):
-{
- "lldp": {
- "interface": [
- { "p1": { ... neighbor A ... } },
- { "p1": { ... neighbor B ... } }
- ]
- }
-}
+{
+ "lldp": {
+ "interface": {
+ "p1": [
+ { ... neighbor A ... },
+ { ... neighbor B ... }
+ ]
+ }
+ }
+}Also applies to: 133-219
🤖 Prompt for AI Agents
In tests/ovs-lldp.at around lines 39 to 83 (and also update the related
expectations at 133-219), the expected LLDP JSON is brittle because "interface"
can be an object or an array; change the test fixtures to match the new stable
schema where "interface" is always an object mapping interface name to an array
of neighbor objects (e.g. "interface": { "p1": [ { ...neighbor1... }, {
...neighbor2... } ] }). Update the printer/serializer code to always emit
interface entries as an object whose values are arrays (wrap single neighbor
objects in a one-element array) and adjust all test expectations in this file
(and any affected tests referenced at 133-219) to the new shape.
3 participants
Auto-submission for "http://patchwork.ozlabs.org/project/openvswitch/list/?series=473181"
Summary by Sourcery
Introduce a new ovs-appctl command to show LLDP neighbor information with extended 802.1 and 802.3 TLVs in both text and JSON formats
New Features:
Enhancements:
Tests:
Summary by CodeRabbit