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?
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.
>
> > + 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.
> 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
