diff --git a/cfgmgr/stpmgr.cpp b/cfgmgr/stpmgr.cpp index 77991e21b0..d3b75e53b1 100644 --- a/cfgmgr/stpmgr.cpp +++ b/cfgmgr/stpmgr.cpp @@ -410,95 +410,109 @@ void StpMgr::doStpVlanPortTask(Consumer &consumer) } } -void StpMgr::processStpPortAttr(const string op, vector&tupEntry, const string intfName) +void StpMgr::processStpPortAttr(const string op, + vector &tupEntry, + const string intfName) { - STP_PORT_CONFIG_MSG *msg; + STP_PORT_CONFIG_MSG *msg = nullptr; uint32_t len = 0; int vlanCnt = 0; vector vlan_list; + // If we're setting this port's attributes, retrieve the list of VLANs for it. if (op == SET_COMMAND) + { vlanCnt = getAllPortVlan(intfName, vlan_list); + } - len = (uint32_t)(sizeof(STP_PORT_CONFIG_MSG) + (vlanCnt * sizeof(VLAN_ATTR))); - msg = (STP_PORT_CONFIG_MSG *)calloc(1, len); + // Allocate enough space for STP_PORT_CONFIG_MSG + all VLAN_ATTR entries. + len = static_cast( + sizeof(STP_PORT_CONFIG_MSG) + (vlanCnt * sizeof(VLAN_ATTR)) + ); + msg = static_cast(calloc(1, len)); if (!msg) { - SWSS_LOG_ERROR("mem failed for %s", intfName.c_str()); + SWSS_LOG_ERROR("calloc failed for interface %s", intfName.c_str()); return; } - strncpy(msg->intf_name, intfName.c_str(), IFNAMSIZ-1); - msg->count = vlanCnt; - SWSS_LOG_INFO("Vlan count %d", vlanCnt); + // Copy interface name and VLAN count into the message. + strncpy(msg->intf_name, intfName.c_str(), IFNAMSIZ - 1); + msg->count = vlanCnt; + SWSS_LOG_INFO("VLAN count for %s is %d", intfName.c_str(), vlanCnt); - if(msg->count) + // If there are VLANs, copy them into the message structure. + if (msg->count > 0) { - int i = 0; -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Waddress-of-packed-member" - VLAN_ATTR *attr = msg->vlan_list; -#pragma GCC diagnostic pop - for (auto p = vlan_list.begin(); p != vlan_list.end(); p++) + for (int i = 0; i < msg->count; i++) { - attr[i].inst_id = p->inst_id; - attr[i].mode = p->mode; - SWSS_LOG_DEBUG("Inst:%d Mode:%d", p->inst_id, p->mode); - i++; + msg->vlan_list[i].inst_id = vlan_list[i].inst_id; + msg->vlan_list[i].mode = vlan_list[i].mode; + msg->vlan_list[i].vlan_id = vlan_list[i].vlan_id; + SWSS_LOG_DEBUG("Inst:%d Mode:%d", + vlan_list[i].inst_id, + vlan_list[i].mode); } } + // Populate message fields based on the operation (SET or DEL). if (op == SET_COMMAND) { - msg->opcode = STP_SET_COMMAND; - msg->priority = -1; + msg->opcode = STP_SET_COMMAND; + msg->priority = -1; // Default priority unless specified - for (auto i : tupEntry) + for (auto &fvt : tupEntry) { - SWSS_LOG_DEBUG("Field: %s Val: %s", fvField(i).c_str(), fvValue(i).c_str()); - if (fvField(i) == "enabled") + const auto &field = fvField(fvt); + const auto &value = fvValue(fvt); + + SWSS_LOG_DEBUG("Field: %s, Value: %s", field.c_str(), value.c_str()); + + if (field == "enabled") { - msg->enabled = (fvValue(i) == "true") ? 1 : 0; + msg->enabled = (value == "true") ? 1 : 0; } - else if (fvField(i) == "root_guard") + else if (field == "root_guard") { - msg->root_guard = (fvValue(i) == "true") ? 1 : 0; + msg->root_guard = (value == "true") ? 1 : 0; } - else if (fvField(i) == "bpdu_guard") + else if (field == "bpdu_guard") { - msg->bpdu_guard = (fvValue(i) == "true") ? 1 : 0; + msg->bpdu_guard = (value == "true") ? 1 : 0; } - else if (fvField(i) == "bpdu_guard_do_disable") + else if (field == "bpdu_guard_do_disable") { - msg->bpdu_guard_do_disable = (fvValue(i) == "true") ? 1 : 0; + msg->bpdu_guard_do_disable = (value == "true") ? 1 : 0; } - else if (fvField(i) == "path_cost") + else if (field == "path_cost") { - msg->path_cost = stoi(fvValue(i).c_str()); + msg->path_cost = stoi(value); } - else if (fvField(i) == "priority") + else if (field == "priority") { - msg->priority = stoi(fvValue(i).c_str()); + msg->priority = stoi(value); } - else if (fvField(i) == "portfast") + else if (field == "portfast") { - msg->portfast = (fvValue(i) == "true") ? 1 : 0; + msg->portfast = (value == "true") ? 1 : 0; } - else if (fvField(i) == "uplink_fast") + else if (field == "uplink_fast") { - msg->uplink_fast = (fvValue(i) == "true") ? 1 : 0; + msg->uplink_fast = (value == "true") ? 1 : 0; } } } else if (op == DEL_COMMAND) { - msg->opcode = STP_DEL_COMMAND; + msg->opcode = STP_DEL_COMMAND; msg->enabled = 0; } - sendMsgStpd(STP_PORT_CONFIG, len, (void *)msg); - if (msg) - free(msg); + // Send the fully prepared message to the STP daemon. + sendMsgStpd(STP_PORT_CONFIG, len, reinterpret_cast(msg)); + + // Clean up. + free(msg); } void StpMgr::doStpPortTask(Consumer &consumer) diff --git a/cfgmgr/stpmgr.h b/cfgmgr/stpmgr.h index 8eb7ffdf80..ca66067a3b 100644 --- a/cfgmgr/stpmgr.h +++ b/cfgmgr/stpmgr.h @@ -15,36 +15,41 @@ #include #include +// We remove PACKED definitions, only keep ALIGNED +#if defined(__GNUC__) +#define ALIGNED(x) __attribute__((aligned(x))) +#else +#define ALIGNED(x) +#endif + #define STPMGRD_SOCK_NAME "/var/run/stpmgrd.sock" -#define TAGGED_MODE 1 -#define UNTAGGED_MODE 0 -#define INVALID_MODE -1 +#define TAGGED_MODE 1 +#define UNTAGGED_MODE 0 +#define INVALID_MODE -1 -#define MAX_VLANS 4096 +#define MAX_VLANS 4096 // Maximum number of instances supported -#define L2_INSTANCE_MAX MAX_VLANS -#define STP_DEFAULT_MAX_INSTANCES 255 -#define INVALID_INSTANCE -1 - +#define L2_INSTANCE_MAX MAX_VLANS +#define STP_DEFAULT_MAX_INSTANCES 255 +#define INVALID_INSTANCE -1 #define GET_FIRST_FREE_INST_ID(_idx) \ while (_idx < (int)l2InstPool.size() && l2InstPool.test(_idx)) ++_idx; \ l2InstPool.set(_idx) #define FREE_INST_ID(_idx) l2InstPool.reset(_idx) - #define FREE_ALL_INST_ID() l2InstPool.reset() - -#define IS_INST_ID_AVAILABLE() (l2InstPool.count() < max_stp_instances) ? true : false +#define IS_INST_ID_AVAILABLE() (l2InstPool.count() < max_stp_instances) #define STPD_SOCK_NAME "/var/run/stpipc.sock" -typedef enum L2_PROTO_MODE{ +// Enumerations must match stp_ipc.h +typedef enum L2_PROTO_MODE { L2_NONE, L2_PVSTP, -}L2_PROTO_MODE; +} L2_PROTO_MODE; typedef enum STP_MSG_TYPE { STP_INVALID_MSG, @@ -56,7 +61,7 @@ typedef enum STP_MSG_TYPE { STP_VLAN_MEM_CONFIG, STP_STPCTL_MSG, STP_MAX_MSG -}STP_MSG_TYPE; +} STP_MSG_TYPE; typedef enum STP_CTL_TYPE { STP_CTL_HELP, @@ -75,88 +80,105 @@ typedef enum STP_CTL_TYPE { STP_CTL_CLEAR_INTF, STP_CTL_CLEAR_VLAN_INTF, STP_CTL_MAX -}STP_CTL_TYPE; +} STP_CTL_TYPE; +// Remove PACKED, add ALIGNED(4) typedef struct STP_IPC_MSG { - int msg_type; - unsigned int msg_len; - char data[0]; -}STP_IPC_MSG; + int msg_type; + unsigned int msg_len; + char data[0]; +} ALIGNED(4) STP_IPC_MSG; -#define STP_SET_COMMAND 1 -#define STP_DEL_COMMAND 0 +#define STP_SET_COMMAND 1 +#define STP_DEL_COMMAND 0 +// Add padding for alignment if needed (compare to stp_ipc.h) typedef struct STP_INIT_READY_MSG { - uint8_t opcode; // enable/disable - uint16_t max_stp_instances; -}__attribute__ ((packed))STP_INIT_READY_MSG; + uint8_t opcode; // enable/disable + uint16_t max_stp_instances; + // Example: potential extra padding if alignment warnings arise + // uint8_t padding[1]; +} ALIGNED(4) STP_INIT_READY_MSG; +// Add padding for alignment if needed typedef struct STP_BRIDGE_CONFIG_MSG { - uint8_t opcode; // enable/disable - uint8_t stp_mode; - int rootguard_timeout; - uint8_t base_mac_addr[6]; -}__attribute__ ((packed))STP_BRIDGE_CONFIG_MSG; - + uint8_t opcode; // enable/disable + uint8_t stp_mode; + int rootguard_timeout; + uint8_t base_mac_addr[6]; + // Potential padding for alignment: + // uint8_t padding[2]; +} ALIGNED(4) STP_BRIDGE_CONFIG_MSG; + +// Must match the version in stp_ipc.h exactly typedef struct PORT_ATTR { - char intf_name[IFNAMSIZ]; - int8_t mode; - uint8_t enabled; -}PORT_ATTR; - + char intf_name[IFNAMSIZ]; // 16 bytes typically + int8_t mode; + uint8_t enabled; + // Add padding to align to 4 bytes + uint16_t padding; +} ALIGNED(4) PORT_ATTR; + +// Must match the version in stp_ipc.h exactly typedef struct STP_VLAN_CONFIG_MSG { - uint8_t opcode; // enable/disable - uint8_t newInstance; - int vlan_id; - int inst_id; - int forward_delay; - int hello_time; - int max_age; - int priority; - int count; - PORT_ATTR port_list[0]; -}__attribute__ ((packed))STP_VLAN_CONFIG_MSG; + uint8_t opcode; // enable/disable + uint8_t newInstance; + int vlan_id; + int inst_id; + int forward_delay; + int hello_time; + int max_age; + int priority; + int count; + PORT_ATTR port_list[0]; +} ALIGNED(4) STP_VLAN_CONFIG_MSG; typedef struct STP_VLAN_PORT_CONFIG_MSG { - uint8_t opcode; // enable/disable - int vlan_id; - char intf_name[IFNAMSIZ]; - int inst_id; - int path_cost; - int priority; -}__attribute__ ((packed))STP_VLAN_PORT_CONFIG_MSG; + uint8_t opcode; // enable/disable + int vlan_id; + char intf_name[IFNAMSIZ]; + int inst_id; + int path_cost; + int priority; +} ALIGNED(4) STP_VLAN_PORT_CONFIG_MSG; typedef struct VLAN_ATTR { - int inst_id; - int vlan_id; - int8_t mode; -}VLAN_ATTR; + int inst_id; + int vlan_id; + int8_t mode; + // Add padding to align to 4 bytes + uint8_t padding[3]; +} ALIGNED(4) VLAN_ATTR; typedef struct STP_PORT_CONFIG_MSG { - uint8_t opcode; // enable/disable - char intf_name[IFNAMSIZ]; - uint8_t enabled; - uint8_t root_guard; - uint8_t bpdu_guard; - uint8_t bpdu_guard_do_disable; - uint8_t portfast; - uint8_t uplink_fast; - int path_cost; - int priority; - int count; - VLAN_ATTR vlan_list[0]; -}__attribute__ ((packed))STP_PORT_CONFIG_MSG; + uint8_t opcode; // enable/disable + char intf_name[IFNAMSIZ]; + uint8_t enabled; + uint8_t root_guard; + uint8_t bpdu_guard; + uint8_t bpdu_guard_do_disable; + uint8_t portfast; + uint8_t uplink_fast; + // Add 2 bytes padding for alignment + uint16_t padding; + int path_cost; + int priority; + int count; + VLAN_ATTR vlan_list[0]; +} ALIGNED(4) STP_PORT_CONFIG_MSG; typedef struct STP_VLAN_MEM_CONFIG_MSG { - uint8_t opcode; // enable/disable - int vlan_id; - int inst_id; - char intf_name[IFNAMSIZ]; - uint8_t enabled; - int8_t mode; - int path_cost; - int priority; -}__attribute__ ((packed))STP_VLAN_MEM_CONFIG_MSG; + uint8_t opcode; // enable/disable + int vlan_id; + int inst_id; + char intf_name[IFNAMSIZ]; + uint8_t enabled; + int8_t mode; + // Add 1 byte padding + uint8_t padding; + int path_cost; + int priority; +} ALIGNED(4) STP_VLAN_MEM_CONFIG_MSG; namespace swss { @@ -164,11 +186,11 @@ class StpMgr : public Orch { public: StpMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, - const std::vector &tables); + const std::vector &tables); using Orch::doTask; - void ipcInitStpd(); - int sendMsgStpd(STP_MSG_TYPE msgType, uint32_t msgLen, void *data); + void ipcInitStpd(); + int sendMsgStpd(STP_MSG_TYPE msgType, uint32_t msgLen, void *data); MacAddress macAddress; bool isPortInitDone(DBConnector *app_db); uint16_t getStpMaxInstances(void); @@ -186,7 +208,7 @@ class StpMgr : public Orch Table m_stateStpTable; std::bitset l2InstPool; - int stpd_fd; + int stpd_fd; enum L2_PROTO_MODE l2ProtoEnabled; int m_vlanInstMap[MAX_VLANS]; bool portCfgDone; @@ -210,16 +232,23 @@ class StpMgr : public Orch bool isLagStateOk(const std::string &alias); bool isStpPortEmpty(); bool isStpEnabled(const std::string &intf_name); - int getAllVlanMem(const std::string &vlanKey, std::vector&port_list); - int getAllPortVlan(const std::string &intfKey, std::vector&vlan_list); + int getAllVlanMem(const std::string &vlanKey, std::vector& port_list); + int getAllPortVlan(const std::string &intfKey, std::vector& vlan_list); int8_t getVlanMemMode(const std::string &key); int allocL2Instance(uint32_t vlan_id); void deallocL2Instance(uint32_t vlan_id); bool isLagEmpty(const std::string &key); - void processStpPortAttr(const std::string op, std::vector&tupEntry, const std::string intfName); - void processStpVlanPortAttr(const std::string op, uint32_t vlan_id, const std::string intfName, - std::vector&tupEntry); + + // The rest of the methods remain unchanged except we replaced the old approach with alignment-based approach + void processStpPortAttr(const std::string op, + std::vector &tupEntry, + const std::string intfName); + void processStpVlanPortAttr(const std::string op, + uint32_t vlan_id, + const std::string intfName, + std::vector &tupEntry); }; -} +} // namespace swss + #endif