[lldp-devel] [PATCH 2/8] introduce support for multiple agents per port

John Fastabend john.r.fastabend at intel.com
Mon Aug 15 16:06:55 UTC 2011


On 8/15/2011 5:57 AM, Jens Osterkamp wrote:
> On Wednesday 10 August 2011, John Fastabend wrote:
>> On 8/5/2011 8:35 AM, Jens Osterkamp wrote:
>>> This patch introduces support for multiple agents per port.
>>> Each agent sends and receives frames to and from different group mac
>>> addresses. Right now these are nearest bridge, nearest customer bridge and
>>> nearest non-twoport mac-relaying bridge.
>>>
>>> It splits out most of the agent specific variables from the port structure
>>> to an agent structure. This requires changes in the rx and tx state
>>> machines.
>>>
>>> With the changes for the multi-agent support, adminStatus has moved from the
>>> port to the agent. As port->adminStatus has been used to control the VDP
>>> state machines as well, a new mechanism was needed here.
>>> The VDP module does not have agent support, so port->adminStatus is replaced
>>> by the vdp->enableTx flag. If this is set to yes by lldptool, VDP will be
>>> active, and will be inactive if set to no.
>>>
>>> Signed-off-by: Jens Osterkamp <jens at linux.vnet.ibm.com>
>>
>> Some minor nits and one null check needed see comments below.
> 
> Hi John,
> 
> thank you for your thorough review !
> Please see my comments below.
> 

Few comments to your comments inline. I mostly just agreed
with you. Looking forward to seeing the next version.

Thanks,
John.

