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.
> - 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;
> + 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.
> + 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).
> + 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
