Skip to content

Commit

Permalink
Make TamEvent::SwitchEventType optional
Browse files Browse the repository at this point in the history
Summary:
TamEvent::SwitchEventType is currently a non-optional part of TamEvent::CreateAttribute. However it's not supported by [broadcom SDK](https://www.internalfb.com/code/fbsource/[3fe38940ae02]/fbcode/fboss/agent/hw/sai/api/bcm/TamApi.cpp?lines=17), and attempting to create a TamEvent causes the following error:

```
F1016 16:00:58.424264 603833 SaiAttribute.h:906] Check failed: id.has_value()  unknown id
*** Check failure stack trace: ***
*** Aborted at 1729119658 (Unix time, try 'date -d 1729119658') ***
*** Signal 6 (SIGABRT) (0x925a2) received by PID 599458 (pthread TID 0x7fdd1adb7640) (linux TID 603833) (maybe from PID 599458, UID 0) (code: -6), stack trace: ***
    @ 0000000027b40ebb (unknown)
    @ 00000000000442df (unknown)
    @ 0000000000099783 pthread_kill
    @ 000000000004422c raise
    @ 000000000002c432 abort
    @ 000000001a2fc3c9 google::LogMessage::Fail()
    @ 000000001a300604 google::LogMessage::SendToLog()
    @ 000000001a2fc042 google::LogMessage::Flush()
    @ 000000001a2fcd18 google::LogMessageFatal::~LogMessageFatal()
    @ 0000000029098a0b facebook::fboss::SaiExtensionAttribute<std::vector<int, std::allocator<int> >, facebook::fboss::SaiTamEventTraits::Attributes::AttributeSwitchEventType, void>::kExtensionAttributeId()
    @ 000000002e525b13 facebook::fboss::SaiTamManager::setupMirrorOnDrop(folly::IPAddress, facebook::fboss::PortID)
```

This commit makes the attribute optional so that the invalid access is avoided. There is no functional impact to either bcm or tajo code paths.

Reviewed By: nivinl

Differential Revision: D64337375

fbshipit-source-id: 423f673f7076d5ae19f985d82a78e1cee675f863
  • Loading branch information
maxwindiff authored and facebook-github-bot committed Oct 17, 2024
1 parent 7af70f7 commit 2774043
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 14 deletions.
2 changes: 1 addition & 1 deletion fboss/agent/hw/sai/api/TamApi.h
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ struct SaiTamEventTraits {
Attributes::Type,
Attributes::ActionList,
Attributes::CollectorList,
Attributes::SwitchEventType,
std::optional<Attributes::SwitchEventType>,
std::optional<Attributes::DeviceId>,
std::optional<Attributes::SwitchEventId>,
std::optional<Attributes::ExtensionsCollectorList>,
Expand Down
4 changes: 2 additions & 2 deletions fboss/agent/hw/sai/api/tests/TamApiTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -154,8 +154,8 @@ TEST_F(TamApiTest, TamEvent) {
std::get<SaiTamEventTraits::Attributes::ActionList>(eventAttr) = eventActions;
std::get<SaiTamEventTraits::Attributes::CollectorList>(eventAttr) =
eventCollectors;
std::get<SaiTamEventTraits::Attributes::SwitchEventType>(eventAttr) =
eventTypes;
std::get<std::optional<SaiTamEventTraits::Attributes::SwitchEventType>>(
eventAttr) = eventTypes;
std::get<std::optional<SaiTamEventTraits::Attributes::DeviceId>>(eventAttr) =
deviceId;
std::get<std::optional<SaiTamEventTraits::Attributes::SwitchEventId>>(
Expand Down
22 changes: 13 additions & 9 deletions fboss/agent/hw/sai/store/tests/TamStoreTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ class TamStoreTest : public SaiStoreTest {
SAI_TAM_EVENT_TYPE_PACKET_DROP;
std::get<SaiTamEventTraits::Attributes::ActionList>(result) = actions;
std::get<SaiTamEventTraits::Attributes::CollectorList>(result) = collectors;
std::get<SaiTamEventTraits::Attributes::SwitchEventType>(result) =
eventTypes;
std::get<std::optional<SaiTamEventTraits::Attributes::SwitchEventType>>(
result) = eventTypes;
std::get<std::optional<SaiTamEventTraits::Attributes::DeviceId>>(result) =
deviceId;
std::get<std::optional<SaiTamEventTraits::Attributes::SwitchEventId>>(
Expand Down Expand Up @@ -293,8 +293,10 @@ TEST_F(TamStoreTest, tamCtors) {
std::get<SaiTamEventTraits::Attributes::CollectorList>(tamEventAhk)
.value());
EXPECT_EQ(
GET_ATTR(TamEvent, SwitchEventType, eventObj.attributes()),
std::get<SaiTamEventTraits::Attributes::SwitchEventType>(tamEventAhk)
GET_OPT_ATTR(TamEvent, SwitchEventType, eventObj.attributes()),
std::get<std::optional<SaiTamEventTraits::Attributes::SwitchEventType>>(
tamEventAhk)
.value()
.value());

auto tamObj = createObj<SaiTamTraits>(tam);
Expand Down Expand Up @@ -425,8 +427,10 @@ TEST_F(TamStoreTest, setObject) {
GET_ATTR(TamEvent, CollectorList, event->attributes()),
std::get<SaiTamEventTraits::Attributes::CollectorList>(eventAhk).value());
EXPECT_EQ(
GET_ATTR(TamEvent, SwitchEventType, event->attributes()),
std::get<SaiTamEventTraits::Attributes::SwitchEventType>(eventAhk)
GET_OPT_ATTR(TamEvent, SwitchEventType, event->attributes()),
std::get<std::optional<SaiTamEventTraits::Attributes::SwitchEventType>>(
eventAhk)
.value()
.value());

auto tamAhk = tamTraits(event->adapterKey());
Expand Down Expand Up @@ -470,11 +474,11 @@ TEST_F(TamStoreTest, updateObject) {
auto tam = s.get<SaiTamTraits>().setObject(tamAhk, tamAhk);

std::vector<sai_int32_t> newEvents = {4, 5, 6};
std::get<SaiTamEventTraits::Attributes::SwitchEventType>(eventAhk) =
newEvents;
std::get<std::optional<SaiTamEventTraits::Attributes::SwitchEventType>>(
eventAhk) = newEvents;
auto updatedEvent = s.get<SaiTamEventTraits>().setObject(eventAhk, eventAhk);
EXPECT_EQ(
GET_ATTR(TamEvent, SwitchEventType, updatedEvent->attributes()),
GET_OPT_ATTR(TamEvent, SwitchEventType, updatedEvent->attributes()),
newEvents);

std::get<SaiTamTransportTraits::Attributes::DstPort>(transportAhk) = 10003;
Expand Down
4 changes: 2 additions & 2 deletions fboss/agent/hw/sai/switch/npu/tajo/SaiTamManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ SaiTamManager::SaiTamManager(
std::get<SaiTamEventTraits::Attributes::ActionList>(eventTraits) = actions;
std::get<SaiTamEventTraits::Attributes::CollectorList>(eventTraits) =
collectors;
std::get<SaiTamEventTraits::Attributes::SwitchEventType>(eventTraits) =
eventTypes;
std::get<std::optional<SaiTamEventTraits::Attributes::SwitchEventType>>(
eventTraits) = eventTypes;
auto& eventStore = saiStore_->get<SaiTamEventTraits>();
auto event = eventStore.setObject(eventTraits, eventTraits);

Expand Down

0 comments on commit 2774043

Please sign in to comment.