[acltable]: Separate class AclTable from AclOrch file #1071
Conversation
Signed-off-by: Shu0t1an Cheng <shuche@microsoft.com> Signed-off-by: Shu0T1an ChenG <shuche@microsoft.com>
| typedef map<string, acl_stage_type_t> acl_stage_type_lookup_t; | ||
|
|
||
| #endif /* SWSS_ACLTABLE_H */ | ||
| class AclOrch; |
There was a problem hiding this comment.
I think it is better to include 'aclorch.h' in this file as that would a superset for other things as well.
There was a problem hiding this comment.
I think it's generally not a good practice to mutually include two header files.
There was a problem hiding this comment.
We have portsorch.h included in most header files. But aside from that, aclorch.h has most of the references for acltable.h.. My point is here as per description, the aim is to reduce the file size and not separate out the functionality.
There was a problem hiding this comment.
sorry i didn't quite get it. i think there's no duplication between the acltable.h and aclorch.h files.
There was a problem hiding this comment.
I agree Shuotian, I don't think having aclorch include acltable and vice-versa is a good idea.
From what I can tell the only dependency that AclTable has on AclOrch is m_mirrorTableCapabilities[ACL_TABLE_MIRRORV6] and m_isCombinedMirrorV6Table in create(). I think it would be cleaner to pass those two variables into create() (or the constructor if you think they might be useful in other methods as well) and then remove the reference to m_pAclOrch entirely. That way there's no cyclical dependency between AclTable and AclOrch.
|
retest this please |
| typedef map<string, acl_stage_type_t> acl_stage_type_lookup_t; | ||
|
|
||
| #endif /* SWSS_ACLTABLE_H */ | ||
| class AclOrch; |
There was a problem hiding this comment.
I agree Shuotian, I don't think having aclorch include acltable and vice-versa is a good idea.
From what I can tell the only dependency that AclTable has on AclOrch is m_mirrorTableCapabilities[ACL_TABLE_MIRRORV6] and m_isCombinedMirrorV6Table in create(). I think it would be cleaner to pass those two variables into create() (or the constructor if you think they might be useful in other methods as well) and then remove the reference to m_pAclOrch entirely. That way there's no cyclical dependency between AclTable and AclOrch.
| ACL_TABLE_CTRLPLANE, | ||
| ACL_TABLE_DTEL_FLOW_WATCHLIST, | ||
| ACL_TABLE_DTEL_DROP_WATCHLIST | ||
| } acl_table_type_t; |
| #include <set> | ||
| #include <string> | ||
| #include <vector> | ||
| #include <map> | ||
|
|
||
| #include "observer.h" | ||
|
|
||
| using namespace std; |
There was a problem hiding this comment.
Can you remove this from the header like we've started doing with the other classes in OA?
| #include <set> | ||
| #include <string> | ||
| #include <vector> | ||
| #include <map> | ||
|
|
||
| #include "observer.h" | ||
|
|
||
| using namespace std; | ||
|
|
There was a problem hiding this comment.
I think you can drop the TODO below this line.
| @@ -27,6 +29,78 @@ typedef enum | |||
| ACL_STAGE_EGRESS | |||
| } acl_stage_type_t; | |||
There was a problem hiding this comment.
Same thing as below, can we use enum class?
| sai_object_id_t getOid() { return m_oid; } | ||
| string getId() { return id; } |
There was a problem hiding this comment.
We're not really consistent about using getters vs. public variables here. We should either:
- Make all the variables public and don't bother with getters, or
- Make all the variables private and include (inline) getters for them
My personal preference is option 2, but I'm fine with either as long as we're consistent (1 is probably going to be easier as far as code changes go).
| // Map port oid to group member oid | ||
| std::map<sai_object_id_t, sai_object_id_t> ports; | ||
| // Map rule name to rule data | ||
| map<string, shared_ptr<AclRule>> rules; | ||
| // Set to store the ACL table port alias | ||
| set<string> portSet; | ||
| // Set to store the not cofigured ACL table port alias | ||
| set<string> pendingPortSet; |
There was a problem hiding this comment.
Probably doesn't make a huge difference since we're not storing very much data, but can we use unordered maps/sets here instead of the normal maps/sets?
| if (ruleIter != rules.end()) | ||
| { | ||
| // If ACL rule already exists, delete it first | ||
| if (ruleIter->second->remove()) |
There was a problem hiding this comment.
Can we have logging or something similar to the other methods if remove() fails?
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| assert(ports.find(portOid) != ports.end()); |
There was a problem hiding this comment.
Is this intentional? I know you didn't originally write AclTable but do you know if there's a reason assert is called here rather than performing some logging and returning false?
| return true; | ||
| } | ||
|
|
||
| bool AclTable::create() |
There was a problem hiding this comment.
Just a note, this method is kinda large and there are a lot of conditional checks for different types of tables and it gets a bit confusing in places. I think it would be worth breaking this up into private helpers to handle different table types, but that is also a pretty big change and probably out of the scope of this PR. Can you leave a note about it?
Make the length of each file smaller for better maintenance.