[vslib]Add MACsec Filters #713
Conversation
Signed-off-by: Ze Gan <[email protected]>
vslib/inc/MACsecFilter.h
Outdated
|
|
||
| namespace saivs | ||
| { | ||
| class MACsecFilter : public TrafficFilter |
There was a problem hiding this comment.
Done, please review it.
vslib/inc/MACsecFilter.h
Outdated
|
|
||
| virtual FilterStatus execute( | ||
| _Inout_ void *buffer, | ||
| _Inout_ ssize_t &length) override; |
There was a problem hiding this comment.
why signed size ? and not size_t ?
There was a problem hiding this comment.
This function will be call after recvmsg, So I directly use the type in this code
https://github.com/Azure/sonic-sairedis/blob/425ffbcbf57fdb66160c1ba84bbec2c0f1f5e983/vslib/src/HostInterfaceInfo.cpp#L145
Should I cast it to unsigned size?
There was a problem hiding this comment.
ok, this is fine, i though this size is buffer length, since then it make no sense for it to allow negative numbers
so i looked at your code, so you actually passing &length, but forward function is without (&), and you are not modifying length parameter in any way, so it should also be without (&), then recvmsg returns ssize, and its negative when error, but you done return that ether, you have your own FilterStatus error code, so i think this ssize_t& should be change to size_t in all places and if needed cast to int in recvmgs method since thats the definition
There was a problem hiding this comment.
I agree your point. size_t should make more sense.
And the function execute, I think, is for processing the data, maybe it will modify the data. So the paramete of execute are with (&).
But the function forward, I think, is for forwarding data. The forwarding action will never change the data. So the parameters are without (&).
vslib/inc/MACsecFilter.h
Outdated
| _Inout_ void *buffer, | ||
| _Inout_ ssize_t &length) override; | ||
|
|
||
| void enable_macsec_device(bool enable); |
There was a problem hiding this comment.
each param needs prefix In and 1 per line
There was a problem hiding this comment.
Done, please review it.
vslib/inc/MACsecFilter.h
Outdated
|
|
||
| void enable_macsec_device(bool enable); | ||
|
|
||
| void set_macsec_fd(int macsecfd); |
There was a problem hiding this comment.
same here 1 param per line
There was a problem hiding this comment.
Done, please review it.
vslib/inc/MACsecFilter.h
Outdated
| protected: | ||
| virtual FilterStatus forward( | ||
| _In_ const void *buffer, | ||
| _In_ ssize_t length) = 0; |
There was a problem hiding this comment.
Same reason as previous comment.
vslib/inc/MACsecIngressFilter.h
Outdated
|
|
||
| namespace saivs | ||
| { | ||
| class MACsecIngressFilter : public MACsecFilter |
There was a problem hiding this comment.
Done, please review it.
vslib/inc/MACsecIngressFilter.h
Outdated
| class MACsecIngressFilter : public MACsecFilter | ||
| { | ||
| public: | ||
| MACsecIngressFilter(_In_ const std::string &macsec_interface_name); |
There was a problem hiding this comment.
Done, please review it.
vslib/src/MACsecEgressFilter.cpp
Outdated
|
|
||
| using namespace saivs; | ||
|
|
||
| MACsecEgressFilter::MACsecEgressFilter(_In_ const std::string &macsecInterfaceName): |
There was a problem hiding this comment.
Done, please review it.
| } | ||
|
|
||
| void MACsecFilter::enable_macsec_device(bool enable) | ||
| { |
There was a problem hiding this comment.
Done, please review it.
| } | ||
|
|
||
| void MACsecFilter::set_macsec_fd(int macsecfd) | ||
| { |
There was a problem hiding this comment.
Done, please review it.
vslib/src/TrafficFilterPipes.cpp
Outdated
| ret = filter->execute( | ||
| buffer, | ||
| length); |
There was a problem hiding this comment.
join 3 lines to one line ? this is small enough
There was a problem hiding this comment.
Done, please review it.
vslib/src/TrafficFilterPipes.cpp
Outdated
| { | ||
| itr = m_filters.erase(itr); | ||
| } | ||
|
|
There was a problem hiding this comment.
Done, please review it.
Signed-off-by: Ze Gan <[email protected]>
Pterosaur
force-pushed
the
macsec_filter
branch
from
362725b to
c21d2c9
Compare
Signed-off-by: Ze Gan <[email protected]>
Signed-off-by: Ze Gan <[email protected]>
All filters will be installed in HostInterfaceInfo to filter the EAPOL and data traffic. The MACsecEgressFilter will forward EAPOL packets to eth device and forward data packets to MACsec device. The MACsecIngressFilter will forward EAPOL packets to Ethernet device and drop all data packets. Because the data packets will be forwarded to MACsec device by Linux kernel. The TrafficFilterPipes provides an interfaces to add more filters for future functions.
The document about MACsec Filters can be found at: https://github.com/Azure/SONiC/blob/88c7ada900051f73a29aaf4a28bcaf5062ae7305/doc/macsec/MACsec_hld.md#345-virtual-macsec-sai
All filters will be installed in
HostInterfaceInfoto filter the EAPOL and data traffic.The
MACsecEgressFilterwill forward EAPOL packets to eth device and forward data packets to MACsec device.The
MACsecIngressFilterwill forward EAPOL packets to Ethernet device and drop all data packets. Because the data packets will be forwarded to MACsec device by Linux kernel.The
TrafficFilterPipesprovides an interfaces to add more filters for future functions.Signed-off-by: Ze Gan [email protected]