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 :)
> + * 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.
> 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.
> + 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(&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.
> + : 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?
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?
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?
> + 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.
> + 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?
Also, this is not really an interface, but a neighbor.
> + }
> + }
> + 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
