Hi, Aaron
On Thu, Jul 17, 2025 at 12:18 AM Aaron Conole <acon...@redhat.com> wrote:
>
> Changliang Wu <changliang...@smartx.com> writes:
>
> > The 802.1 and 802.3 standards list ways for communicating important PoE,
> > Maximum Frame Size, and other important link-level features that peers
> > may want to look at. This commit provides a port of the LLDPD project's
> > protocol decode for 802.1 and 802.3 TLVs of interest, preserving the
> > implementation as closely as possible to the source materials.
> >
> > The supported decodes will allow decoding and displaying:
> >
> > * IEEE 802.1-2005 (Port VLAN and Protocol Configuration)
> > * IEEE 802.3at (Power over Ethernet plus)
> > * IEEE 802.3bt (4-pair PoE)
> >
> > The commit supports displaying this data in human readable and json
> > formats.
> >
> > Signed-off-by: Changliang Wu <changliang...@smartx.com>
> > ---
> > NEWS | 2 +
> > lib/lldp/lldp-const.h | 5 +
> > lib/lldp/lldp.c | 164 +++++++++++++++++++-
> > lib/lldp/lldpd-structs.c | 31 ++++
> > lib/lldp/lldpd-structs.h | 69 +++++++++
> > lib/ovs-lldp.c | 316 +++++++++++++++++++++++++++++++++++++++
> > 6 files changed, 585 insertions(+), 2 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 931fe5c71..d46f78743 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -21,6 +21,8 @@ Post-v3.5.0
> > - ovs-appctl:
> > * Added JSON output support to the 'ovs/route/show' command.
> > * Added 'lldp/neighbor' command that displays lldp neighbor
> > infomation.
> > + - lldp:
> > + * Added support for decoding dot1 and dot3 TLVs.
> > - SSL/TLS:
> > * Support for deprecated TLSv1 and TLSv1.1 protocols on OpenFlow and
> > database connections is now removed.
> > diff --git a/lib/lldp/lldp-const.h b/lib/lldp/lldp-const.h
> > index 8c5c0733e..e03c77f71 100644
> > --- a/lib/lldp/lldp-const.h
> > +++ b/lib/lldp/lldp-const.h
> > @@ -98,6 +98,11 @@
> > #define LLDP_DOT3_POWER_8023AT_TYPE1 1
> > #define LLDP_DOT3_POWER_8023AT_TYPE2 2
> >
> > +/* 802.3bt additions */
> > +#define LLDP_DOT3_POWER_8023BT_OFF 0
> > +#define LLDP_DOT3_POWER_8023BT_TYPE3 1
> > +#define LLDP_DOT3_POWER_8023BT_TYPE4 2
> > +
> > /* Dot3 power source */
> > #define LLDP_DOT3_POWER_SOURCE_UNKNOWN 0
> > #define LLDP_DOT3_POWER_SOURCE_PRIMARY 1
> > diff --git a/lib/lldp/lldp.c b/lib/lldp/lldp.c
> > index 6fdcfef56..c10bb27d9 100644
> > --- a/lib/lldp/lldp.c
> > +++ b/lib/lldp/lldp.c
> > @@ -18,6 +18,8 @@
> > */
> >
> > #include <config.h>
> > +#include "lldp-const.h"
> > +#include "lldp-tlv.h"
> > #include "lldpd.h"
> > #include <errno.h>
> > #include <time.h>
> > @@ -372,6 +374,12 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame,
> > int s,
> > void *b;
> > struct lldpd_aa_isid_vlan_maps_tlv *isid_vlan_map = NULL;
> > u_int8_t msg_auth_digest[LLDP_TLV_AA_ISID_VLAN_DIGEST_LENGTH];
> > +
> > + struct lldpd_vlan *vlan = NULL;
> > + int vlan_len;
> > + struct lldpd_ppvid *ppvid;
> > + struct lldpd_pi *pi = NULL;
> > +
>
> As before, please prefer reverse xmas tree for initialization. It isn't
> a hard rule, but since this series will need a respin, please do it
> in the next version.
Ok, I'll check all the other places.
By the way, the current coding style is as described in the document.
Is it possible to use clang-format or similar to format and verify?
>
> > struct lldpd_mgmt *mgmt;
> > u_int8_t addr_str_length, addr_str_buffer[32];
> > u_int8_t addr_family, addr_length, *addr_ptr, iface_subtype;
> > @@ -384,6 +392,9 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char *frame,
> > int s,
> >
> > port = xzalloc(sizeof *port);
> > ovs_list_init(&port->p_isid_vlan_maps);
> > + ovs_list_init(&port->p_vlans);
> > + ovs_list_init(&port->p_ppvids);
> > + ovs_list_init(&port->p_pids);
> >
> > length = s;
> > pos = (u_int8_t*) frame;
> > @@ -568,9 +579,158 @@ lldp_decode(struct lldpd *cfg OVS_UNUSED, char
> > *frame, int s,
> > PEEK_BYTES(orgid, sizeof orgid);
> > tlv_subtype = PEEK_UINT8;
> > if (memcmp(dot1, orgid, sizeof orgid) == 0) {
> > - hardware->h_rx_unrecognized_cnt++;
> > + switch (tlv_subtype) {
> > + case LLDP_TLV_DOT1_VLANNAME:
> > + CHECK_TLV_SIZE(7, "VLAN");
> > + vlan = xzalloc(sizeof *vlan);
> > + vlan->v_vid = PEEK_UINT16;
> > + vlan_len = PEEK_UINT8;
> > + CHECK_TLV_SIZE(7 + vlan_len, "VLAN");
> > + vlan->v_name = xzalloc(vlan_len + 1);
> > + PEEK_BYTES(vlan->v_name, vlan_len);
> > + ovs_list_push_back(&port->p_vlans, &vlan->v_entries);
> > + vlan = NULL;
> > + break;
> > +
> > + case LLDP_TLV_DOT1_PVID:
> > + CHECK_TLV_SIZE(6, "PVID");
> > + port->p_pvid = PEEK_UINT16;
> > + break;
> > +
> > + case LLDP_TLV_DOT1_PPVID:
> > + CHECK_TLV_SIZE(7, "PPVID");
> > + /* validation needed */
> > + /* PPVID has to be unique if more than
> > + one PPVID TLVs are received -
> > + discard if duplicate */
> > + /* if support bit is not set and
> > + enabled bit is set - PPVID TLV is
> > + considered error and discarded */
> > + /* if PPVID > 4096 - bad and discard */
> > + ppvid = xzalloc(sizeof *ppvid);
> > + ppvid->p_cap_status = PEEK_UINT8;
> > + ppvid->p_ppvid = PEEK_UINT16;
> > + ovs_list_push_back(&port->p_ppvids, &ppvid->p_entries);
> > + break;
> > +
> > + case LLDP_TLV_DOT1_PI:
> > + /* validation needed */
> > + /* PI has to be unique if more than
> > + one PI TLVs are received - discard
> > + if duplicate ?? */
> > + pi = xzalloc(sizeof *pi);
> > + pi->p_pi_len = PEEK_UINT8;
> > + CHECK_TLV_SIZE(5 + pi->p_pi_len, "PI");
> > + pi->p_pi = xzalloc(pi->p_pi_len);
> > + PEEK_BYTES(pi->p_pi, pi->p_pi_len);
> > + ovs_list_push_back(&port->p_pids, &pi->p_entries);
> > + pi = NULL;
> > + break;
> > +
> > + default:
> > + VLOG_INFO("unknown org tlv [%02x:%02x:%02x] received "
> > + "on %s",
> > + orgid[0], orgid[1], orgid[2],
> > + hardware->h_ifname);
> > + hardware->h_rx_unrecognized_cnt++;
> > + }
> > } else if (memcmp(dot3, orgid, sizeof orgid) == 0) {
> > - hardware->h_rx_unrecognized_cnt++;
> > + switch (tlv_subtype) {
> > + case LLDP_TLV_DOT3_MAC:
> > + CHECK_TLV_SIZE(9, "MAC/PHY");
> > + port->p_macphy.autoneg_support = PEEK_UINT8;
> > + port->p_macphy.autoneg_enabled =
> > + (port->p_macphy.autoneg_support & 0x2) >> 1;
> > + port->p_macphy.autoneg_support =
> > + port->p_macphy.autoneg_support & 0x1;
> > + port->p_macphy.autoneg_advertised = PEEK_UINT16;
> > + port->p_macphy.mau_type = PEEK_UINT16;
> > + break;
> > +
> > + case LLDP_TLV_DOT3_LA:
> > + CHECK_TLV_SIZE(9, "Link aggregation");
> > + PEEK_DISCARD_UINT8;
> > + port->p_aggregid = PEEK_UINT32;
> > + break;
> > +
> > + case LLDP_TLV_DOT3_MFS:
> > + CHECK_TLV_SIZE(6, "MFS");
> > + port->p_mfs = PEEK_UINT16;
> > + break;
> > +
> > + case LLDP_TLV_DOT3_POWER:
> > + CHECK_TLV_SIZE(7, "Power");
> > + port->p_power.devicetype = PEEK_UINT8;
> > + port->p_power.supported =
> > + (port->p_power.devicetype & 0x2) >> 1;
> > + port->p_power.enabled = (port->p_power.devicetype &
> > 0x4) >>
> > + 2;
> > + port->p_power.paircontrol =
> > + (port->p_power.devicetype & 0x8) >> 3;
> > + port->p_power.devicetype = (port->p_power.devicetype &
> > 0x1)
> > + ? LLDP_DOT3_POWER_PSE
> > + : LLDP_DOT3_POWER_PD;
> > + port->p_power.pairs = PEEK_UINT8;
> > + port->p_power.class = PEEK_UINT8;
> > + /* 802.3at? */
> > + if (tlv_size >= 12) {
> > + port->p_power.powertype = PEEK_UINT8;
> > + port->p_power.source =
> > + (port->p_power.powertype & (1 << 5 | 1 << 4))
> > >> 4;
> > + port->p_power.priority =
> > + (port->p_power.powertype & (1 << 1 | 1 << 0));
> > + port->p_power.powertype =
> > + (port->p_power.powertype & (1 << 7))
> > + ? LLDP_DOT3_POWER_8023AT_TYPE1
> > + : LLDP_DOT3_POWER_8023AT_TYPE2;
> > + port->p_power.requested = PEEK_UINT16;
> > + port->p_power.allocated = PEEK_UINT16;
> > + } else {
> > + port->p_power.powertype =
> > LLDP_DOT3_POWER_8023AT_OFF;
> > + }
> > + /* 802.3bt? */
> > + if (tlv_size >= 29) {
> > + port->p_power.requested_a = PEEK_UINT16;
> > + port->p_power.requested_b = PEEK_UINT16;
> > + port->p_power.allocated_a = PEEK_UINT16;
> > + port->p_power.allocated_b = PEEK_UINT16;
> > + port->p_power.pse_status = PEEK_UINT16;
> > + port->p_power.pd_status =
> > + (port->p_power.pse_status & (1 << 13 | 1 <<
> > 12)) >>
> > + 12;
> > + port->p_power.pse_pairs_ext =
> > + (port->p_power.pse_status & (1 << 11 | 1 <<
> > 10)) >>
> > + 10;
> > + port->p_power.class_a = (port->p_power.pse_status &
> > + (1 << 9 | 1 << 8 | 1 <<
> > 7)) >>
> > + 7;
> > + port->p_power.class_b = (port->p_power.pse_status &
> > + (1 << 6 | 1 << 5 | 1 <<
> > 4)) >>
> > + 4;
> > + port->p_power.class_ext =
> > + (port->p_power.pse_status & 0xf);
> > + port->p_power.pse_status =
> > + (port->p_power.pse_status & (1 << 15 | 1 <<
> > 14)) >>
> > + 14;
> > + port->p_power.type_ext = PEEK_UINT8;
> > + port->p_power.pd_load = (port->p_power.type_ext &
> > 0x1);
> > + port->p_power.type_ext =
> > + ((port->p_power.type_ext &
> > + (1 << 3 | 1 << 2 | 1 << 1)) +
> > + 1);
> > + port->p_power.pse_max = PEEK_UINT16;
> > + } else {
> > + port->p_power.type_ext =
> > LLDP_DOT3_POWER_8023BT_OFF;
> > + }
> > + break;
> > +
> > + default:
> > + VLOG_INFO("unknown org tlv [%02x:%02x:%02x] received "
> > + "on %s",
> > + orgid[0], orgid[1], orgid[2],
> > + hardware->h_ifname);
> > + hardware->h_rx_unrecognized_cnt++;
> > + }
> > } else if (memcmp(med, orgid, sizeof orgid) == 0) {
> > /* LLDP-MED */
> > hardware->h_rx_unrecognized_cnt++;
> > diff --git a/lib/lldp/lldpd-structs.c b/lib/lldp/lldpd-structs.c
> > index a8c7fad09..9e1215293 100644
> > --- a/lib/lldp/lldpd-structs.c
> > +++ b/lib/lldp/lldpd-structs.c
> > @@ -113,6 +113,35 @@ lldpd_aa_maps_cleanup(struct lldpd_port *port)
> > }
> > }
> >
> > +static void
> > +lldpd_dot13_lists_cleanup(struct lldpd_port *port)
> > +{
> > + struct lldpd_vlan *vlan;
> > + struct lldpd_ppvid *ppvid;
> > + struct lldpd_pi *pid;
> > + if (port->p_vlans.next && !ovs_list_is_empty(&port->p_vlans)) {
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;
}
> > + LIST_FOR_EACH_SAFE (vlan, v_entries, &port->p_vlans) {
> > + ovs_list_remove(&vlan->v_entries);
> > + free(vlan);
>
> I think we need to release the v_name element here, right?
Yes.
>
> > + }
> > + ovs_list_init(&port->p_vlans);
> > + }
> > + if (port->p_ppvids.next && !ovs_list_is_empty(&port->p_ppvids)) {
> > + LIST_FOR_EACH_SAFE (ppvid, p_entries, &port->p_ppvids) {
> > + ovs_list_remove(&ppvid->p_entries);
> > + free(ppvid);
> > + }
> > + ovs_list_init(&port->p_ppvids);
> > + }
> > + if (port->p_pids.next && !ovs_list_is_empty(&port->p_pids)) {
> > + LIST_FOR_EACH_SAFE (pid, p_entries, &port->p_pids) {
> > + ovs_list_remove(&pid->p_entries);
> > + free(pid);
>
> Likewise, we need to delete the p_pi element.
Yes.
>
> > + }
> > + ovs_list_init(&port->p_pids);
> > + }
> > +}
> > +
> > /* If `all' is true, clear all information, including information that
> > are not refreshed periodically. Port should be freed manually. */
> > void
> > @@ -127,6 +156,8 @@ lldpd_port_cleanup(struct lldpd_port *port, bool all)
> > /* Cleanup auto-attach mappings */
> > lldpd_aa_maps_cleanup(port);
> >
> > + lldpd_dot13_lists_cleanup(port);
> > +
> > if (all) {
> > free(port->p_lastframe);
> > /* Chassis may not have been attributed, yet.*/
> > diff --git a/lib/lldp/lldpd-structs.h b/lib/lldp/lldpd-structs.h
> > index fe5d5f9f8..1ec7fe777 100644
> > --- a/lib/lldp/lldpd-structs.h
> > +++ b/lib/lldp/lldpd-structs.h
> > @@ -45,6 +45,63 @@ lldpd_af(int af)
> > }
> > }
> >
> > +struct lldpd_ppvid {
> > + struct ovs_list p_entries;
> > + u_int8_t p_cap_status;
> > + u_int16_t p_ppvid;
>
> NOT: please space the variables at a consistent whitespace.
OK
>
> struct lldpd_ppvid {
> struct ovs_list p_entries;
> u_int8_t p_cap_status;
> u_int16_t p_ppvid;
> };
>
> struct lldpd_vlan {
> struct ovs_list v_entries;
> char *v_name;
> u_int16_6 v_vid;
> };
>
> etc. I also suggest adding a brief description of the field.
OK
>
> > +};
> > +
> > +struct lldpd_vlan {
> > + struct ovs_list v_entries;
> > + char *v_name;
> > + u_int16_t v_vid;
> > +};
> > +
> > +struct lldpd_pi {
> > + struct ovs_list p_entries;
> > + char *p_pi;
> > + int p_pi_len;
> > +};
> > +
> > +struct lldpd_dot3_macphy {
> > + u_int8_t autoneg_support;
> > + u_int8_t autoneg_enabled;
> > + u_int16_t autoneg_advertised;
> > + u_int16_t mau_type;
> > +};
> > +
> > +struct lldpd_dot3_power {
> > + u_int8_t devicetype;
> > + u_int8_t supported;
> > + u_int8_t enabled;
> > + u_int8_t paircontrol;
> > + u_int8_t pairs;
> > + u_int8_t class;
> > + u_int8_t powertype; /* If set to LLDP_DOT3_POWER_8023AT_OFF,
> > + following fields have no meaning */
>
> NIT: please make the comment spacing here consistent. For multiline
> comments, we prefer:
>
> /* Comment
> * line 2 */
>
> Also please end the comments with a '.'
OK
>
> > + u_int8_t source;
> > + u_int8_t priority;
> > + u_int16_t requested;
> > + u_int16_t allocated;
> > +
> > + /* For 802.3BT */
> > + u_int8_t pd_4pid;
> > + u_int16_t requested_a;
> > + u_int16_t requested_b;
> > + u_int16_t allocated_a;
> > + u_int16_t allocated_b;
> > + u_int16_t pse_status;
> > + u_int8_t pd_status;
> > + u_int8_t pse_pairs_ext;
> > + u_int8_t class_a;
> > + u_int8_t class_b;
> > + u_int8_t class_ext;
> > + u_int8_t type_ext;
> > + u_int8_t pd_load;
> > + u_int16_t pse_max;
> > +};
> > +
> > +
> > #define LLDPD_MGMT_MAXADDRSIZE 16 /* sizeof(struct in6_addr) */
> > struct lldpd_mgmt {
> > struct ovs_list m_entries;
> > @@ -98,6 +155,18 @@ struct lldpd_port {
> > char *p_descr;
> > u_int16_t p_mfs;
> > struct lldpd_aa_element_tlv p_element;
> > +
> > + /* Dot3 stuff */
> > + u_int32_t p_aggregid;
> > + struct lldpd_dot3_macphy p_macphy;
> > + struct lldpd_dot3_power p_power;
> > +
> > + /* Dot1 stuff */
> > + u_int16_t p_pvid;
> > + struct ovs_list p_vlans; /* Contains "struct lldpd_vlan"s. */
> > + struct ovs_list p_ppvids; /* Contains "struct lldpd_ppvid"s. */
> > + struct ovs_list p_pids; /* Contains "struct lldpd_pi"s. */
> > +
> > struct ovs_list p_isid_vlan_maps; /* Contains "struct
> > lldpd_aa_isid_vlan_maps_tlv"s. */
> > };
> >
> > diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
> > index 2f3f6046a..ff153fc7e 100644
> > --- a/lib/ovs-lldp.c
> > +++ b/lib/ovs-lldp.c
> > @@ -313,6 +313,316 @@ aa_print_isid_status(struct ds *ds, struct lldp
> > *lldp) OVS_REQUIRES(mutex)
> > }
> > }
> >
> > +static void
> > +lldp_print_neighbor_port_dot1(struct ds *ds, struct lldpd_port *port)
> > +{
> > + struct lldpd_vlan *vlan;
> > + struct lldpd_ppvid *ppvid;
> > + struct lldpd_pi *pid;
>
> Same comment about reverse xmas tree
OK
>
> > +
> > + LIST_FOR_EACH (vlan, v_entries, &port->p_vlans) {
> > + ds_put_format(ds, " %-20s%d, %s", "VLAN:", vlan->v_vid,
> > + port->p_pvid == vlan->v_vid ? "pvid: yes" : "pvid:
> > no");
> > + if (vlan->v_name) {
> > + ds_put_format(ds, ", %s", vlan->v_name);
> > + }
> > + ds_put_format(ds, "\n");
> > + }
> > +
> > + LIST_FOR_EACH (ppvid, p_entries, &port->p_ppvids) {
> > + ds_put_format(ds, " %-20s", "PPVID:");
> > + if (ppvid->p_ppvid) {
> > + ds_put_format(ds, ", %d ", ppvid->p_ppvid);
> > + }
> > + ds_put_format(
> > + ds, "supported: %s,enabled %s\n",
> > + (ppvid->p_cap_status & LLDP_PPVID_CAP_SUPPORTED) ? "yes" :
> > "no",
> > + (ppvid->p_cap_status & LLDP_PPVID_CAP_ENABLED) ? "yes" : "no");
> > + }
> > +
> > + LIST_FOR_EACH (pid, p_entries, &port->p_pids) {
> > + if (pid->p_pi && pid->p_pi_len > 0) {
> > + ds_put_format(ds, " %-20s", "PI:");
> > + ds_put_buffer(ds, pid->p_pi, pid->p_pi_len);
> > + ds_put_format(ds, "\n");
> > + }
> > + }
> > +}
> > +
> > +static void
> > +lldp_print_neighbor_port_dot1_json(struct json *interface_item_json,
> > + struct lldpd_port *port)
> > +{
> > + struct json *vlan_json_array = NULL;
> > + struct json *vlan_json = NULL;
> > + struct json *ppvid_json_array = NULL;
> > + struct json *ppvid_json = NULL;
> > + struct json *pi_json_array = NULL;
> > + struct json *pi_json = NULL;
> > +
> > + struct lldpd_vlan *vlan;
> > + struct lldpd_ppvid *ppvid;
> > + struct lldpd_pi *pid;
> > +
> > + LIST_FOR_EACH (vlan, v_entries, &port->p_vlans) {
> > + vlan_json = json_object_create();
> > + json_object_put(vlan_json, "vlan-id",
> > + json_integer_create(vlan->v_vid));
> > + json_object_put(vlan_json, "pvid",
> > + json_boolean_create(port->p_pvid == vlan->v_vid));
> > + if (vlan->v_name) {
> > + json_object_put(vlan_json, "value",
> > + json_string_create(vlan->v_name));
> > + }
> > + if (ovs_list_size(&port->p_vlans) > 1) {
> > + if (vlan_json_array == NULL) {
> > + vlan_json_array = json_array_create_empty();
> > + }
> > + json_array_add(vlan_json_array, vlan_json);
> > + }
> > + }
> > + if (vlan_json_array || vlan_json) {
> > + json_object_put(interface_item_json, "vlan",
> > + vlan_json_array ? vlan_json_array : vlan_json);
> > + }
> > +
> > + LIST_FOR_EACH (ppvid, p_entries, &port->p_ppvids) {
> > + ppvid_json = json_object_create();
> > + if (ppvid->p_ppvid) {
> > + json_object_put(ppvid_json, "ppvid",
> > + json_integer_create(ppvid->p_ppvid));
> > + }
> > + json_object_put(ppvid_json, "supported",
> > + json_boolean_create(ppvid->p_cap_status &
> > + LLDP_PPVID_CAP_SUPPORTED));
> > + json_object_put(ppvid_json, "enabled",
> > + json_boolean_create(ppvid->p_cap_status &
> > + LLDP_PPVID_CAP_ENABLED));
> > + if (ovs_list_size(&port->p_ppvids) > 1) {
> > + if (ppvid_json_array == NULL) {
> > + ppvid_json_array = json_array_create_empty();
> > + }
> > + json_array_add(ppvid_json_array, ppvid_json);
> > + }
> > + }
> > + if (ppvid_json_array || ppvid_json) {
> > + json_object_put(interface_item_json, "ppvid",
> > + ppvid_json_array ? ppvid_json_array : ppvid_json);
> > + }
> > +
> > + LIST_FOR_EACH (pid, p_entries, &port->p_pids) {
> > + if (pid->p_pi && pid->p_pi_len > 0) {
> > + pi_json = json_object_create();
> > + struct ds pi_ds = DS_EMPTY_INITIALIZER;
>
> pi_ds can be moved above the pi_json.
OK
>
> > + ds_put_buffer(&pi_ds, pid->p_pi, pid->p_pi_len);
> > + json_object_put(ppvid_json, "ppvid",
> > + json_string_create(ds_cstr_ro(&pi_ds)));
> > + ds_destroy(&pi_ds);
> > + if (ovs_list_size(&port->p_pids) > 1) {
> > + if (pi_json_array == NULL) {
> > + pi_json_array = json_array_create_empty();
> > + }
> > + json_array_add(pi_json_array, pi_json);
> > + }
> > + }
> > + }
> > + if (pi_json_array || pi_json) {
> > + json_object_put(interface_item_json, "pi",
> > + pi_json_array ? pi_json_array : pi_json);
> > + }
> > +}
> > +
> > +static void
> > +lldp_dot3_autoneg_advertised(struct ds *ds, uint16_t pmd_auto_nego, int
> > bithd,
> > + int bitfd, const char *type)
> > +{
> > + if (!((pmd_auto_nego & bithd) || (pmd_auto_nego & bitfd))) {
> > + return;
> > + }
> > +
> > + ds_put_format(ds, " %-18s%s", "Adv:", type);
> > + if (bithd != bitfd) {
> > + ds_put_format(ds, ", HD: %s, FD: %s\n",
> > + (pmd_auto_nego & bithd) ? "yes" : "no",
> > + (pmd_auto_nego & bitfd) ? "yes" : "no");
> > + }
> > +}
> > +
> > +static void
> > +lldp_dot3_autoneg_advertised_json(struct json **advertised_json,
> > + uint16_t pmd_auto_nego, int bithd, int
> > bitfd,
> > + const char *type)
> > +{
> > + if (!((pmd_auto_nego & bithd) || (pmd_auto_nego & bitfd))) {
> > + return;
> > + }
> > +
> > + if (!*advertised_json) {
> > + *advertised_json = json_array_create_empty();
> > + }
> > +
> > + struct json *advertised_item_json = json_object_create();
> > + json_object_put(advertised_item_json, "type",
> > json_string_create(type));
> > + if (bithd != bitfd) {
> > + json_object_put(advertised_item_json, "hd",
> > + json_boolean_create((pmd_auto_nego & bithd) ? true
> > + :
> > false));
> > + json_object_put(advertised_item_json, "fd",
> > + json_boolean_create((pmd_auto_nego & bitfd) ? true
> > + :
> > false));
> > + }
> > + json_array_add(*advertised_json, advertised_item_json);
> > +}
> > +
> > +static void
> > +lldp_print_neighbor_port_dot3(struct ds *ds, struct lldpd_port *port)
> > +{
> > + if (port->p_mfs) {
> > + ds_put_format(ds, " %-20s%d\n", "MFS:", port->p_mfs);
> > + }
> > +
> > + long int autoneg_support, autoneg_enabled, autoneg_advertised, mautype;
> > + autoneg_support = port->p_macphy.autoneg_support;
> > + autoneg_enabled = port->p_macphy.autoneg_enabled;
> > + autoneg_advertised = port->p_macphy.autoneg_advertised;
> > + mautype = port->p_macphy.mau_type;
>
> I suggest something like:
>
> uint16_t autoneg_advertised = port->p_macphy.autoneg_advertised,
> autoneg_support = port->p_macphy.autoneg_support,
> autoneg_enabled = port->p_macphy.autoneg_enabled,
> mautype = port->p_macphytype.mau_type;
>
> To keep the declaration and assignments together. This can be moved
> above the 'if (port->p_mfs)' check.
OK
>
> > + if (autoneg_support > 0 || autoneg_enabled > 0 || mautype > 0) {
>
> That also means we can just test:
>
> if (autoneg_support || autoneg_enabled || mautype)
>
> > + ds_put_format(ds, " PMD autoneg: supported: %s, enabled: %s\n",
> > + autoneg_support ? "yes" : "no",
> > + autoneg_enabled ? "yes" : "no");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_10BASE_T,
> > + LLDP_DOT3_LINK_AUTONEG_10BASET_FD,
> > + "10Base-T");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_100BASE_T4,
> > + LLDP_DOT3_LINK_AUTONEG_100BASE_T4,
> > + "100Base-T4");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_100BASE_TX,
> > + LLDP_DOT3_LINK_AUTONEG_100BASE_TXFD,
> > + "100Base-TX");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_100BASE_T2,
> > + LLDP_DOT3_LINK_AUTONEG_100BASE_T2FD,
> > + "100Base-T2");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_1000BASE_X,
> > + LLDP_DOT3_LINK_AUTONEG_1000BASE_XFD,
> > + "1000Base-X");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_1000BASE_T,
> > + LLDP_DOT3_LINK_AUTONEG_1000BASE_TFD,
> > + "1000Base-T");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE,
> > + "FDX_PAUSE");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE,
> > + "FDX_APAUSE");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE,
> > + "FDX_SPAUSE");
> > + lldp_dot3_autoneg_advertised(ds, autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE,
> > + "FDX_BPAUSE");
> > + ds_put_format(ds, " %-18s%ld\n", "MAU oper type:", mautype);
> > + }
> > +
> > + if (port->p_power.devicetype) {
> > + ds_put_format(ds,
> > + " MDI Power: supported: %s, enabled: %s, pair "
> > + "control: %s\n",
> > + port->p_power.supported ? "yes" : "no",
> > + port->p_power.enabled ? "yes" : "no",
> > + port->p_power.paircontrol ? "yes" : "no");
> > + }
> > +}
> > +
> > +static void
> > +lldp_print_neighbor_port_dot3_json(struct json *port_json,
> > + struct lldpd_port *port)
> > +{
> > + if (port->p_mfs) {
> > + json_object_put(port_json, "mfs",
> > json_integer_create(port->p_mfs));
> > + }
> > +
> > + long int autoneg_support, autoneg_enabled, autoneg_advertised, mautype;
> > + autoneg_support = port->p_macphy.autoneg_support;
> > + autoneg_enabled = port->p_macphy.autoneg_enabled;
> > + autoneg_advertised = port->p_macphy.autoneg_advertised;
> > + mautype = port->p_macphy.mau_type;
>
> Same comments as before.
>
> > + if (autoneg_support > 0 || autoneg_enabled > 0 || mautype > 0) {
> > + struct json *auto_nego_json = json_object_create();
> > + json_object_put(auto_nego_json, "supported",
> > + json_boolean_create(autoneg_support));
> > + json_object_put(auto_nego_json, "enabled",
> > + json_boolean_create(autoneg_enabled));
> > +
> > + struct json *advertised_json = NULL;
>
> Please move this with the auto_nego_json declaration and put whitespace
> between the declarations and the rest of the control blocks.
OK
>
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_10BASE_T,
> > +
> > LLDP_DOT3_LINK_AUTONEG_10BASET_FD,
> > + "10Base-T");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > +
> > LLDP_DOT3_LINK_AUTONEG_100BASE_T4,
> > +
> > LLDP_DOT3_LINK_AUTONEG_100BASE_T4,
> > + "100Base-T4");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > +
> > LLDP_DOT3_LINK_AUTONEG_100BASE_TX,
> > +
> > LLDP_DOT3_LINK_AUTONEG_100BASE_TXFD,
> > + "100Base-TX");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > +
> > LLDP_DOT3_LINK_AUTONEG_100BASE_T2,
> > +
> > LLDP_DOT3_LINK_AUTONEG_100BASE_T2FD,
> > + "100Base-T2");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > +
> > LLDP_DOT3_LINK_AUTONEG_1000BASE_X,
> > +
> > LLDP_DOT3_LINK_AUTONEG_1000BASE_XFD,
> > + "1000Base-X");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > +
> > LLDP_DOT3_LINK_AUTONEG_1000BASE_T,
> > +
> > LLDP_DOT3_LINK_AUTONEG_1000BASE_TFD,
> > + "1000Base-T");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE,
> > + LLDP_DOT3_LINK_AUTONEG_FDX_PAUSE,
> > + "FDX_PAUSE");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > +
> > LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE,
> > +
> > LLDP_DOT3_LINK_AUTONEG_FDX_APAUSE,
> > + "FDX_APAUSE");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > +
> > LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE,
> > +
> > LLDP_DOT3_LINK_AUTONEG_FDX_SPAUSE,
> > + "FDX_SPAUSE");
> > + lldp_dot3_autoneg_advertised_json(&advertised_json,
> > autoneg_advertised,
> > +
> > LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE,
> > +
> > LLDP_DOT3_LINK_AUTONEG_FDX_BPAUSE,
> > + "FDX_BPAUSE");
> > + json_object_put(auto_nego_json, "current",
> > + json_integer_create(mautype));
> > +
> > + json_object_put(port_json, "auto-negotiation", auto_nego_json);
> > + }
> > +
> > + if (port->p_power.devicetype) {
> > + struct json *power_json = json_object_create();
> > +
> > + json_object_put(power_json, "supported",
> > + json_boolean_create(port->p_power.supported));
> > + json_object_put(power_json, "enabled",
> > + json_boolean_create(port->p_power.enabled));
> > + json_object_put(power_json, "paircontrol",
> > + json_boolean_create(port->p_power.paircontrol));
> > + json_object_put(port_json, "power", power_json);
> > + }
> > +}
> > +
> > static void
> > lldp_print_neighbor(struct ds *ds, struct lldp *lldp) OVS_REQUIRES(mutex)
> > {
> > @@ -426,6 +736,9 @@ lldp_print_neighbor(struct ds *ds, struct lldp *lldp)
> > OVS_REQUIRES(mutex)
> > ds_put_format(ds, " %-20s%s\n", "PortDescr:",
> > strlen(port->p_descr) ? port->p_descr :
> > none_str);
> > ds_put_format(ds, " %-20s%d\n", "TTL:",
> > port->p_chassis->c_ttl);
> > +
> > + lldp_print_neighbor_port_dot3(ds, port);
> > + lldp_print_neighbor_port_dot1(ds, port);
> > }
> > }
> > }
> > @@ -601,6 +914,9 @@ lldp_print_neighbor_json(struct json *interface_json,
> > struct lldp *lldp)
> > json_object_put(port_json, "ttl",
> > json_integer_create(port->p_chassis->c_ttl));
> >
> > + lldp_print_neighbor_port_dot1_json(interface_item_json, port);
> > + lldp_print_neighbor_port_dot3_json(port_json, port);
> > +
> > json_object_put(interface_item_json, "chassis", chassis_json);
> > json_object_put(interface_item_json, "port", port_json);
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
