Skip to content

Commit e0b2ac6

Browse files
authored
swss: Align STP structures and remove GCC diagnostic pragmas (#3440)
What I did: - Updated stpmgr.h to replace packed structs with aligned structs - Replaced the old processStpPortAttr in stpmgr.cpp with a version that uses properly aligned STP_PORT_CONFIG_MSG allocations How I did it: - Applied the same alignment fixes from 202211 branch to master - Removed #pragma GCC diagnostic usage and introduced ALIGNED(4) as well as padding fields in the relevant STP_* structures Why I did it: - To resolve address-of-packed-member warnings that break builds with certain compiler/platform configurations (e.g., Broadcom) - Aligning data structures properly avoids potential misalignment errors
1 parent 120ed90 commit e0b2ac6

2 files changed

Lines changed: 175 additions & 132 deletions

File tree

cfgmgr/stpmgr.cpp

Lines changed: 57 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -410,95 +410,109 @@ void StpMgr::doStpVlanPortTask(Consumer &consumer)
410410
}
411411
}
412412

413-
void StpMgr::processStpPortAttr(const string op, vector<FieldValueTuple>&tupEntry, const string intfName)
413+
void StpMgr::processStpPortAttr(const string op,
414+
vector<FieldValueTuple> &tupEntry,
415+
const string intfName)
414416
{
415-
STP_PORT_CONFIG_MSG *msg;
417+
STP_PORT_CONFIG_MSG *msg = nullptr;
416418
uint32_t len = 0;
417419
int vlanCnt = 0;
418420
vector<VLAN_ATTR> vlan_list;
419421

422+
// If we're setting this port's attributes, retrieve the list of VLANs for it.
420423
if (op == SET_COMMAND)
424+
{
421425
vlanCnt = getAllPortVlan(intfName, vlan_list);
426+
}
422427

423-
len = (uint32_t)(sizeof(STP_PORT_CONFIG_MSG) + (vlanCnt * sizeof(VLAN_ATTR)));
424-
msg = (STP_PORT_CONFIG_MSG *)calloc(1, len);
428+
// Allocate enough space for STP_PORT_CONFIG_MSG + all VLAN_ATTR entries.
429+
len = static_cast<uint32_t>(
430+
sizeof(STP_PORT_CONFIG_MSG) + (vlanCnt * sizeof(VLAN_ATTR))
431+
);
432+
msg = static_cast<STP_PORT_CONFIG_MSG *>(calloc(1, len));
425433
if (!msg)
426434
{
427-
SWSS_LOG_ERROR("mem failed for %s", intfName.c_str());
435+
SWSS_LOG_ERROR("calloc failed for interface %s", intfName.c_str());
428436
return;
429437
}
430438

431-
strncpy(msg->intf_name, intfName.c_str(), IFNAMSIZ-1);
432-
msg->count = vlanCnt;
433-
SWSS_LOG_INFO("Vlan count %d", vlanCnt);
439+
// Copy interface name and VLAN count into the message.
440+
strncpy(msg->intf_name, intfName.c_str(), IFNAMSIZ - 1);
441+
msg->count = vlanCnt;
442+
SWSS_LOG_INFO("VLAN count for %s is %d", intfName.c_str(), vlanCnt);
434443

435-
if(msg->count)
444+
// If there are VLANs, copy them into the message structure.
445+
if (msg->count > 0)
436446
{
437-
int i = 0;
438-
#pragma GCC diagnostic push
439-
#pragma GCC diagnostic ignored "-Waddress-of-packed-member"
440-
VLAN_ATTR *attr = msg->vlan_list;
441-
#pragma GCC diagnostic pop
442-
for (auto p = vlan_list.begin(); p != vlan_list.end(); p++)
447+
for (int i = 0; i < msg->count; i++)
443448
{
444-
attr[i].inst_id = p->inst_id;
445-
attr[i].mode = p->mode;
446-
SWSS_LOG_DEBUG("Inst:%d Mode:%d", p->inst_id, p->mode);
447-
i++;
449+
msg->vlan_list[i].inst_id = vlan_list[i].inst_id;
450+
msg->vlan_list[i].mode = vlan_list[i].mode;
451+
msg->vlan_list[i].vlan_id = vlan_list[i].vlan_id;
452+
SWSS_LOG_DEBUG("Inst:%d Mode:%d",
453+
vlan_list[i].inst_id,
454+
vlan_list[i].mode);
448455
}
449456
}
450457

458+
// Populate message fields based on the operation (SET or DEL).
451459
if (op == SET_COMMAND)
452460
{
453-
msg->opcode = STP_SET_COMMAND;
454-
msg->priority = -1;
461+
msg->opcode = STP_SET_COMMAND;
462+
msg->priority = -1; // Default priority unless specified
455463

456-
for (auto i : tupEntry)
464+
for (auto &fvt : tupEntry)
457465
{
458-
SWSS_LOG_DEBUG("Field: %s Val: %s", fvField(i).c_str(), fvValue(i).c_str());
459-
if (fvField(i) == "enabled")
466+
const auto &field = fvField(fvt);
467+
const auto &value = fvValue(fvt);
468+
469+
SWSS_LOG_DEBUG("Field: %s, Value: %s", field.c_str(), value.c_str());
470+
471+
if (field == "enabled")
460472
{
461-
msg->enabled = (fvValue(i) == "true") ? 1 : 0;
473+
msg->enabled = (value == "true") ? 1 : 0;
462474
}
463-
else if (fvField(i) == "root_guard")
475+
else if (field == "root_guard")
464476
{
465-
msg->root_guard = (fvValue(i) == "true") ? 1 : 0;
477+
msg->root_guard = (value == "true") ? 1 : 0;
466478
}
467-
else if (fvField(i) == "bpdu_guard")
479+
else if (field == "bpdu_guard")
468480
{
469-
msg->bpdu_guard = (fvValue(i) == "true") ? 1 : 0;
481+
msg->bpdu_guard = (value == "true") ? 1 : 0;
470482
}
471-
else if (fvField(i) == "bpdu_guard_do_disable")
483+
else if (field == "bpdu_guard_do_disable")
472484
{
473-
msg->bpdu_guard_do_disable = (fvValue(i) == "true") ? 1 : 0;
485+
msg->bpdu_guard_do_disable = (value == "true") ? 1 : 0;
474486
}
475-
else if (fvField(i) == "path_cost")
487+
else if (field == "path_cost")
476488
{
477-
msg->path_cost = stoi(fvValue(i).c_str());
489+
msg->path_cost = stoi(value);
478490
}
479-
else if (fvField(i) == "priority")
491+
else if (field == "priority")
480492
{
481-
msg->priority = stoi(fvValue(i).c_str());
493+
msg->priority = stoi(value);
482494
}
483-
else if (fvField(i) == "portfast")
495+
else if (field == "portfast")
484496
{
485-
msg->portfast = (fvValue(i) == "true") ? 1 : 0;
497+
msg->portfast = (value == "true") ? 1 : 0;
486498
}
487-
else if (fvField(i) == "uplink_fast")
499+
else if (field == "uplink_fast")
488500
{
489-
msg->uplink_fast = (fvValue(i) == "true") ? 1 : 0;
501+
msg->uplink_fast = (value == "true") ? 1 : 0;
490502
}
491503
}
492504
}
493505
else if (op == DEL_COMMAND)
494506
{
495-
msg->opcode = STP_DEL_COMMAND;
507+
msg->opcode = STP_DEL_COMMAND;
496508
msg->enabled = 0;
497509
}
498510

499-
sendMsgStpd(STP_PORT_CONFIG, len, (void *)msg);
500-
if (msg)
501-
free(msg);
511+
// Send the fully prepared message to the STP daemon.
512+
sendMsgStpd(STP_PORT_CONFIG, len, reinterpret_cast<void *>(msg));
513+
514+
// Clean up.
515+
free(msg);
502516
}
503517

504518
void StpMgr::doStpPortTask(Consumer &consumer)

0 commit comments

Comments
 (0)