Skip to content

Commit

Permalink
Code-review comments addressed
Browse files Browse the repository at this point in the history
  • Loading branch information
Vasant committed Feb 2, 2020
1 parent 36f26aa commit 397d61c
Show file tree
Hide file tree
Showing 7 changed files with 92 additions and 88 deletions.
29 changes: 17 additions & 12 deletions orchagent/aclorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1324,7 +1324,7 @@ bool AclTable::create()
table_attrs.push_back(attr);

attr.id = SAI_ACL_TABLE_ATTR_ACL_STAGE;
attr.value.s32 = (stage == ACL_STAGE_INGRESS) ?
attr.value.s32 = (stage == ACL_STAGE_INGRESS) ?
SAI_ACL_STAGE_INGRESS : SAI_ACL_STAGE_EGRESS;
table_attrs.push_back(attr);

Expand Down Expand Up @@ -1539,7 +1539,6 @@ void AclTable::update(SubjectType type, void *cntx)
}
else
{
// TODO: deal with port removal scenario
if (portSet.find(port.m_alias) != portSet.end())
{
unbind(bind_port_id);
Expand All @@ -1564,10 +1563,10 @@ bool AclTable::bind(sai_object_id_t portOid)
sai_object_id_t group_member_oid;
if (!gPortsOrch->bindAclTable(portOid, m_oid, group_member_oid, stage))
{
SWSS_LOG_NOTICE("Failed to bind port oid: %" PRIx64 "", portOid);
SWSS_LOG_ERROR("Failed to bind port oid: %" PRIx64 "", portOid);
return false;
}
SWSS_LOG_NOTICE("Successfully bound port oid: %" PRIx64", group member oid:%" PRIx64 "",
SWSS_LOG_NOTICE("Successfully bound port oid: %" PRIx64", group member oid:%" PRIx64 "",
portOid, group_member_oid);
ports[portOid] = group_member_oid;
return true;
Expand All @@ -1584,7 +1583,7 @@ bool AclTable::unbind(sai_object_id_t portOid)
{
return false;
}
SWSS_LOG_NOTICE("%" PRIx64" port is unbound from %s ACL table",
SWSS_LOG_NOTICE("%" PRIx64" port is unbound from %s ACL table",
portOid, id.c_str());
ports[portOid] = SAI_NULL_OBJECT_ID;
return true;
Expand Down Expand Up @@ -2556,10 +2555,11 @@ void AclOrch::getAddDeletePorts(AclTable &newT,
curPortSet.insert(p);
}

// Get the difference newPortSet-curPortSet and curPortSet-newPortsSet
// Get all the ports to be added
std::set_difference(newPortSet.begin(), newPortSet.end(),
curPortSet.begin(), curPortSet.end(),
std::inserter(addSet, addSet.end()));
// Get all the ports to be deleted
std::set_difference(curPortSet.begin(), curPortSet.end(),
newPortSet.begin(), newPortSet.end(),
std::inserter(delSet, delSet.end()));
Expand All @@ -2583,7 +2583,8 @@ bool AclOrch::updateAclTablePorts(AclTable &newTable, AclTable &curTable)
{
SWSS_LOG_NOTICE("Removed:%s from pendingPortSet", p.c_str());
curTable.pendingPortSet.erase(p);
} else if (curTable.portSet.find(p) != curTable.portSet.end())
}
else if (curTable.portSet.find(p) != curTable.portSet.end())
{
gPortsOrch->getAclBindPortId(p, port_oid);
assert(port_oid != SAI_NULL_OBJECT_ID);
Expand Down Expand Up @@ -2936,19 +2937,21 @@ void AclOrch::doAclTableTask(Consumer &consumer)
!isAclTableStageUpdated(newTable.stage,
m_AclTables[table_oid]))
{
// Update the table existing table using the info in newTable
// Update the existing table using the info in newTable
if (updateAclTable(m_AclTables[table_oid], newTable))
{
SWSS_LOG_NOTICE("Successfully updated existing ACL table %s",
table_id.c_str());
it = consumer.m_toSync.erase(it);
} else
}
else
{
SWSS_LOG_ERROR("Failed to update existing ACL table %s",
table_id.c_str());
it++;
}
} else
}
else
{
if (addAclTable(newTable))
it = consumer.m_toSync.erase(it);
Expand Down Expand Up @@ -2993,7 +2996,7 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
string op = kfvOp(t);

SWSS_LOG_INFO("OP: %s, TABLE_ID: %s, RULE_ID: %s", op.c_str(), table_id.c_str(), rule_id.c_str());

if (table_id.empty())
{
SWSS_LOG_WARN("ACL rule with RULE_ID: %s is not valid as TABLE_ID is empty", rule_id.c_str());
Expand Down Expand Up @@ -3115,7 +3118,7 @@ bool AclOrch::processAclTablePorts(string portList, AclTable &aclTable)
{
SWSS_LOG_ERROR("Failed to get port %s bind port ID for ACL table %s",
alias.c_str(), aclTable.id.c_str());
return false;
return false;
}

aclTable.link(bind_port_id);
Expand All @@ -3129,6 +3132,7 @@ bool AclOrch::isAclTableTypeUpdated(acl_table_type_t table_type, AclTable &t)
{
return (table_type != t.type);
}

bool AclOrch::processAclTableType(string type, acl_table_type_t &table_type)
{
SWSS_LOG_ENTER();
Expand Down Expand Up @@ -3160,6 +3164,7 @@ bool AclOrch::isAclTableStageUpdated(acl_stage_type_t acl_stage, AclTable &t)
{
return (acl_stage != t.stage);
}

bool AclOrch::processAclTableStage(string stage, acl_stage_type_t &acl_stage)
{
SWSS_LOG_ENTER();
Expand Down
15 changes: 10 additions & 5 deletions orchagent/aclorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,8 +389,13 @@ class AclTable {
class AclOrch : public Orch, public Observer
{
public:
AclOrch(vector<TableConnector>& connectors, TableConnector switchTable,
PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch, DTelOrch *m_dTelOrch = NULL);
AclOrch(vector<TableConnector>& connectors,
TableConnector switchTable,
PortsOrch *portOrch,
MirrorOrch *mirrorOrch,
NeighOrch *neighOrch,
RouteOrch *routeOrch,
DTelOrch *m_dTelOrch = NULL);
~AclOrch();
void update(SubjectType, void *);

Expand Down Expand Up @@ -456,9 +461,9 @@ class AclOrch : public Orch, public Observer
bool validateAclTable(AclTable &aclTable);
bool updateAclTablePorts(AclTable &newTable, AclTable &curTable);
void getAddDeletePorts(AclTable &newT,
AclTable &curT,
set<string> &addSet,
set<string> &delSet);
AclTable &curT,
set<string> &addSet,
set<string> &delSet);
sai_status_t createDTelWatchListTables();
sai_status_t deleteDTelWatchListTables();

Expand Down
47 changes: 26 additions & 21 deletions orchagent/portsorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -868,13 +868,13 @@ bool PortsOrch::setPortPfcAsym(Port &port, string pfc_asym)
*
* Description:
* To bind a port to ACL table we need to do two things.
* 1. Create ACL table member, which maps
* 1. Create ACL table member, which maps
* ACL table group OID --> ACL table OID
* 2. Set ACL table group OID as value port attribute.
*
* This function performs the second step of binding.
*
* Also, while unbinding we use this function to
* Also, while unbinding we use this function to
* set port attribute value to SAI_NULL_OBJECT_ID
*
* Port attribute name is derived from port type
Expand Down Expand Up @@ -953,10 +953,10 @@ bool PortsOrch::unbindRemoveAclTableGroup(sai_object_id_t port_oid,
}


sai_object_id_t &group_oid_ref =
sai_object_id_t &group_oid_ref =
ingress? port.m_ingress_acl_table_group_id :
port.m_egress_acl_table_group_id;
unordered_set<sai_object_id_t> &acl_list_ref =
unordered_set<sai_object_id_t> &acl_list_ref =
ingress ? port.m_ingress_acl_tables_uset :
port.m_egress_acl_tables_uset;

Expand All @@ -967,7 +967,7 @@ bool PortsOrch::unbindRemoveAclTableGroup(sai_object_id_t port_oid,
}
assert(acl_list_ref.find(acl_table_oid) != acl_list_ref.end());
acl_list_ref.erase(acl_table_oid);
if (!acl_list_ref.empty())
if (!acl_list_ref.empty())
{
// This port is in more than one acl table's port list
// So, we need to preserve group OID
Expand All @@ -977,11 +977,12 @@ bool PortsOrch::unbindRemoveAclTableGroup(sai_object_id_t port_oid,
}

port.clear_dependency(Port::ACL_DEP);
SWSS_LOG_NOTICE("Removing port OID %" PRIx64" ACL table grop ID", port_oid);
SWSS_LOG_NOTICE("Removing port OID %" PRIx64" ACL table group ID", port_oid);

// Unbind ACL group
if (!bindUnbindAclTableGroup(port, ingress, false))
{
SWSS_LOG_ERROR("Failed to remove ACL group ID from port");
return false;
}

Expand All @@ -998,7 +999,7 @@ bool PortsOrch::unbindRemoveAclTableGroup(sai_object_id_t port_oid,
SWSS_LOG_ERROR("Unknown SAI ACL bind point type");
return false;
}
gCrmOrch->decCrmAclUsedCounter(CrmResourceType::CRM_ACL_GROUP,
gCrmOrch->decCrmAclUsedCounter(CrmResourceType::CRM_ACL_GROUP,
ingress ? SAI_ACL_STAGE_INGRESS : SAI_ACL_STAGE_EGRESS,
bind_type, group_oid_ref);

Expand All @@ -1007,9 +1008,9 @@ bool PortsOrch::unbindRemoveAclTableGroup(sai_object_id_t port_oid,
return true;
}

bool PortsOrch::createBindAclTableGroup(sai_object_id_t port_oid,
sai_object_id_t acl_table_oid,
sai_object_id_t &group_oid,
bool PortsOrch::createBindAclTableGroup(sai_object_id_t port_oid,
sai_object_id_t acl_table_oid,
sai_object_id_t &group_oid,
acl_stage_type_t acl_stage)
{
SWSS_LOG_ENTER();
Expand All @@ -1023,22 +1024,22 @@ bool PortsOrch::createBindAclTableGroup(sai_object_id_t port_oid,

sai_status_t status;
Port port;
bool ingress = (ACL_STAGE_INGRESS == acl_stage) ?
bool ingress = (ACL_STAGE_INGRESS == acl_stage) ?
true : false;
if (!getPort(port_oid, port))
{
SWSS_LOG_ERROR("Failed to get port by port ID %" PRIx64, port_oid);
return false;
}

unordered_set<sai_object_id_t> &acl_list_ref =
unordered_set<sai_object_id_t> &acl_list_ref =
ingress ? port.m_ingress_acl_tables_uset :
port.m_egress_acl_tables_uset;
sai_object_id_t &group_oid_ref =
sai_object_id_t &group_oid_ref =
ingress ? port.m_ingress_acl_table_group_id :
port.m_egress_acl_table_group_id;
if (acl_list_ref.empty())

if (acl_list_ref.empty())
{
// Port ACL table group does not exist, create one
assert(group_oid_ref == SAI_NULL_OBJECT_ID);
Expand Down Expand Up @@ -1078,7 +1079,7 @@ bool PortsOrch::createBindAclTableGroup(sai_object_id_t port_oid,
assert(group_oid_ref != SAI_NULL_OBJECT_ID);

gCrmOrch->incCrmAclUsedCounter(CrmResourceType::CRM_ACL_GROUP,
ingress ? SAI_ACL_STAGE_INGRESS :
ingress ? SAI_ACL_STAGE_INGRESS :
SAI_ACL_STAGE_EGRESS, bind_type);

// Bind ACL table group
Expand All @@ -1089,7 +1090,7 @@ bool PortsOrch::createBindAclTableGroup(sai_object_id_t port_oid,

port.set_dependency(Port::ACL_DEP);

SWSS_LOG_NOTICE("Create %s ACL table group and bind port %s to it",
SWSS_LOG_NOTICE("Create %s ACL table group and bind port %s to it",
ingress ? "ingress" : "egress", port.m_alias.c_str());
}

Expand Down Expand Up @@ -1120,7 +1121,11 @@ bool PortsOrch::unbindAclTable(sai_object_id_t port_oid,
acl_group_member_oid);
return false;
}
unbindRemoveAclTableGroup(port_oid, acl_table_oid, acl_stage);

if (!unbindRemoveAclTableGroup(port_oid, acl_table_oid, acl_stage)) {
return false;
}

return true;
}

Expand Down Expand Up @@ -2268,7 +2273,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
if (m_portList[alias].has_dependency())
{
// Port has one or more dependencies, cannot remove
SWSS_LOG_WARN("Cannot to remove port because of dependency");
SWSS_LOG_WARN("Cannot to remove port because of dependency");
continue;
}

Expand Down Expand Up @@ -2615,7 +2620,7 @@ void PortsOrch::doLagTask(Consumer &consumer)
if (m_portList[alias].has_dependency())
{
// LAG has one or more dependencies, cannot remove
SWSS_LOG_WARN("Cannot to remove LAG because of dependency");
SWSS_LOG_WARN("Cannot to remove LAG because of dependency");
continue;
}

Expand Down Expand Up @@ -3880,7 +3885,7 @@ bool PortsOrch::getSaiAclBindPointType(Port::Type type,
return false;
}
return true;
}
}

bool PortsOrch::removeAclTableGroup(const Port &p)
{
Expand Down
20 changes: 10 additions & 10 deletions orchagent/portsorch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,23 +78,23 @@ class PortsOrch : public Orch, public Subject
bool setHostIntfsOperStatus(const Port& port, bool up) const;
void updateDbPortOperStatus(const Port& port, sai_port_oper_status_t status) const;
bool createBindAclTableGroup(sai_object_id_t port_oid,
sai_object_id_t acl_table_oid,
sai_object_id_t &group_oid,
acl_stage_type_t acl_stage = ACL_STAGE_EGRESS);
sai_object_id_t acl_table_oid,
sai_object_id_t &group_oid,
acl_stage_type_t acl_stage = ACL_STAGE_EGRESS);
bool unbindRemoveAclTableGroup(sai_object_id_t port_oid,
sai_object_id_t acl_table_oid,
sai_object_id_t acl_table_oid,
acl_stage_type_t acl_stage);
bool bindAclTable(sai_object_id_t id,
sai_object_id_t table_oid,
sai_object_id_t &group_member_oid,
acl_stage_type_t acl_stage = ACL_STAGE_INGRESS);
sai_object_id_t table_oid,
sai_object_id_t &group_member_oid,
acl_stage_type_t acl_stage = ACL_STAGE_INGRESS);
bool unbindAclTable(sai_object_id_t port_oid,
sai_object_id_t acl_table_oid,
sai_object_id_t acl_table_oid,
sai_object_id_t acl_group_member_oid,
acl_stage_type_t acl_stage);
bool bindUnbindAclTableGroup(Port &port,
bool ingress,
bool bind);
bool bind);
bool getPortPfc(sai_object_id_t portId, uint8_t *pfc_bitmask);
bool setPortPfc(sai_object_id_t portId, uint8_t pfc_bitmask);

Expand Down Expand Up @@ -232,7 +232,7 @@ class PortsOrch : public Orch, public Subject
bool setPortSerdesAttribute(sai_object_id_t port_id, sai_attr_id_t attr_id,
vector<uint32_t> &serdes_val);
bool getSaiAclBindPointType(Port::Type type,
sai_acl_bind_point_type_t &sai_acl_bind_type);
sai_acl_bind_point_type_t &sai_acl_bind_type);
};
#endif /* SWSS_PORTSORCH_H */

21 changes: 1 addition & 20 deletions tests/conftest.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -847,7 +847,7 @@ def remove_acl_table(self, table):
def update_acl_table(self, table, fvs):
tbl = swsscommon.Table(self.cdb, "ACL_TABLE")
tbl.set(table, fvs)
time.sleep(1)
time.sleep(1)

def get_acl_table_ids(self):
tbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE")
Expand All @@ -872,25 +872,6 @@ def verify_if_any_acl_table_created(self):

return False

def clean_up_left_over(self):
atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE_GROUP")
keys = atbl.getKeys()
for key in keys:
atbl._del(key)

atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE_GROUP")
keys = atbl.getKeys()
assert len(keys) == 0

atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE_GROUP_MEMBER")
keys = atbl.getKeys()
for key in keys:
atbl._del(key)

atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE_GROUP_MEMBER")
keys = atbl.getKeys()
assert len(keys) == 0

def verify_acl_group_num(self, expt):
atbl = swsscommon.Table(self.adb, "ASIC_STATE:SAI_OBJECT_TYPE_ACL_TABLE_GROUP")
acl_table_groups = atbl.getKeys()
Expand Down
Loading

0 comments on commit 397d61c

Please sign in to comment.