On 9/1/25 10:28 AM, 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]>
> ---
> NEWS | 3 +
> lib/ovs-lldp.c | 371 ++++++++++++++++++++++++++++++++++++-
> vswitchd/ovs-vswitchd.8.in | 4 +
> 3 files changed, 377 insertions(+), 1 deletion(-)
>
Not a full review, but since I'm here and a new version will be needed
anyway for the build failure, a few more nits below.
> 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.
> + * 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..d8cf9e372 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) {
> ds_put_hex_with_delimiter(&id, port->p_chassis->c_id,
> port->p_chassis->c_id_len, ":");
> }
> @@ -310,6 +313,328 @@ 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;
> + LIST_FOR_EACH (hw, h_entries, &lldp->lldpd->g_hardware) {
> + if (!hw->h_rports.next) {
> + continue;
> + }
> + LIST_FOR_EACH (port, p_entries, &hw->h_rports) {
> + struct ds chassis_id = DS_EMPTY_INITIALIZER;
An empty line would be good here.
> + if (!port->p_chassis) {
> + continue;
> + }
> +
> + if (!is_first_interface) {
> + ds_put_format(ds, "\n");
> + }
> + is_first_interface = false;
> +
> + ds_put_format(ds, "Interface: %s\n", lldp->name);
> +
> + /* Basic TLV, Chassis ID (Type = 1). */
> + 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, ":");
> + }
> + ds_put_format(ds, " %-20s%s\n", "Chassis ID:",
> + chassis_id.length ? ds_cstr_ro(&chassis_id)
> + : none_str);
> + ds_destroy(&chassis_id);
> + /* 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);
> + } else {
> + ds_put_format(ds, " %-20s%.*s\n", "PortID:",
> + (int) (port->p_id ? port->p_id_len
> + : strlen(none_str)),
> + port->p_id ?: none_str);
> + }
> + /* 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);
The ? and : lines should start at the same level as the condition,
i.e. should be moved 4 spaces to the left in this case.
Same for all the cases below.
> + /* 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");
> + }
> + /* 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;
An empty line would be nice here.
> + 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)
> +{
> + const char *none_str = "<None>";
> + 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;
> + }
> + LIST_FOR_EACH (port, p_entries, &hw->h_rports) {
> + struct ds chassis_id = DS_EMPTY_INITIALIZER;
> + if (!port->p_chassis) {
> + continue;
> + }
> +
> + 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();
> +
> + 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(chassis_id.length
> + ? ds_cstr_ro(&chassis_id)
> + : none_str));
There is no point in putting '<None>' as an element into JSON. This would
be hard to parse as users will need to parse the string in order to check
if it's the actual MAC or the '<None>' string. Just don't put the value
at all, that will mean that it is not there. Or put the JSON null object
instead in the worst case.
But also, I'm not sure why all these type/value pairs are separate JSON
objects and not key-value pairs? I.e. why is it
"id": {"type": "mac", "value": "XXX"}
and not simply:
"id": {"mac": "XXX"}
or even (since "mac" is the only possible type here anyway):
"id": "XXX"
?
Current structure is not how JSON supposed to be written/used, IMO.
Same for all the fields below.
> + json_object_put(chassis_sys_json, "id", chassis_id_json);
> + json_object_put(chassis_json,
> +
> nullable_string_not_empty(port->p_chassis->c_name)
> + ? port->p_chassis->c_name
> + : none_str,
> + chassis_sys_json);
> + json_object_put(chassis_sys_json, "descr",
> + json_string_create(nullable_string_not_empty(
> + port->p_chassis->c_descr)
> + ? port->p_chassis->c_descr
> + : none_str));
> + ds_destroy(&chassis_id);
> +
> + struct lldpd_mgmt *mgmt;
> + LIST_FOR_EACH (mgmt, m_entries, &port->p_chassis->c_mgmt) {
> + char addr_str[INET6_ADDRSTRLEN];
> + 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;
> + }
> +
> + 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);
> + }
> + 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);
> + }
> + 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);
> + }
> + 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);
> + }
> + 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) {
> + 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(lladdr.length
> + ? ds_cstr_ro(&lladdr)
> + : none_str));
> + ds_destroy(&lladdr);
> + } else {
> + json_object_put(port_id_json, "type",
> + json_string_create("ifname"));
> + if (port->p_id) {
> + struct ds port_ds = DS_EMPTY_INITIALIZER;
> + 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);
> + } else {
> + json_object_put(port_id_json, "value",
> + json_string_create(none_str));
> + }
> + }
> + json_object_put(port_json, "id", port_id_json);
> +
> + json_object_put(
> + port_json, "desc",
> + json_string_create(nullable_string_not_empty(port->p_descr)
> + ? port->p_descr
> + : none_str));
> + 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;
> + }
> + }
> + }
> + 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 +693,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 +1002,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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