>>
>>> ---
>>>  config.c               |  115 ++++++++++--
>>>  dcb_protocol.c         |    9 +-
>>>  ecp/ecp.h              |    6 +-
>>>  ecp/ecp_rx.c           |   15 +-
>>>  ecp/ecp_tx.c           |   14 +-
>>>  event_iface.c          |   45 +++--
>>>  include/clif_msgs.h    |    2 +
>>>  include/config.h       |    8 +-
>>>  include/lldp.h         |    2 +-
>>>  include/lldp_8021qaz.h |   11 +-
>>>  include/lldp_8023.h    |    7 +-
>>>  include/lldp_basman.h  |    7 +-
>>>  include/lldp_dcbx.h    |   10 +-
>>>  include/lldp_evb.h     |    9 +-
>>>  include/lldp_mand.h    |    7 +-
>>>  include/lldp_med.h     |    7 +-
>>>  include/lldp_mod.h     |   12 +-
>>>  include/lldp_vdp.h     |    1 +
>>>  include/tlv_dcbx.h     |   14 +-
>>>  lldp/agent.c           |  159 ++++++++++++++--
>>>  lldp/agent.h           |  143 ++++++++++++++-
>>>  lldp/l2_packet.h       |    9 +-
>>>  lldp/l2_packet_linux.c |   16 +-
>>>  lldp/ports.c           |  285 +++++++++++++---------------
>>>  lldp/ports.h           |  108 ++---------
>>>  lldp/rx.c              |  496 ++++++++++++++++++++++++------------------------
>>>  lldp/states.h          |   57 +++---
>>>  lldp/tx.c              |  327 +++++++++++++++++---------------
>>>  lldp_8021qaz.c         |  134 ++++++++-----
>>>  lldp_8021qaz_cmds.c    |   16 +-
>>>  lldp_8023.c            |   22 ++-
>>>  lldp_8023_cmds.c       |    2 +-
>>>  lldp_basman.c          |   19 +-
>>>  lldp_basman_cmds.c     |    6 +-
>>>  lldp_dcbx.c            |   82 ++++----
>>>  lldp_dcbx_cmds.c       |   16 +-
>>>  lldp_evb.c             |   36 ++--
>>>  lldp_evb_cmds.c        |   26 ++--
>>>  lldp_mand.c            |   33 ++--
>>>  lldp_mand_cmds.c       |   19 ++-
>>>  lldp_med.c             |   19 +-
>>>  lldp_med_cmds.c        |    4 +-
>>>  lldp_tlv.c             |   17 +-
>>>  lldp_vdp.c             |   43 ++--
>>>  lldpad.c               |   11 +-
>>>  tlv_dcbx.c             |  133 ++++++++------
>>>  46 files changed, 1455 insertions(+), 1084 deletions(-)
>>>
>>
>> [...]
>>
>>
>>>
>>> @@ -226,6 +233,7 @@ static void event_if_decode_nlmsg(int route_type, void *data, int len)
>>>         const struct lldp_mod_ops *ops;
>>>         struct rtattr *rta;
>>>         char device_name[IFNAMSIZ];
>>> +       struct lldp_agent *agent;
>>>         int attrlen;
>>>         int valid;
>>>         int link_status = IF_OPER_UNKNOWN;
>>> @@ -268,14 +276,21 @@ static void event_if_decode_nlmsg(int route_type, void *data, int len)
>>>                         if (!valid)
>>>                                 break;
>>>
>>> -                       LIST_FOREACH(np, &lldp_head, lldp) {
>>> -                               ops = np->ops;
>>> -                               if (ops->lldp_mod_ifdown)
>>> -                                       ops->lldp_mod_ifdown(device_name);
>>> +                       struct port *port = port_find_by_name(device_name);
>>                                     ^^^^^
>> Need to check port is not null here?
> 
> Yes, will be in my next version, thanks !
> 
>>
>>> +                       LIST_FOREACH(agent, &port->agent_head, entry) {
>>> +                               LLDPAD_DBG("%s: calling ifdown for agent %p.\n",
>>> +                                          __func__, agent);
>>> +                               LIST_FOREACH(np, &lldp_head, lldp) {
>>> +                                       ops = np->ops;
>>> +                                       if (ops->lldp_mod_ifdown)
>>> +                                               ops->lldp_mod_ifdown(device_name,
>>> +                                                                    agent);
>>> +                               }
>>>                         }
>>>
>>>                         /* Disable Port */
>>> -                       set_lldp_port_enable_state(device_name, 0);
>>> +                       set_lldp_port_enable(device_name, 0);
>>>
>>>                         if (route_type == RTM_DELLINK) {
>>>                                 LLDPAD_INFO("%s: %s: device removed!\n",
>>
>> [...]
>>
>>
>>>  #endif /* _LLDP_H */
>>> diff --git a/include/lldp_8021qaz.h b/include/lldp_8021qaz.h
>>> index 8f5cf56..0cf2ac1 100644
>>> --- a/include/lldp_8021qaz.h
>>> +++ b/include/lldp_8021qaz.h
>>> @@ -228,11 +228,12 @@ int ieee8021qaz_check_active(const char *ifname);
>>>
>>>  struct lldp_module *ieee8021qaz_register(void);
>>>  void ieee8021qaz_unregister(struct lldp_module *mod);
>>> -struct packed_tlv *ieee8021qaz_gettlv(struct port *port);
>>> -int ieee8021qaz_rchange(struct port *port, struct unpacked_tlv *tlv);
>>> -void ieee8021qaz_ifup(char *ifname);
>>> -void ieee8021qaz_ifdown(char *ifname);
>>> -u8 ieee8021qaz_mibDeleteObject(struct port *port);
>>> +struct packed_tlv *ieee8021qaz_gettlv(struct port *port, struct lldp_agent *);
>>> +int ieee8021qaz_rchange(struct port *port, struct lldp_agent *,
>>> +                       struct unpacked_tlv *tlv);
>>> +void ieee8021qaz_ifup(char *ifname, struct lldp_agent *);
>>> +void ieee8021qaz_ifdown(char *ifname, struct lldp_agent *);
>>
>> These should all be 'const char' I believe, not a fault of this patch though.
>> I'll roll a follow up patch to constify things correctly.
> 
> I left these untouched for correction in a later patch.
> 
>>
>> [...]
>>
>>
>>> diff --git a/include/lldp_mod.h b/include/lldp_mod.h
>>> index 208bdd4..009734d 100644
>>> --- a/include/lldp_mod.h
>>> +++ b/include/lldp_mod.h
>>> @@ -55,13 +55,14 @@
>>>  struct lldp_mod_ops {
>>>         struct lldp_module *    (* lldp_mod_register)(void);
>>>         void                    (* lldp_mod_unregister)(struct lldp_module *);
>>> -       struct packed_tlv *     (* lldp_mod_gettlv)(struct port *);
>>> +       struct packed_tlv *     (* lldp_mod_gettlv)(struct port *, struct lldp_agent *);
>>>         int                     (* lldp_mod_rchange)(struct port *,
>>> +                                                    struct lldp_agent *,
>>>                                                     struct unpacked_tlv *);
>>>         void                    (* lldp_mod_utlv)(struct port *);
>>> -       void                    (* lldp_mod_ifup)(char *);
>>> -       void                    (* lldp_mod_ifdown)(char *);
>>> -       u8                      (* lldp_mod_mibdelete)(struct port *port);
>>> +       void                    (* lldp_mod_ifup)(char *, struct lldp_agent *);
>>> +       void                    (* lldp_mod_ifdown)(char *, struct lldp_agent *);
>>> +       u8                      (* lldp_mod_mibdelete)(struct port *port, struct lldp_agent *);
>>>         u32                     (* client_register)(void);
>>>         int                     (* client_cmd)(void *data,
>>>                                               struct sockaddr_un *from,
>>> @@ -70,7 +71,7 @@ struct lldp_mod_ops {
>>>         int                     (* print_tlv)(u32, u16, char *);
>>>         u32                     (* lookup_tlv_name)(char *);
>>
>> Do print_tlv and lookup_tlv_name need to be per agent as well? Maybe in a follow up patch.
> 
> I did not find a reason in my testing yet, but this could also be easily introduced in a later patch.

I agree if needed later we can add this.

> 
>>
>>>         int                     (* print_help)();
>>> -       int                     (* timer)();
>>> +       int                     (* timer)(struct port *, struct lldp_agent *);
>>>         struct arg_handlers *   (* get_arg_handler)(void);
>>>  };
>>>
>>> @@ -95,7 +96,6 @@ struct lldp_module {
>>>  LIST_HEAD(lldp_head, lldp_module);
>>>  struct lldp_head lldp_head;
>>>
>>> -
>>>  static inline struct lldp_module *find_module_by_id(struct lldp_head *head, int id)
>>>  {
>>>         struct lldp_module *mod;
>>> diff --git a/include/lldp_vdp.h b/include/lldp_vdp.h
>>> index 2f6adf8..f9cdee2 100644
>>
>> [...]
>>
>>> diff --git a/include/tlv_dcbx.h b/include/tlv_dcbx.h
>>> index d64d7b4..eb9ba38 100644
>>> --- a/include/tlv_dcbx.h
>>> +++ b/include/tlv_dcbx.h
>>> @@ -241,13 +241,13 @@ struct unpacked_tlv *bld_dcbx2_app_tlv(struct dcbx_tlvs *, u32 sub_type,
>>>  struct unpacked_tlv *bld_dcbx_llink_tlv(struct dcbx_tlvs *, u32 sub_type,
>>>                                         bool *success);
>>>
>>> -bool   unpack_dcbx1_tlvs(struct port *, struct unpacked_tlv *);
>>> -bool   unpack_dcbx2_tlvs(struct port *, struct unpacked_tlv *);
>>> -bool   process_dcbx_ctrl_tlv(struct port *);
>>> -bool   process_dcbx_pg_tlv(struct port *);
>>> -bool   process_dcbx_pfc_tlv(struct port *);
>>> -bool   process_dcbx_app_tlv(struct port *, int);
>>> -bool   process_dcbx_llink_tlv(struct port *);
>>> +bool   unpack_dcbx1_tlvs(struct port *, struct lldp_agent *, struct unpacked_tlv *);
>>> +bool   unpack_dcbx2_tlvs(struct port *, struct lldp_agent *, struct unpacked_tlv *);
>>> +bool   process_dcbx_ctrl_tlv(struct port *, struct lldp_agent *);
>>> +bool   process_dcbx_pg_tlv(struct port *, struct lldp_agent *);
>>> +bool   process_dcbx_pfc_tlv(struct port *, struct lldp_agent *);
>>> +bool   process_dcbx_app_tlv(struct port *, struct lldp_agent *, int);
>>> +bool   process_dcbx_llink_tlv(struct port *, struct lldp_agent *);
>>>
>>
>> I'm not sure how relevant multicast addresses besides 'nearest bridge' are
>> to DCBX. Can we avoid adding the agent logic here for now? At least we need
>> to verify that each ifname uses only enables DCBX on a single agent.
> 
> The problem is that the dcbx tlv code wants to access dupTlvs, dcbx_st and tooManyNghbrs
> in the rx struct which has now moved into the agent struct. I do not know enough about the
> dcbx module, but at least dupTlvs and dcbx_st look like they could move to a dcbx specific struct,
> but tooManyNghbrs is from the LLDP standard and cannot be changed that easily.
> Does DCBX really need access to this or can this be retrieved via some other method ?
> 

'tooManyNghbrs' is baked into the CEE spec. I think its fine the way you have
it here. If I come up with something a bit neater I'll post a follow up patch.

>>
>> [...]
>>
>>
>>> +
>>> +int lldp_add_agent(char *ifname, int type)
>>> +{
>>
>> Making these 'const char *ifname' would be good.
>>
>>> +       int count;
>>> +       struct port *port;
>>> +       struct lldp_agent *agent, *newagent;
>>> +
>>> +       port = port_find_by_name(ifname);
>>> +
>>> +       if (port == NULL)
>>> +               return -1;
>>> +
>>> +       /* check if lldp_agents for this if already exist */
>>> +       LIST_FOREACH(agent, &port->agent_head, entry) {
>>> +               if (agent->type != type)
>>> +                       continue;
>>> +               else
>>> +                       return -1;
>>> +       }
>>> +
>>> +       /* if not, create one and initialize it */
>>> +       LLDPAD_DBG("%s(%i): creating new agent for port %s.\n", __func__,
>>> +                  __LINE__, ifname);
>>> +       newagent  = (struct lldp_agent *)malloc(sizeof(struct lldp_agent));
>>> +       if (newagent == NULL) {
>>> +               LLDPAD_DBG("%s(%i): creation of new agent failed !.\n",
>>> +                          __func__,  __LINE__);
>>> +               return -1;
>>> +       }
>>> +
>>> +       lldp_init_agent(port, newagent, type);
>>> +
>>> +       if (get_config_setting_by_agent(ifname, newagent->type, ARG_ADMINSTATUS,
>>> +                       (void *)&newagent->adminStatus, CONFIG_TYPE_INT))
>>> +                       newagent->adminStatus = disabled;
>>> +
>>> +       LLDPAD_DBG("%s(%i): agent->adminStatus = %s (%i).\n", __func__,
>>> +                  __LINE__, (newagent->adminStatus == disabled) ? "disabled" : "enabled",
>>> +                  newagent->adminStatus);
>>> +
>>> +       LIST_INSERT_HEAD(&port->agent_head, newagent, entry);
>>> +
>>> +       count = 0;
>>> +       LIST_FOREACH(agent, &port->agent_head, entry) {
>>> +               count++;
>>> +       }
>>
>> Could probably count these in LIST_FOREACH above. If you think it is
>> cleaner here though that is fine.
> 
> Done.
> 
>>
>>> +
>>> +       LLDPAD_DBG("%s: %i agents on if %s.\n", __func__, count, port->ifname);
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +int lldp_remove_agent(char *ifname, int type)
>>> +{
>>> +       struct lldp_agent *agent;
>>> +
>>> +       agent = lldp_agent_find_by_type(ifname, type);
>>> +
>>> +       if (agent == NULL)
>>> +               return 0;
>>> +
>>> +       LIST_REMOVE(agent, entry);
>>> +
>>> +       free(agent);
>>> +
>>> +       return 0;
>>> +}
>>>
>>>  static void timer(void *eloop_data, void *user_ctx)
>>>  {
>>> -       struct port *port = porthead;
>>>         struct lldp_module *n;
>>> +       struct lldp_agent *agent;
>>> +       struct port *port = porthead;
>>>
>>>         while (port != NULL) {
>>> -               update_tx_timers(port);
>>> -               run_tx_timers_sm(port);
>>> -               run_tx_sm(port);
>>> -               run_rx_sm(port);
>>> -               update_rx_timers(port);
>>> +               /* execute rx and tx sm for all agents on a port */
>>> +               LIST_FOREACH(agent, &port->agent_head, entry) {
>>> +                       char macstring[30];
>>> +                       mac2str(&agent->mac_addr[0], macstring, 29);
>>> +
>>> +                       update_tx_timers(agent);
>>> +                       run_tx_timers_sm(port, agent);
>>> +                       run_tx_sm(port, agent);
>>> +                       run_rx_sm(port, agent);
>>> +                       update_rx_timers(agent);
>>> +
>>>                 LIST_FOREACH(n, &lldp_head, lldp) {
>>                   ^^^^^ ->
>>
>> Need another tab indent here.
> 
> Done.
> 
>>
>>>                         if (n->ops && n->ops->timer)
>>> -                               n->ops->timer(port);
>>> +                               n->ops->timer(port, agent);
>>> +               }
>>> +
>>>                 }
>>> -               if (port->timers.dormantDelay)
>>> -                       port->timers.dormantDelay--;
>>> +
>>> +               if (port->dormantDelay)
>>> +                       port->dormantDelay--;
>>> +
>>
>> Should dormantDelay be per agent? We can make this a follow up
>> patch rather than make this patch even larger. I think enabling
>> and disabling an agent would want a per agent instance of the
>> dormantDelay. Any thoughts?
> 
> So far, only the dcbx and the qaz module use dormantDelay and I therefore think
> it is safe to be left in port. Changing this would mean adding more agent
> dependencies to these 2 modules. Not sure how much effort this would be.
> 

Agreed. Lets just leave it as is for now. If there is a real
need to make this per agent later we can post a follow up
patch.

>>
>> [...]
>>
>>
>>> diff --git a/lldp/agent.h b/lldp/agent.h
>>> index 96c6bef..27d2d1c 100644
>>> --- a/lldp/agent.h
>>> +++ b/lldp/agent.h
>>> @@ -27,8 +27,145 @@
>>>  #ifndef AGENT_H
>>>  #define AGENT_H
>>>
>>> -int start_lldp_agent(void);
>>> -void stop_lldp_agent(void);
>>> -void clean_lldp_agent(void);
>>> +#include "lldp.h"
>>> +#include "mibdata.h"
>>> +
>>> +#ifndef ETH_ALEN
>>> +#define ETH_ALEN    6
>>> +#endif
>>> +#ifndef IFNAMSIZ
>>> +#define IFNAMSIZ    16  /* must match MAX_DEVICE_NAME_LEN */
>>> +#endif
>>> +#ifndef ETH_P_ALL
>>> +#define ETH_P_ALL   0x0003
>>> +#endif
>>
>> Why do we need ETH_xxx and IFNAMSIZ here? I'm sure I just missed it.
>> Also is this done because in including the correct headers conflicts
>> with something.
> 
> If we wanted to use this from /usr/include/linux/if.h we would get a lot of
> collisions with other definitions.
> 

OK I remember this now.

>>
>>> +
>>> +enum agent_type {
>>> +       NEAREST_BRIDGE = 0,
>>> +       NEAREST_NONTPMR_BRIDGE,
>>> +       NEAREST_CUSTOMER_BRIDGE,
>>> +       AGENT_MAX,
>>> +};
>>> +
>>> +/* IEEE 802.1AB-2009 - Table 7-1: group MAC addresses used by LLDP */
>>> +static const u8 nearest_bridge[ETH_ALEN] = {0x01,0x80,0xc2,0x00,0x00,0x0e};
>>> +static const u8 nearest_nontpmr_bridge[ETH_ALEN] = {0x01,0x80,0xc2,0x00,0x00,0x03};
>>> +static const u8 nearest_customer_bridge[ETH_ALEN] = {0x01,0x80,0xc2,0x00,0x00,0x00};
>>> +
>>> +static const u8 groupmacs[AGENT_MAX][ETH_ALEN] = {
>>> +       [NEAREST_BRIDGE] = {0x01,0x80,0xc2,0x00,0x00,0x0e},
>>> +       [NEAREST_NONTPMR_BRIDGE] = {0x01,0x80,0xc2,0x00,0x00,0x03},
>>> +       [NEAREST_CUSTOMER_BRIDGE] = {0x01,0x80,0xc2,0x00,0x00,0x00},
>>> +};
>>
>> Two constants for the same value?
> 
> Fixed.
> 
>>
>>> +
>>> +struct agenttimers {
>>> +/* Tx */
>>> +       u16 state;
>>> +       u16 reinitDelay;
>>> +       u16 msgTxHold;
>>> +       u16 msgTxInterval;
>>> +       u16 msgFastTx;
>>> +       u16 txFastInit;
>>> +       u16 txTTR;
>>> +       u16 txShutdownWhile;
>>> +       u16 txCredit;
>>> +       u16 txMaxCredit;
>>> +       bool txTick;
>>> +/* Rx */
>>> +       u16 tooManyNghbrsTimer;
>>> +       u16 rxTTL;
>>> +       u16 lastrxTTL;  /* cache last received */
>>> +};
>>> +
>>> +struct agenttx {
>>> +       u8 *frameout;
>>> +       u32 sizeout;
>>> +       u8 state;
>>> +       u8 localChange;
>>> +       u16 txTTL;
>>> +       bool txNow;
>>> +       u16 txFast;
>>> +};
>>> +
>>> +/* per agent statistical counter as in chapter 9.2.6
>>> + * of IEEE 802.1AB-2009 */
>>> +struct agentstats {
>>> +/* Tx */
>>> +       u32 statsFramesOutTotal;
>>> +/* Rx */
>>> +       u32 statsAgeoutsTotal;
>>> +       u32 statsFramesDiscardedTotal;
>>> +       u32 statsFramesInErrorsTotal;
>>> +       u32 statsFramesInTotal;
>>> +       u32 statsTLVsDiscardedTotal;
>>> +       u32 statsTLVsUnrecognizedTotal;
>>> +};
>>> +
>>> +typedef struct rxmanifest{
>>> +       struct unpacked_tlv *chassis;
>>> +       struct unpacked_tlv *portid;
>>> +       struct unpacked_tlv *ttl;
>>> +       struct unpacked_tlv *portdesc;
>>> +       struct unpacked_tlv *sysname;
>>> +       struct unpacked_tlv *sysdesc;
>>> +       struct unpacked_tlv *syscap;
>>> +       struct unpacked_tlv *mgmtadd;
>>> +}rxmanifest;
>>    ^^^
>>
>> nit, should be a space between '}' and rxmanifest.
> 
> Fixed.
> 
>>
>>> +
>>> +struct agentrx {
>>> +       u8 *framein;
>>> +       u16 sizein;
>>> +       u8 state;
>>> +       u8 badFrame;
>>> +       u8 rcvFrame;
>>> +       u8 rxInfoAge;
>>> +       u8 remoteChange;
>>> +       u8 tooManyNghbrs;
>>> +       u8 dupTlvs;
>>> +       u8 dcbx_st;
>>> +       bool newNeighbor;
>>> +       rxmanifest *manifest;
>>> +};
>>> +
>>> +enum agentAdminStatus {
>>> +       disabled,
>>> +       enabledTxOnly,
>>> +       enabledRxOnly,
>>> +       enabledRxTx,
>>> +};
>>> +
>>> +/* lldp agent specific structure as in chapter 9.2.5
>>> + * of IEEE 802.1AB-2009 */
>>> +struct lldp_agent {
>>> +       int     adminStatus;
>>> +
>>> +       int     pad;
>>
>> What is 'pad' here. Its not listed in 9.2.5 or is this to
>> pad the struct for some reason?
> 
> It is to pad the struct, I saw a crash without it.
> 

hmm does this indicate a bad memcpy somewhere? If I get a
chance I might try to track this down.

>>
>>> +
>>> +       u8      mac_addr[ETH_ALEN];
>>> +
>>> +       struct  agentrx rx;
>>> +       struct  agenttx tx;
>>> +       struct  agentstats stats;
>>> +       struct  agenttimers timers;
>>> +       u8      rxChanges;
>>> +       u16     lldpdu;
>>> +       struct  msap msap;
>>> +
>>> +       enum    agent_type type;
>>> +
>>> +        LIST_ENTRY(lldp_agent) entry;
>>> +};
>>> +
>>> +struct lldp_agent *lldp_agent_find_by_type(const char *, int);
>>> +int lldp_add_agent(char *ifname, int type);
>>                       ^^^^^^^
>> const char ifname
> 
> Done.
> 
>>
>>> +int lldp_remove_agent(char *ifname, int type);
>>                        ^^^^^
>> const char ifname
>>
>>
>>> +
>>> +void set_lldp_agent_admin(const char *ifname, int type, int enable);
>>> +int get_lldp_agent_admin(const char *ifname, int type);
>>> +int get_lldp_agent_statistics(char *ifname, struct agentstats *, int);
>>                                ^^^^^^^^^^^^
>> same 'const' comment throughout. Actually I believe these are just
>> copied renamed routines. In that case don't worry about it here.
>> We can post a patch on top to fixup that nit.
> 
> Done.
> 
>>
>>> +
>>> +int start_lldp_agents(void);
>>> +void stop_lldp_agents(void);
>>> +void clean_lldp_agents(void);
>>>
>>>  #endif /* AGENT_H */
>>> diff --git a/lldp/l2_packet.h b/lldp/l2_packet.h
>>> index 763b3f6..c1428ad 100644
>>> --- a/lldp/l2_packet.h
>>> +++ b/lldp/l2_packet.h
>>> @@ -60,11 +60,6 @@
>>>  #define ETH_MIN_PKT_LEN     (ETH_MIN_DATA_LEN + ETH_HDR_LEN)
>>>  #endif
>>>
>>> -#define LLDP_MULTICAST_MAC 0x0180c200000e
>>> -
>>> -
>>> -static const u8 multi_cast_source[ETH_ALEN] = {0x01,0x80,0xc2,0x00,0x00,0x0e};
>>> -
>>>  /**
>>>   * struct l2_packet_data - Internal l2_packet data structure
>>>   *
>>> @@ -129,7 +124,7 @@ int l2_packet_get_own_src_addr(struct l2_packet_data *l2, u8 *addr);
>>>   */
>>>  int l2_packet_get_own_addr(struct l2_packet_data *l2, u8 *addr);
>>>
>>> -void get_remote_peer_mac_addr(struct port *port);
>>> +void get_remote_peer_mac_addr(struct port *port, struct lldp_agent *);
>>>  void l2_packet_get_remote_addr(struct l2_packet_data *l2, u8 *addr);
>>>
>>
>> [...]
>>
>>>
>>>  void set_port_oper_delay(const char *ifname)
>>>  {
>>> -       struct port *port = NULL;
>>> -
>>> -       port = porthead;
>>> -       while (port != NULL) {
>>> -               if (!strncmp(ifname, port->ifname, IFNAMSIZ))
>>> -                       break;
>>> -               port = port->next;
>>> -       }
>>> +       struct port *port = port_find_by_name(ifname);
>>>
>>>         if (port == NULL)
>>>                 return;
>>>
>>> -       port->timers.dormantDelay = DORMANT_DELAY;
>>> +       port->dormantDelay = DORMANT_DELAY;
>>> +
>>>         return;
>>>  }
>>>
>>> @@ -198,13 +201,7 @@ int set_port_hw_resetting(const char *ifname, int resetting)
>>>  {
>>>         struct port *port = NULL;
>>>
>>> -       port = porthead;
>>> -       while (port != NULL) {
>>> -               if (!strncmp(ifname, port->ifname, IFNAMSIZ)) {
>>> -                       break;
>>> -               }
>>> -               port = port->next;
>>> -       }
>>> +       port = port_find_by_name(ifname);
>>>
>>
>> These optimizations could be there own patch.
> 
> Ok, I'll try to pull it out.
> 
>>
>>>         if (port == NULL)
>>>                 return -1;
>>> @@ -218,12 +215,7 @@ int get_port_hw_resetting(const char *ifname)
>>>  {
>>>         struct port *port = NULL;
>>>
>>> -       port = porthead;
>>> -       while (port != NULL) {
>>> -               if (!strncmp(ifname, port->ifname, IFNAMSIZ))
>>> -                       break;
>>> -               port = port->next;
>>> -       }
>>> +       port = port_find_by_name(ifname);
>>>
>>
>> same comment for all the rest.
>>
>>>         if (port)
>>>                 return port->hw_resetting;
>>> @@ -233,43 +225,45 @@ int get_port_hw_resetting(const char *ifname)
>>>
>>>  int reinit_port(const char *ifname)
>>>  {
>>> +       struct lldp_agent *agent;
>>>         struct port *port;
>>>
>>> -       port = porthead;
>>> -       while (port != NULL) {
>>> -               if (!strncmp(ifname, port->ifname, IFNAMSIZ))
>>> -                       break;
>>> -               port = port->next;
>>> -       }
>>> +       port = port_find_by_name(ifname);
>>>
>>> -       if (!port)
>>> +       if (port == NULL)
>>
>> unnecessary change here.
>>
>>>                 return -1;
>>>
>>>         /* Reset relevant port variables */
>>> -       port->tx.state  = TX_LLDP_INITIALIZE;
>>> -       port->timers.state  = TX_TIMER_BEGIN;
>>> -       port->rx.state = LLDP_WAIT_PORT_OPERATIONAL;
>>>         port->hw_resetting = false;
>>>         port->portEnabled = false;
>>> -       port->tx.txTTL = 0;
>>> -       port->msap.length1 = 0;
>>> -       port->msap.msap1 = NULL;
>>> -       port->msap.length2 = 0;
>>> -       port->msap.msap2 = NULL;
>>> -       port->lldpdu = false;
>>> -       port->timers.dormantDelay = DORMANT_DELAY;
>>> -
>>> -       /* init & enable RX path */
>>> -       rxInitializeLLDP(port);
>>> -
>>> -       /* init TX path */
>>> -       txInitializeTimers(port);
>>> -       txInitializeLLDP(port);
>>> +       port->dormantDelay = DORMANT_DELAY;
>>> +
>>> +       LIST_FOREACH(agent, &port->agent_head, entry) {
>>> +               /* init TX path */
>>> +
>>> +               /* Reset relevant state variables */
>>> +               agent->tx.state  = TX_LLDP_INITIALIZE;
>>> +               agent->rx.state = LLDP_WAIT_PORT_OPERATIONAL;
>>> +               agent->tx.txTTL = 0;
>>> +               agent->msap.length1 = 0;
>>> +               agent->msap.msap1 = NULL;
>>> +               agent->msap.length2 = 0;
>>> +               agent->msap.msap2 = NULL;
>>> +               agent->lldpdu = false;
>>> +               agent->timers.state  = TX_TIMER_BEGIN;
>>> +
>>> +               /* init & enable RX path */
>>> +               rxInitializeLLDP(port, agent);
>>> +
>>> +               /* init TX path */
>>> +               txInitializeTimers(agent);
>>> +               txInitializeLLDP(agent);
>>> +       }
>>>
>>>         return 0;
>>>  }
>>>
>>
>> [...]
>>
>>>
>>> +/* lldp port specific structure */
>>>  struct port {
>>>         char *ifname;
>>>         u8 hw_resetting;
>>>         u8 portEnabled;
>>>         u8 prevPortEnabled;
>>> -       u8 adminStatus;
>>> +       struct porttimers *timers;
>>>
>>> -       /* protocol specific */
>>> +       u16 dormantDelay;
>>> +
>>> +       LIST_HEAD(agent_head, lldp_agent) agent_head;
>>>         struct l2_packet_data *l2;
>>> -       struct portrx rx;
>>> -       struct porttx tx;
>>> -       struct portstats stats;
>>> -       struct porttimers timers;
>>> -       u8 rxChanges;
>>> -       u16   lldpdu;
>>> -       struct msap msap;
>>>
>>>         struct port *next;
>>>  };
>>>
>>>  extern struct port *porthead;
>>> -extern struct port *portcurrent;
>>> -extern struct port *porttail;
>>>
>>
>> [...]
>>
>>> diff --git a/lldp_8021qaz.c b/lldp_8021qaz.c
>>> index 2a6685b..0be1dad 100644
>>> --- a/lldp_8021qaz.c
>>> +++ b/lldp_8021qaz.c
>>> @@ -51,8 +51,8 @@
>>>  struct lldp_head lldp_head;
>>>  struct config_t lldpad_cfg;
>>>
>>> -static int ieee8021qaz_check_pending(struct port *port);
>>> -static void run_all_sm(struct port *port);
>>> +static int ieee8021qaz_check_pending(struct port *port, struct lldp_agent *);
>>> +static void run_all_sm(struct port *port, struct lldp_agent *agent);
>>>  static void ieee8021qaz_mibUpdateObjects(struct port *port);
>>>  static void ieee8021qaz_app_reset(struct app_tlv_head *head);
>>>
>>> @@ -68,7 +68,8 @@ static const struct lldp_mod_ops ieee8021qaz_ops = {
>>>         .timer                  = ieee8021qaz_check_pending,
>>>  };
>>>
>>
>> Somewhere in 802.1Qaz there needs to be a check to only allow DCBX on
>> on one agent per port. Additionally, 'nearest customer bridge' and
>> 'nontpmr' should be disabled by default. This can be done with a follow
>> up patch.
> 
> I already have an idea on how to do this. I will work on a followup patch for this.
> 
>> [Edit] Although it looks like 802.1qaz does nothing with the agent so
>> this should be fine. Is this true?
> 
> Not sure right now, I need to look into this again.
> 
>>
>>> -static int ieee8021qaz_check_pending(struct port *port)
>>> +static int ieee8021qaz_check_pending(struct port *port,
>>> +                                    struct lldp_agent *agent)
>>>  {
>>>         struct ieee8021qaz_user_data *iud;
>>>         struct ieee8021qaz_tlvs *tlv = NULL;
>>> @@ -81,11 +82,12 @@ static int ieee8021qaz_check_pending(struct port *port)
>>>                 LIST_FOREACH(tlv, &iud->head, entry) {
>>>                         if (!strncmp(port->ifname, tlv->ifname, IFNAMSIZ)) {
>>>                                 if (tlv->active && tlv->pending &&
>>> -                                   port->timers.dormantDelay == 1) {
>>> +                                   port->dormantDelay == 1) {
>>>                                         tlv->pending = false;
>>>                                         ieee8021qaz_app_reset(&tlv->app_head);
>>> -                                       run_all_sm(port);
>>> -                                       somethingChangedLocal(port->ifname);
>>> +                                       run_all_sm(port, agent);
>>> +                                       somethingChangedLocal(port->ifname,
>>> +                                                             agent->type);
>>>                                 }
>>>                                 break;
>>>                         }
>>
>>>
>>> -void run_all_sm(struct port *port)
>>> +void run_all_sm(struct port *port, struct lldp_agent *agent)
>>>  {
>>>         struct ieee8021qaz_tlvs *tlvs;
>>>         struct ieee_ets *ets;
>>> @@ -1004,7 +1020,8 @@ void run_all_sm(struct port *port)
>>>         if (!tlvs)
>>>                 return;
>>>
>>> -       if (!is_tlv_txdisabled(port->ifname, TLVID_8021(LLDP_8021QAZ_ETSCFG))) {
>>> +       if (!is_tlv_txdisabled(port->ifname,
>>> +                              TLVID_8021(LLDP_8021QAZ_ETSCFG))) {
>>
>> Why the new line wrapping? This can be done in a separate patch.
> 
> Ok, I'll try to pull out the unrelated changes.
> 

Thanks!

>>
>>
>>>                 ets_sm(tlvs->ets->cfgl, tlvs->ets->recr,
>>>                        &tlvs->ets->current_state);
>>>         }
>>> @@ -1023,7 +1040,8 @@ void run_all_sm(struct port *port)
>>>         else
>>>                 ets_cfg_to_ieee(ets, tlvs->ets->cfgl);
>>>
>>> -       if (!is_tlv_txdisabled(port->ifname, TLVID_8021(LLDP_8021QAZ_PFC)))
>>> +       if (!is_tlv_txdisabled(port->ifname,
>>> +                              TLVID_8021(LLDP_8021QAZ_PFC)))
>>
>> same comment.
>>
>>>                 pfc_sm(tlvs);
>>>
>>>         if (tlvs->pfc->current_state == RX_RECOMMEND)
>>> @@ -1281,7 +1299,8 @@ error:
>>>   * ieee8021qaz_bld_tlv - builds all IEEE8021QAZ TLVs
>>>   * Returns 1 on success, NULL if any of the TLVs fail to build correctly.
>>>   */
>>> -static struct packed_tlv *ieee8021qaz_bld_tlv(struct port *port)
>>> +static struct packed_tlv *ieee8021qaz_bld_tlv(struct port *port,
>>> +                                             struct lldp_agent *agent)
>>>  {
>>>         struct ieee8021qaz_tlvs *data;
>>>         struct packed_tlv *ptlv = NULL;
>>> @@ -1297,13 +1316,17 @@ static struct packed_tlv *ieee8021qaz_bld_tlv(struct port *port)
>>>         if (!data->active)
>>>                 return ptlv;
>>>
>>> -       if (!is_tlv_txdisabled(port->ifname, TLVID_8021(LLDP_8021QAZ_ETSCFG)))
>>> +       if (!is_tlv_txdisabled(port->ifname,
>>> +                              TLVID_8021(LLDP_8021QAZ_ETSCFG)))
>>>                 etscfg_tlv = bld_ieee8021qaz_etscfg_tlv(data);
>>> -       if (is_tlv_txenabled(port->ifname, TLVID_8021(LLDP_8021QAZ_ETSREC)))
>>> +       if (is_tlv_txenabled(port->ifname,
>>> +                            TLVID_8021(LLDP_8021QAZ_ETSREC)))
>>>                 etsrec_tlv = bld_ieee8021qaz_etsrec_tlv(data);
>>> -       if (!is_tlv_txdisabled(port->ifname, TLVID_8021(LLDP_8021QAZ_PFC)))
>>> +       if (!is_tlv_txdisabled(port->ifname,
>>> +                              TLVID_8021(LLDP_8021QAZ_PFC)))
>>>                 pfc_tlv = bld_ieee8021qaz_pfc_tlv(data);
>>> -       if (is_tlv_txenabled(port->ifname, TLVID_8021(LLDP_8021QAZ_APP)))
>>> +       if (is_tlv_txenabled(port->ifname,
>>> +                            TLVID_8021(LLDP_8021QAZ_APP)))
>>>                 app_tlv = bld_ieee8021qaz_app_tlv(port->ifname);
>>>
>>
>> same comment on above changes.
>>
>>>         size = TLVSIZE(etscfg_tlv)
>>> @@ -1333,18 +1356,21 @@ err:
>>>  }
>>>
>>>  /* LLDP_8021QAZ_MOD_OPS - GETTLV */
>>> -struct packed_tlv *ieee8021qaz_gettlv(struct port *port)
>>> +struct packed_tlv *ieee8021qaz_gettlv(struct port *port,
>>> +                                     struct lldp_agent *agent)
>>>  {
>>>         struct packed_tlv *ptlv = NULL;
>>>
>>>         /* Update TLV State Machines */
>>> -       run_all_sm(port);
>>> +       run_all_sm(port, agent);
>>>         /* Build TLVs */
>>> -       ptlv = ieee8021qaz_bld_tlv(port);
>>> +       ptlv = ieee8021qaz_bld_tlv(port, agent);
>>>         return ptlv;
>>>  }
>>>
>>
>> [...]
>>
>>
>>> diff --git a/lldp_dcbx.c b/lldp_dcbx.c
>>> index 7949147..c2011ff 100644
>>> --- a/lldp_dcbx.c
>>> +++ b/lldp_dcbx.c
>>> @@ -30,6 +30,8 @@
>>>  #include "linux/if.h"
>>>  #include "include/linux/dcbnl.h"
>>>  #include "lldp.h"
>>> +#include "lldp/ports.h"
>>> +#include "lldp/states.h"
>>>  #include "dcb_types.h"
>>>  #include "lldp_dcbx.h"
>>>  #include "dcb_protocol.h"
>>> @@ -41,8 +43,6 @@
>>>  #include "clif_msgs.h"
>>>  #include "lldp_mod.h"
>>>  #include "lldp_mand_clif.h"
>>> -#include "lldp/ports.h"
>>> -#include "lldp/states.h"
>>>  #include "lldp_dcbx_nl.h"
>>>  #include "lldp_dcbx_cfg.h"
>>>  #include "lldp_dcbx_cmds.h"
>>> @@ -55,11 +55,11 @@
>>>  extern u8 gdcbx_subtype;
>>>
>>>  void dcbx_free_tlv(struct dcbx_tlvs *tlvs);
>>> -static int dcbx_check_operstate(struct port *port);
>>> +static int dcbx_check_operstate(struct port *port, struct lldp_agent *agent);
>>>
>>>  const struct lldp_mod_ops dcbx_ops = {
>>> -       .lldp_mod_register      = dcbx_register,
>>> -       .lldp_mod_unregister    = dcbx_unregister,
>>> +       .lldp_mod_register      = dcbx_register,
>>> +       .lldp_mod_unregister    = dcbx_unregister,
>>
>> Some spacing fixes?
> 
> Yes, could be pulled out as well.
> 
>>
>>>         .lldp_mod_gettlv        = dcbx_gettlv,
>>>         .lldp_mod_rchange       = dcbx_rchange,
>>>         .lldp_mod_ifup          = dcbx_ifup,
>>> @@ -70,7 +70,7 @@ const struct lldp_mod_ops dcbx_ops = {
>>>         .timer                  = dcbx_check_operstate,
>>>  };
>>>
>>> -static int dcbx_check_operstate(struct port *port)
>>> +static int dcbx_check_operstate(struct port *port, struct lldp_agent *agent)
>>>  {
>>>         int err;
>>>         u8 app_good = 0;
>>> @@ -78,7 +78,7 @@ static int dcbx_check_operstate(struct port *port)
>>>         app_attribs app_data;
>>>         pfc_attribs pfc_data;
>>>
>>> -       if (!port->portEnabled || !port->timers.dormantDelay)
>>> +       if (!port->portEnabled || !port->dormantDelay)
>>>                 return 0;
>>>
>>>         err = get_app(port->ifname, 0, &app_data);
>>> @@ -95,10 +95,10 @@ static int dcbx_check_operstate(struct port *port)
>>>             !app_data.protocol.Enable)
>>>                 app_good = 1;
>>>
>>> -       if ((pfc_good && app_good) || port->timers.dormantDelay == 1) {
>>> +       if ((pfc_good && app_good) || port->dormantDelay == 1) {
>>>                 LLDPAD_DBG("%s: %s: IF_OPER_UP delay, %u pfc oper %u"
>>>                            "app oper %u\n",
>>> -                       __func__, port->ifname, port->timers.dormantDelay,
>>> +                       __func__, port->ifname, port->dormantDelay,
>>>                         pfc_data.protocol.OperMode,
>>>                         app_data.protocol.OperMode);
>>>                 set_operstate(port->ifname, IF_OPER_UP);
>>> @@ -122,7 +122,7 @@ struct dcbx_tlvs *dcbx_data(const char *ifname)
>>>                                 return tlv;
>>>                 }
>>>         }
>>> -
>>> +
>>>         return NULL;
>>>  }
>>>
>>> @@ -258,7 +258,7 @@ void dcbx_free_manifest(struct dcbx_manifest *manifest)
>>>  {
>>>         if (!manifest)
>>>                 return;
>>> -
>>> +
>>>         if (manifest->dcbx1)
>>>                 manifest->dcbx1 = free_unpkd_tlv(manifest->dcbx1);
>>>         if (manifest->dcbx2)
>>> @@ -323,7 +323,7 @@ void dcbx_free_tlv(struct dcbx_tlvs *tlvs)
>>>         return;
>>>  }
>>>
>>> -struct packed_tlv* dcbx_gettlv(struct port *port)
>>> +struct packed_tlv* dcbx_gettlv(struct port *port, struct lldp_agent *agent)
>>>  {
>>>         struct packed_tlv *ptlv = NULL;
>>>         struct dcbx_tlvs *tlvs;
>>> @@ -453,7 +453,7 @@ void dcbx_unregister(struct lldp_module *mod)
>>>         LLDPAD_DBG("%s: unregister dcbx complete.\n", __func__);
>>>  }
>>>
>>
>> [...]
>>
>> The rest just looks like search and replace. This looks really good outside the
>> few nits and questions above. The null check being the only real comment. Thanks
>> for doing this Jens.
> 
> 
> Thanks for reviewing ! I will resend the series soon.
> 
> Jens
> 
> --
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzender des Aufsichtsrats: Martin Jetter
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294




More information about the lldp-devel mailing list