sonic-net / sonic-sairedis

SAI object interface to Redis database, as used in the SONiC project
Other
56 stars 263 forks source link

[Link Event Damping] Add tracker to track the selectable timers used by link event damper #1323

Open Ashish1805 opened 9 months ago

Ashish1805 commented 9 months ago

HLD: sonic-net/SONiC#1071

Ashish1805 commented 9 months ago

Hi @kcudnik CodeQL / Analyze (cpp) (pull_request_target) check is failing.

/usr/bin/ld: ../../syncd/libSyncd.a(libSyncd_a-PortStateChangeHandler.o): in function `syncd::PortStateChangeHandler::PortStateChangeHandler(std::shared_ptr<swss::SelectableEvent>)':
/home/runner/work/sonic-sairedis/sonic-sairedis/syncd/PortStateChangeHandler.cpp:21: undefined reference to `syncd::PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE'
collect2: error: ld returned 1 exit status

PORT_STATE_CHANGE_QUEUE_SIZE is defined as static and constexpr class data member (https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/PortStateChangeHandler.h#L43). I see sairedis repo uses c++14, so it is necessary to also provide definition for this static data member in .cc file. Like:

// Defined in PortStateChangeHandler.cc
constexpr size_t PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE;

PortStateChangeHandler.cpp was added in https://github.com/sonic-net/sonic-sairedis/pull/1310 PR and CodeQL / Analyze (cpp) (pull_request_target) check passed in that PR: https://github.com/sonic-net/sonic-sairedis/actions/runs/6885079249. Not sure why it didn't fail during CodeQL check of that PR.

I am not sure what is the protocol here: do we add fix PR for this or revert the culprit PR https://github.com/sonic-net/sonic-sairedis/pull/1310 and merge it again with suggested changes?

kcudnik commented 9 months ago

Hi @kcudnik CodeQL / Analyze (cpp) (pull_request_target) check is failing.

/usr/bin/ld: ../../syncd/libSyncd.a(libSyncd_a-PortStateChangeHandler.o): in function `syncd::PortStateChangeHandler::PortStateChangeHandler(std::shared_ptr<swss::SelectableEvent>)':
/home/runner/work/sonic-sairedis/sonic-sairedis/syncd/PortStateChangeHandler.cpp:21: undefined reference to `syncd::PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE'
collect2: error: ld returned 1 exit status

PORT_STATE_CHANGE_QUEUE_SIZE is defined as static and constexpr class data member (https://github.com/sonic-net/sonic-sairedis/blob/master/syncd/PortStateChangeHandler.h#L43). I see sairedis repo uses c++14, so it is necessary to also provide definition for this static data member in .cc file. Like:

// Defined in PortStateChangeHandler.cc
constexpr size_t PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE;

PortStateChangeHandler.cpp was added in #1310 PR and CodeQL / Analyze (cpp) (pull_request_target) check passed in that PR: https://github.com/sonic-net/sonic-sairedis/actions/runs/6885079249. Not sure why it didn't fail during CodeQL check of that PR.

I am not sure what is the protocol here: do we add fix PR for this or revert the culprit PR #1310 and merge it again with suggested changes?

SAME ISSUE CAUSES SWSS ERROR HERE: https://github.com/sonic-net/sonic-sairedis/pull/1310#issuecomment-1817445036

please figure out why this is causing build error or revert the change !

kcudnik commented 9 months ago

change

static constexpr size_t PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE;

to

constexpr size_t PortStateChangeHandler::PORT_STATE_CHANGE_QUEUE_SIZE;

first keyword static requires declaration in cpp file since it's considered a field

Ashish1805 commented 9 months ago

Adding @Junchao-Mellanox for review.