Hi, Aaron and Eelco
I'm preparing the next version patch.
Before submitting it, I'd like to discuss and confirm the final output format.
I prefer to keep the JSON format unchanged and modify the CLI format
according to Eeclo's comments in v5.
Is this okay with you?
Regards,
Changliang
On Thu, Jul 17, 2025 at 10:18 AM Changliang Wu <changliang...@smartx.com> wrote:
>
> Hi, Aaron
>
> On Wed, Jul 16, 2025 at 9:56 PM Aaron Conole <acon...@redhat.com> wrote:
> >
> > Hi Changliang,
> >
> > Changliang Wu <changliang...@smartx.com> writes:
> >
> > > New appctl 'lldp/neighbor' displays lldp neighbor information.
> > > Support json output with --format json --pretty
> > >
> > > Signed-off-by: Changliang Wu <changliang...@smartx.com>
> > > ---
> >
> > Thanks for continuing with this. It's getting closer. There are some
> > comments below. I think Eelco had thoughts on the output of the
> > commands, so please wait for his reply before submitting the v10.
> >
> > > NEWS | 1 +
> > > lib/ovs-lldp.c | 362 +++++++++++++++++++++++++++++++++++++
> > > vswitchd/ovs-vswitchd.8.in | 4 +
> > > 3 files changed, 367 insertions(+)
> > >
> > > diff --git a/NEWS b/NEWS
> > > index c7bb53f2d..931fe5c71 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -20,6 +20,7 @@ Post-v3.5.0
> > > core file size.
> > > - ovs-appctl:
> > > * Added JSON output support to the 'ovs/route/show' command.
> > > + * Added 'lldp/neighbor' command that displays lldp neighbor
> > > infomation.
> >
> > Just an FYI, there was a mid-air collision on the NEWS file, but it
> > isn't difficult to resolve.
> Yes, fix in next version, also a typo here.
> >
> > > - SSL/TLS:
> > > * Support for deprecated TLSv1 and TLSv1.1 protocols on OpenFlow and
> > > database connections is now removed.
> > > diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
> > > index 152777248..2f3f6046a 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"
> > > @@ -310,6 +313,321 @@ 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;
> > > + if (!port->p_chassis) {
> > > + continue;
> > > + }
> > > + if (is_first_interface) {
> > > + is_first_interface = false;
> > > + } else {
> > > + ds_put_format(ds, "\n");
> > > + }
> >
> > A nit - but please prefer putting the common case first:
> >
> > if (!is_first_interface) {
> > ds_put_format(ds, "\n")
> > }
> > is_first_interface = false;
> Yes, that's more clear.
> >
> > > + ds_put_format(ds, "Interface: %s\n", lldp->name);
> > > + ds_put_format(ds, " Chassis:\n");
> > > +
> > > + if (port->p_chassis->c_id_len > 0) {
> >
> > This can be:
> >
> > if (port->p_chassis->c_id_len) {
> >
> > It applies where all the len > 0 checks are present.
> Ok, I'll check all the other places.
> >
> > > + 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_put_format(ds, " %-20s%s\n", "SysName:",
> > > + strlen(port->p_chassis->c_name)
> >
> > I was preparing to apply this and just manually fix up the little nits,
> > but noticed that we only conditionally set port->p_chassis->c_name; so
> > these strlen() checks need to be prefaced with a check that c_name is
> > set.
> >
> > > + ? port->p_chassis->c_name
> > > + : none_str);
> > > + ds_put_format(ds, " %-20s%s\n", "SysDescr:",
> > > + strlen(port->p_chassis->c_descr)
> >
> > Likely here (see the below paragraph).
> >
> > > + ? 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) {
> > > + 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);
> > > + }
> > > +
> > > + 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");
> > > + }
> > > +
> > > + ds_put_format(ds, " Port:\n");
> > > +
> > > + if (port->p_id_subtype == LLDP_PORTID_SUBTYPE_LLADDR) {
> > > + struct ds lladdr = DS_EMPTY_INITIALIZER;
> > > + if (port->p_id_len > 0) {
> > > + 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);
> > > + }
> > > + ds_put_format(ds, " %-20s%s\n", "PortDescr:",
> > > + strlen(port->p_descr) ? port->p_descr :
> > > none_str);
> >
> > Likewise p_descr is conditionally initialized.
> >
> > > + ds_put_format(ds, " %-20s%d\n", "TTL:",
> > > port->p_chassis->c_ttl);
> > > + }
> > > + }
> > > +}
> > > +
> > > +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 > 0) {
> > > + 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));
> > > + json_object_put(chassis_sys_json, "id", chassis_id_json);
> > > + json_object_put(chassis_json,
> > > + strlen(port->p_chassis->c_name)
> > > + ? port->p_chassis->c_name
> > > + : none_str,
> > > + chassis_sys_json);
> >
> > Although it looks like the chassis c_name and c_descr are getting
> > initialized, in the places it gets used there are checks that it is not
> > null before actually using these variables. So I'm inclined to consider
> > that we should check here as well.
> >
> > > + json_object_put(chassis_sys_json, "descr",
> > > +
> > > json_string_create(strlen(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 > 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(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(strlen(port->p_descr)
> > > + ? port->p_descr
> > > + : none_str));
> >
> > Likewise, p_descr is conditionally initialized.
> >
> > Given the number of places we need to do these checks, an improvement
> > I would suggest is to introduce a new function (unless I'm missing one
> > already present) in util.h:
> >
> > static inline bool nullable_string_not_empty(const char *s)
> > {
> > return s && *s != '\0';
> > }
> >
> > That could also be used in lldp/lldp.c for:
> >
> > lib/lldp/lldp.c:194: if (chassis->c_name && *chassis->c_name != '\0') {
> > lib/lldp/lldp.c:201: if (chassis->c_descr && *chassis->c_descr !=
> > '\0') {
> > lib/lldp/lldp.c:234: if (port->p_descr && *port->p_descr != '\0') {
> >
> > as (ex):
> >
> > if (nullable_string_not_empty(chassis->c_name))
> > if (nullable_string_not_empty(chassis->c_descr))
> > if (nullable_string_not_empty(port->p_descr))
> >
> > If you decide to introduce the function instead of open-coding the check
> > (as done in other parts of lldp code), then please introduce it as the
> > first patch in the series just adding the nullable_string_not_empty, and
> > modifying the lib/lldp/lldp.c lines above. Then use in this patch (and
> > following patches in the series where needed).
> Ok, I will implement it in the next version.
> >
> > > + 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 +686,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 *lldp_json = json_object_create();
> > > + struct json *interface_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 +995,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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
