On 10 Oct 2025, at 4:50, Changliang Wu wrote:
> Hi, Eelco, > > On Fri, Sep 19, 2025 at 11:19 PM Eelco Chaudron <[email protected]> wrote: >> >> On 11 Sep 2025, at 10:41, Changliang Wu wrote: >> >>> New appctl 'lldp/neighbor' displays lldp neighbor information. >>> Support json output with --format json --pretty >>> >>> Signed-off-by: Changliang Wu <[email protected]> >> >> Hi Changliang, >> >> Thanks for following up. See some comments below. >> >> //Eelco >> >>> --- >>> NEWS | 3 + >>> lib/ovs-lldp.c | 363 ++++++++++++++++++++++++++++++++++++- >>> vswitchd/ovs-vswitchd.8.in | 4 + >>> 3 files changed, 369 insertions(+), 1 deletion(-) >>> >>> diff --git a/NEWS b/NEWS >>> index 96bf4992c..354d83e73 100644 >>> --- a/NEWS >>> +++ b/NEWS >>> @@ -3,6 +3,9 @@ Post-v3.6.0 >>> - Userspace datapath: >>> * Conntrack now supports the FTP commands EPSV and EPRT with IPv4 >>> connections, instead of limiting these commands to IPv6 only. >>> + - ovs-vsctl: >>> + * Added JSON output support to the 'ovs/route/show' command. >> >> Don't think your change adds this :) > rebase mistake :-( > >> >>> + * Added 'lldp/neighbor' command that displays lldp neighbor >>> information. >>> >>> >>> v3.6.0 - 18 Aug 2025 >>> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c >>> index 152777248..ba2465e66 100644 >>> --- a/lib/ovs-lldp.c >>> +++ b/lib/ovs-lldp.c >>> @@ -36,7 +36,10 @@ >>> #include <stdlib.h> >>> #include "openvswitch/dynamic-string.h" >>> #include "flow.h" >>> +#include "openvswitch/json.h" >>> #include "openvswitch/list.h" >>> +#include "lldp/lldp-const.h" >>> +#include "lldp/lldp-tlv.h" >>> #include "lldp/lldpd.h" >>> #include "lldp/lldpd-structs.h" >>> #include "netdev.h" >>> @@ -193,7 +196,7 @@ aa_print_element_status_port(struct ds *ds, struct >>> lldpd_hardware *hw) >>> struct ds system = DS_EMPTY_INITIALIZER; >>> >>> if (port->p_chassis) { >>> - if (port->p_chassis->c_id_len > 0) { >>> + if (port->p_chassis->c_id_len) { >> >> c_id_len is an int, so we should keep the > 0 check. > > OK > >> >>> ds_put_hex_with_delimiter(&id, port->p_chassis->c_id, >>> port->p_chassis->c_id_len, ":"); >>> } >>> @@ -310,6 +313,320 @@ aa_print_isid_status(struct ds *ds, struct lldp >>> *lldp) OVS_REQUIRES(mutex) >>> } >>> } >>> >>> +static void >>> +lldp_print_neighbor(struct ds *ds, struct lldp *lldp) OVS_REQUIRES(mutex) >>> +{ >>> + const char *none_str = "<None>"; >>> + struct lldpd_hardware *hw; >>> + struct lldpd_port *port; >>> + >>> + if (!lldp->lldpd) { >>> + return; >>> + } >>> + >>> + bool is_first_interface = true; >> >> I would move this to the variable decelerations at the top of this function. >> >>> + LIST_FOR_EACH (hw, h_entries, &lldp->lldpd->g_hardware) { >>> + if (!hw->h_rports.next) { >> >> You should use the proper list API to check if the list is empty. >> However, I would just remove the check, as the below LIST_FOR_EACH() will >> take care of this. > > As mentioned in patch v9: > I found that there is a small time window in the test, and the list > may be created but not initialized. > So I use list.next here to check if the list is available, otherwise > ovs_list_is_empty will return false. > Is there any problem if put this check into ovs_list_is_empty? This is definitely a bug, data structures should never be accessed before they are fully initialized. I would add an ovs_assert() here and debug why this is happening. > static inline bool > ovs_list_is_empty(const struct ovs_list *list) > { > return !list->next || list->next == list; > } > > >> >>> + continue; >>> + } >> >> Extra new line here >> >>> + LIST_FOR_EACH (port, p_entries, &hw->h_rports) { >>> + struct ds chassis_id = DS_EMPTY_INITIALIZER; >>> + >>> + if (!port->p_chassis) { >>> + continue; >>> + } >>> + >>> + if (!is_first_interface) { >>> + ds_put_format(ds, "\n"); >>> + } >>> + is_first_interface = false; >> >> Move this inside an else. >> >>> + >>> + ds_put_format(ds, "Interface: %s\n", lldp->name); >>> + >>> + /* Basic TLV, Chassis ID (Type = 1). */ >>> + if (port->p_chassis->c_id_len) { >> >> Use the > 0 check here also. > ds_put_hex_with_delimiter could handle size=0, so I will remove 'if > check' in next patch. But as port->p_chassis->c_id_len is a signed int, this could still be a problem. Should we change c_id_len to be an unsigned int? >> >>> + ds_put_hex_with_delimiter(&chassis_id, >>> port->p_chassis->c_id, >>> + port->p_chassis->c_id_len, ":"); >>> + } >>> + ds_put_format(ds, " %-20s%s\n", "Chassis ID:", >>> + chassis_id.length ? ds_cstr_ro(&chassis_id) >> >> Don't use chassis_id.length directly, use ds_api if you need this. >> However, why will the following now work? >> >> ds_put_format(ds, " %-20s", "Chassis ID:") >> ds_put_hex_with_delimiter(ds, port->p_chassis->c_id, >> port->p_chassis->c_id_len, ":"); >> ds_put_char(&s, '\n'); >> >> This will save a lot of allocation and destroy dynamic strings. > > This must be better. > >> >>> + : none_str); >>> + ds_destroy(&chassis_id); >> >> However, there are different chassis ID types, not all hex, so more decodes >> might need to be supported. See the comment below about displaying the type >> also? > > Yes, add more types decoded for port id and chassis id in the next patch. > >> >> New line? >> >>> + /* Basic TLV, Port ID (Type = 2). */ >>> + if (port->p_id_subtype == LLDP_PORTID_SUBTYPE_LLADDR) { >>> + struct ds lladdr = DS_EMPTY_INITIALIZER; >>> + if (port->p_id_len) { >>> + ds_put_hex_with_delimiter(&lladdr, (uint8_t *) >>> port->p_id, >>> + port->p_id_len, ":"); >>> + } >>> + ds_put_format(ds, " %-20s%s\n", "PortID:", >>> + lladdr.length ? ds_cstr_ro(&lladdr) : >>> none_str); >>> + ds_destroy(&lladdr); >> >> Same as above. >> >>> + } else { >> >> This assumes the input is ASCII for all other types? Don't think this is >> true. >> We should decode the ones we know; the rest should be decoded as hex. >> >> Should we not also somehow show which port type we are showing? > > Yes > >> >> PortID[ifName]: GigabitEthernet1/0/15 >> >> Note, we should have a testcase for each format we support (and the >> catch-all case). >> >>> + ds_put_format(ds, " %-20s%.*s\n", "PortID:", >>> + (int) (port->p_id ? port->p_id_len >>> + : strlen(none_str)), >>> + port->p_id ? port->p_id : none_str); >>> + } >> >> New line before each comment here and below will make this more readable. >> >>> + /* Basic TLV, Time To Live (Type = 3). */ >>> + ds_put_format(ds, " %-20s%d\n", "TTL:", >>> port->p_chassis->c_ttl); >> >>> + /* Basic TLV, Port Description (Type = 4). */ >>> + ds_put_format(ds, " %-20s%s\n", "PortDescr:", >>> + nullable_string_not_empty(port->p_descr) >>> + ? port->p_descr >>> + : none_str); >> >>> + /* Basic TLV, System Name (Type = 5). */ >>> + ds_put_format(ds, " %-20s%s\n", "SysName:", >>> + >>> nullable_string_not_empty(port->p_chassis->c_name) >>> + ? port->p_chassis->c_name >>> + : none_str); >> >>> + /* Basic TLV, System Description (Type = 6). */ >>> + ds_put_format(ds, " %-20s%s\n", "SysDescr:", >>> + >>> nullable_string_not_empty(port->p_chassis->c_descr) >>> + ? port->p_chassis->c_descr >>> + : none_str); >> >>> + /* Basic TLV, System Capabilities (Type = 7). */ >>> + if (port->p_chassis->c_cap_available & LLDP_CAP_BRIDGE) { >>> + ds_put_format(ds, " %-20sBridge, %s\n","Capability:", >>> + port->p_chassis->c_cap_enabled & >>> LLDP_CAP_BRIDGE >>> + ? "on" >>> + : "off"); >>> + } >>> + if (port->p_chassis->c_cap_available & LLDP_CAP_ROUTER) { >>> + ds_put_format(ds, " %-20sRouter, %s\n","Capability:", >>> + port->p_chassis->c_cap_enabled & >>> LLDP_CAP_ROUTER >>> + ? "on" >>> + : "off"); >>> + } >>> + if (port->p_chassis->c_cap_available & LLDP_CAP_WLAN) { >>> + ds_put_format(ds, " %-20sWlan, %s\n","Capability:", >>> + port->p_chassis->c_cap_enabled & >>> LLDP_CAP_WLAN >>> + ? "on" >>> + : "off"); >>> + } >>> + if (port->p_chassis->c_cap_available & LLDP_CAP_STATION) { >>> + ds_put_format(ds, " %-20sStation, %s\n","Capability:", >>> + port->p_chassis->c_cap_enabled & >>> LLDP_CAP_STATION >>> + ? "on" >>> + : "off"); >>> + } >> >> Do we want to report any of the other bits if set(by bit number)? >> >>> + /* Basic TLV, Management Address (Type = 8). */ >>> + struct lldpd_mgmt *mgmt; >>> + LIST_FOR_EACH (mgmt, m_entries, &port->p_chassis->c_mgmt) { >>> + struct in6_addr ip; >>> + >>> + switch (mgmt->m_family) { >>> + case LLDPD_AF_IPV4: >>> + in6_addr_set_mapped_ipv4(&ip, >>> mgmt->m_addr.inet.s_addr); >>> + break; >>> + >>> + case LLDPD_AF_IPV6: >>> + ip = mgmt->m_addr.inet6; >>> + break; >>> + >>> + default: >>> + continue; >>> + } >>> + ds_put_format(ds, " %-20s", "MgmtIP:"); >>> + ipv6_format_mapped(&ip, ds); >>> + ds_put_format(ds, "\n"); >>> + ds_put_format(ds, " %-20s%d\n", >>> + "MgmtIface:", mgmt->m_iface); >>> + } >>> + } >>> + } >>> +} >>> + >>> +static void >>> +lldp_print_neighbor_json(struct json *interface_json, struct lldp *lldp) >>> + OVS_REQUIRES(mutex) >>> +{ >>> + struct lldpd_hardware *hw; >>> + struct lldpd_port *port; >>> + >>> + if (!lldp->lldpd) { >>> + return; >>> + } >>> + >>> + struct json *interface_item_warp_json_last = NULL; >>> + struct json *interface_array_json = NULL; >>> + bool has_multi_interfaces = false; >>> + >>> + LIST_FOR_EACH (hw, h_entries, &lldp->lldpd->g_hardware) { >>> + if (!hw->h_rports.next) { >>> + continue; >>> + } >> >> See all comments above, as they might apply here too. >> >>> + LIST_FOR_EACH (port, p_entries, &hw->h_rports) { >>> + struct ds chassis_id = DS_EMPTY_INITIALIZER; >> >> new line >> >>> + if (!port->p_chassis) { >>> + continue; >>> + } >> >> This should be below the variable declaration below. >>> + >>> + struct json *interface_item_warp_json = json_object_create(); >>> + struct json *interface_item_json = json_object_create(); >>> + struct json *chassis_json = json_object_create(); >>> + struct json *chassis_sys_json = json_object_create(); >>> + struct json *chassis_id_json = json_object_create(); >>> + struct json *chassis_mgmt_ip_json = json_array_create_empty(); >>> + struct json *chassis_mgmt_iface_json = >>> json_array_create_empty(); >>> + struct json *chassis_capability_json = >>> json_array_create_empty(); >>> + struct json *chassis_capability_item_json; >>> + struct json *port_json = json_object_create(); >>> + struct json *port_id_json = json_object_create(); >> >> Christmas tree order. >> >>> + >>> + if (port->p_chassis->c_id_len) { >>> + ds_put_hex_with_delimiter(&chassis_id, >>> port->p_chassis->c_id, >>> + port->p_chassis->c_id_len, ":"); >>> + } >>> + >>> + json_object_put(chassis_id_json, "type", >>> + json_string_create("mac")); >>> + json_object_put(chassis_id_json, "value", >>> + json_string_create(ds_cstr_ro(&chassis_id))); >> >> Rather than doing a string copy and free, maybe a >> json_string_create_nocopy() + ds_steal_cstr() combination? > > OK > >> >>> + json_object_put(chassis_sys_json, "id", chassis_id_json); >>> + if (nullable_string_not_empty(port->p_chassis->c_name)) { >>> + json_object_put(chassis_json, port->p_chassis->c_name, >>> + chassis_sys_json); >>> + } >>> + if (nullable_string_not_empty(port->p_chassis->c_descr)) { >>> + json_object_put(chassis_sys_json, "descr", >>> + >>> json_string_create(port->p_chassis->c_descr)); >>> + } >>> + ds_destroy(&chassis_id); >>> + >>> + struct lldpd_mgmt *mgmt; >> >> Define at the top >> >>> + LIST_FOR_EACH (mgmt, m_entries, &port->p_chassis->c_mgmt) { >>> + char addr_str[INET6_ADDRSTRLEN]; >>> + struct in6_addr ip; >> >> New line >> >>> + switch (mgmt->m_family) { >>> + case LLDPD_AF_IPV4: >>> + in6_addr_set_mapped_ipv4(&ip, >>> mgmt->m_addr.inet.s_addr); >>> + break; >>> + >>> + case LLDPD_AF_IPV6: >>> + ip = mgmt->m_addr.inet6; >>> + break; >>> + >>> + default: >>> + continue; >>> + } >>> + >>> + ipv6_string_mapped(addr_str, &ip); >>> + json_array_add(chassis_mgmt_ip_json, >>> + json_string_create(addr_str)); >>> + json_array_add(chassis_mgmt_iface_json, >>> + json_integer_create(mgmt->m_iface)); >>> + } >>> + json_object_put(chassis_sys_json, "mgmt-ip", >>> chassis_mgmt_ip_json); >>> + json_object_put(chassis_sys_json, "mgmt-iface", >>> + chassis_mgmt_iface_json); >>> + >>> + if (port->p_chassis->c_cap_available & LLDP_CAP_BRIDGE) { >>> + chassis_capability_item_json = json_object_create(); >>> + json_object_put(chassis_capability_item_json, "type", >>> + json_string_create("Bridge")); >>> + json_object_put( >>> + chassis_capability_item_json, "enabled", >>> + json_boolean_create(port->p_chassis->c_cap_enabled & >>> + LLDP_CAP_BRIDGE)); >>> + json_array_add(chassis_capability_json, >>> + chassis_capability_item_json); >>> + } >> >> new line >> >>> + if (port->p_chassis->c_cap_available & LLDP_CAP_ROUTER) { >>> + chassis_capability_item_json = json_object_create(); >>> + json_object_put(chassis_capability_item_json, "type", >>> + json_string_create("Router")); >>> + json_object_put( >>> + chassis_capability_item_json, "enabled", >>> + json_boolean_create(port->p_chassis->c_cap_enabled & >>> + LLDP_CAP_ROUTER)); >>> + json_array_add(chassis_capability_json, >>> + chassis_capability_item_json); >>> + } >> >> new line >> >>> + if (port->p_chassis->c_cap_available & LLDP_CAP_WLAN) { >>> + chassis_capability_item_json = json_object_create(); >>> + json_object_put(chassis_capability_item_json, "type", >>> + json_string_create("Wlan")); >>> + json_object_put( >>> + chassis_capability_item_json, "enabled", >>> + json_boolean_create(port->p_chassis->c_cap_enabled & >>> + LLDP_CAP_WLAN)); >>> + json_array_add(chassis_capability_json, >>> + chassis_capability_item_json); >>> + } >> >> new line >> >>> + if (port->p_chassis->c_cap_available & LLDP_CAP_STATION) { >>> + chassis_capability_item_json = json_object_create(); >>> + json_object_put(chassis_capability_item_json, "type", >>> + json_string_create("Station")); >>> + json_object_put( >>> + chassis_capability_item_json, "enabled", >>> + json_boolean_create(port->p_chassis->c_cap_enabled & >>> + LLDP_CAP_STATION)); >>> + json_array_add(chassis_capability_json, >>> + chassis_capability_item_json); >>> + } >> >> new line >> >>> + json_object_put(chassis_sys_json, "capability", >>> + chassis_capability_json); >>> + >>> + if (port->p_id_subtype == LLDP_PORTID_SUBTYPE_LLADDR) { >>> + struct ds lladdr = DS_EMPTY_INITIALIZER; >>> + if (port->p_id_len) { >> >> This is an int, so should be > 0 >> >>> + ds_put_hex_with_delimiter(&lladdr, port->p_id, >>> + port->p_id_len, ":"); >>> + } >>> + json_object_put(port_id_json, "type", >>> + json_string_create("mac")); >>> + json_object_put(port_id_json, "value", >>> + json_string_create(ds_cstr_ro(&lladdr))); >>> + ds_destroy(&lladdr); >>> + } else { >>> + struct ds port_ds = DS_EMPTY_INITIALIZER; >>> + json_object_put(port_id_json, "type", >> >> Why fixed to ifname? Other types could be parsed. > > Yes > >> >>> + json_string_create("ifname")); >>> + if (port->p_id) { >>> + ds_put_buffer(&port_ds, port->p_id, port->p_id_len); >>> + } >>> + json_object_put(port_id_json, "value", >>> + json_string_create(ds_cstr_ro(&port_ds))); >>> + ds_destroy(&port_ds); >>> + } >>> + json_object_put(port_json, "id", port_id_json); >>> + >>> + if (nullable_string_not_empty(port->p_descr)) { >>> + json_object_put(port_json, "desc", >>> + json_string_create(port->p_descr)); >>> + } >>> + >>> + json_object_put(port_json, "ttl", >>> + json_integer_create(port->p_chassis->c_ttl)); >>> + >>> + json_object_put(interface_item_json, "chassis", chassis_json); >>> + json_object_put(interface_item_json, "port", port_json); >>> + >>> + json_object_put(interface_item_warp_json, lldp->name, >>> + interface_item_json); >>> + >>> + if (has_multi_interfaces) { >>> + if (!interface_array_json) { >>> + interface_array_json = json_array_create_empty(); >>> + json_array_add(interface_array_json, >>> + interface_item_warp_json_last); >>> + } >>> + json_array_add(interface_array_json, >>> interface_item_warp_json); >>> + } else { >>> + has_multi_interfaces = true; >>> + interface_item_warp_json_last = interface_item_warp_json; >>> + } >> >> Why all this logic here (and below)? Can we not always use an array? > For existing lists, It is not easy to convert arrays. I mean, why not always use a JSON array? This would also make the JSON parser much simpler, as it wouldn’t need additional conditionals. >> Also, this is not really an interface, but a neighbor. > Yes, I will rename these vars. >> >>> + } >>> + } >>> + if (interface_array_json || interface_item_warp_json_last) { >>> + json_object_put(interface_json, "interface", >>> + interface_array_json == NULL >>> + ? interface_item_warp_json_last >>> + : interface_array_json); >>> + } >>> +} >>> + >>> static void >>> aa_unixctl_status(struct unixctl_conn *conn, int argc OVS_UNUSED, >>> const char *argv[] OVS_UNUSED, void *aux OVS_UNUSED) >>> @@ -368,6 +685,48 @@ aa_unixctl_statistics(struct unixctl_conn *conn, int >>> argc OVS_UNUSED, >>> unixctl_command_reply(conn, ds_cstr(&ds)); >>> } >>> >>> +static void >>> +lldp_unixctl_show_neighbor(struct unixctl_conn *conn, int argc, >>> + const char *argv[], void *aux OVS_UNUSED) >>> + OVS_EXCLUDED(mutex) >>> +{ >>> + struct lldp *lldp; >>> + >>> + if (unixctl_command_get_output_format(conn) == >>> UNIXCTL_OUTPUT_FMT_JSON) { >>> + struct json *interface_json = json_object_create(); >>> + struct json *lldp_json = json_object_create(); >>> + >>> + ovs_mutex_lock(&mutex); >>> + HMAP_FOR_EACH (lldp, hmap_node, all_lldps) { >>> + if (argc > 1 && strcmp(argv[1], lldp->name)) { >>> + continue; >>> + } >>> + lldp_print_neighbor_json(interface_json, lldp); >>> + } >>> + ovs_mutex_unlock(&mutex); >>> + >>> + json_object_put(lldp_json, "lldp", interface_json); >>> + >>> + unixctl_command_reply_json(conn, lldp_json); >>> + } else { >>> + struct ds ds = DS_EMPTY_INITIALIZER; >>> + >>> + ds_put_format(&ds, "LLDP neighbor:\n"); >>> + >>> + ovs_mutex_lock(&mutex); >>> + HMAP_FOR_EACH (lldp, hmap_node, all_lldps) { >>> + if (argc > 1 && strcmp(argv[1], lldp->name)) { >>> + continue; >>> + } >>> + lldp_print_neighbor(&ds, lldp); >>> + } >>> + ovs_mutex_unlock(&mutex); >>> + >>> + unixctl_command_reply(conn, ds_cstr(&ds)); >>> + ds_destroy(&ds); >>> + } >>> +} >>> + >>> /* An Auto Attach mapping was configured. Populate the corresponding >>> * structures in the LLDP hardware. >>> */ >>> @@ -635,6 +994,8 @@ lldp_init(void) >>> aa_unixctl_show_isid, NULL); >>> unixctl_command_register("autoattach/statistics", "[bridge]", 0, 1, >>> aa_unixctl_statistics, NULL); >>> + unixctl_command_register("lldp/neighbor", "[interface]", 0, 1, >>> + lldp_unixctl_show_neighbor, NULL); >>> } >>> >>> /* Returns true if 'lldp' should process packets from 'flow'. Sets >>> diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in >>> index 98e58951d..51b0a46b5 100644 >>> --- a/vswitchd/ovs-vswitchd.8.in >>> +++ b/vswitchd/ovs-vswitchd.8.in >>> @@ -149,6 +149,10 @@ enabled. >>> Force the fault status of the CFM module on \fIinterface\fR (or all >>> interfaces if none is given) to be \fIstatus\fR. \fIstatus\fR can be >>> "true", "false", or "normal" which reverts to the standard behavior. >>> +.IP "\fBlldp/neighbor\fR [\fIinterface\fR]" >>> +Displays detailed information about LLDP neighbors on \fIinterface\fR. >>> +If \fIinterface\fR is not specified, then it displays detailed information >>> +about all interfaces with LLDP enabled. >>> .IP "\fBstp/tcn\fR [\fIbridge\fR]" >>> Forces a topology change event on \fIbridge\fR if it's running STP. This >>> may cause it to send Topology Change Notifications to its peers and flush >>> -- >>> 2.43.5 >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
