Fix potential defects #2516

Liran-Ar · 2022-11-13T15:32:02Z

What I did
Fix the following potential risks:

Uncaught exceptions in /src/nvos-swss/orchagent/main.cpp and /src/nvos-swss/portsyncd/portsyncd.cpp
Uninitialized scalar fields in /src/nvos-swss/orchagent/port.h
Unchecked return value in /src/nvos-swss/orchagent/portsorch.cpp
Pointer to local outside scope in /src/nvos-swss/orchagent/portsorch.cpp

How I did it

Add "catch" to the uncaught exceptions
Add initialization value to the uninitialized fields
Add a check to the unchecked return value
Delete an unnecessary comma

How I verified it

Inspection and sanity tests

Signed-off-by: Liran Artzi <[email protected]>

prsunny · 2022-11-14T17:58:00Z

portsyncd/portsyncd.cpp

-    DBConnector appl_db("APPL_DB", 0);
-    DBConnector state_db("STATE_DB", 0);
-    ProducerStateTable p(&appl_db, APP_PORT_TABLE_NAME);
+        DBConnector cfgDb("CONFIG_DB", 0);


Looks like there are unexpected diffs due to tab space. Can you fix the spacing diff in this PR and have only the relevant ones?

Hi, these indents were done on purpose since the code was inserted under the "try" block, stated on line 49.

Hi @prsunny , the PR merging is blocked due to coverage check failure (only 4 code lines are missing to reach the minimum threshold).
We've tried to expand existing unit tests or create new ones but found out that covering the missing lines via unit tests is very complex because it is called only from main() function.
Therefore, and because I didn't add any new functionality, I would ask for a waiver on the coverage check for this very unique case, Is it possible?

Hi @prsunny , the PR merging is blocked due to coverage check failure (only 4 code lines are missing to reach the minimum threshold). We've tried to expand existing unit tests or create new ones but found out that covering the missing lines via unit tests is very complex because it is called only from main() function. Therefore, and because I didn't add any new functionality, I would ask for a waiver on the coverage check for this very unique case, Is it possible?

@prsunny could you please refer to this question?

liat-grozovik · 2022-11-28T13:53:22Z

@Liran-Ar please review the coverage report of the PR and check what type of tests need to be added on code changes.

Liran-Ar · 2022-12-07T22:19:45Z

/AzurePipelines run

azure-pipelines · 2022-12-07T22:19:50Z

Commenter does not have sufficient privileges for PR 2516 in repo sonic-net/sonic-swss

liat-grozovik · 2023-01-11T14:55:27Z

@prsunny could you please review the latest comment to understand if we can move forward with this merge?

liat-grozovik · 2023-01-17T16:50:27Z

/azp run Azure.sonic-swss

azure-pipelines · 2023-01-17T16:50:37Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2023-01-17T22:33:41Z

@qiluo-msft for viz

qiluo-msft · 2023-01-23T22:50:33Z

LGTM

Update sonic-swss submodule pointer to include the following: * a2a483d [acl] Add new ACL key BTH_OPCODE and AETH_SYNDROME ([sonic-net#2617](sonic-net/sonic-swss#2617)) * 9d1f66b [bfdorch] add local discriminator to state DB ([sonic-net#2629](sonic-net/sonic-swss#2629)) * c54b3d1 Vxlan tunnel endpoint custom monitoring APPL DB table. ([sonic-net#2589](sonic-net/sonic-swss#2589)) * 7f03db2 Fix potential risks ([sonic-net#2516](sonic-net/sonic-swss#2516)) * 383ee68 [refactor]Refactoring sai handle status ([sonic-net#2621](sonic-net/sonic-swss#2621)) * cd95972 Fix issue 13341 ARP entry can be out of sync between kernel and APPL_DB if multiple updates are received from RTNL ([sonic-net#2619](sonic-net/sonic-swss#2619)) * a01470f Remove TODO comments that are no longer relevant ([sonic-net#2622](sonic-net/sonic-swss#2622)) * d058390 Changed the BFD default detect multiplier to 10x ([sonic-net#2614](sonic-net/sonic-swss#2614)) * d78b528 [MuxOrch] Enabling neighbor when adding in active state ([sonic-net#2601](sonic-net/sonic-swss#2601)) * 4ebdad1 [routesync] Fix for stale dynamic neighbor ([sonic-net#2553](sonic-net/sonic-swss#2553)) * 8857f92 Added new attributes for Vnet and Vxlan ecmp configurations. ([sonic-net#2584](sonic-net/sonic-swss#2584)) * b6bbc3e Revert [voq][chassis]Add show fabric counters port/queue commands (2522) ([sonic-net#2611](sonic-net/sonic-swss#2611)) * 52406e2 Add missing parameter to on_switch_shutdown_request method. ([sonic-net#2567](sonic-net/sonic-swss#2567)) * 4ac9ad9 Increase diff coverage to 80% ([sonic-net#2599](sonic-net/sonic-swss#2599)) * 8a0bb36 Handle Mac address 'none' ([sonic-net#2593](sonic-net/sonic-swss#2593)) * f496ab3 [vstest] Only collect stdout of orchagent_restart_check in vstest ([sonic-net#2597](sonic-net/sonic-swss#2597)) * 1dab495 Avoid aborting orchagent when setting TUNNEL attributes ([sonic-net#2591](sonic-net/sonic-swss#2591)) * 4395cea Fix neighbor doesn't update all attribute ([sonic-net#2577](sonic-net/sonic-swss#2577)) Signed-off-by: dprital <[email protected]>

Update sonic-swss submodule pointer to include the following: * a2a483d [acl] Add new ACL key BTH_OPCODE and AETH_SYNDROME ([#2617](sonic-net/sonic-swss#2617)) * 9d1f66b [bfdorch] add local discriminator to state DB ([#2629](sonic-net/sonic-swss#2629)) * c54b3d1 Vxlan tunnel endpoint custom monitoring APPL DB table. ([#2589](sonic-net/sonic-swss#2589)) * 7f03db2 Fix potential risks ([#2516](sonic-net/sonic-swss#2516)) * 383ee68 [refactor]Refactoring sai handle status ([#2621](sonic-net/sonic-swss#2621)) * cd95972 Fix issue 13341 ARP entry can be out of sync between kernel and APPL_DB if multiple updates are received from RTNL ([#2619](sonic-net/sonic-swss#2619)) * a01470f Remove TODO comments that are no longer relevant ([#2622](sonic-net/sonic-swss#2622)) * d058390 Changed the BFD default detect multiplier to 10x ([#2614](sonic-net/sonic-swss#2614)) * d78b528 [MuxOrch] Enabling neighbor when adding in active state ([#2601](sonic-net/sonic-swss#2601)) * 4ebdad1 [routesync] Fix for stale dynamic neighbor ([#2553](sonic-net/sonic-swss#2553)) * 8857f92 Added new attributes for Vnet and Vxlan ecmp configurations. ([#2584](sonic-net/sonic-swss#2584)) * b6bbc3e Revert [voq][chassis]Add show fabric counters port/queue commands (2522) ([#2611](sonic-net/sonic-swss#2611)) * 52406e2 Add missing parameter to on_switch_shutdown_request method. ([#2567](sonic-net/sonic-swss#2567)) * 4ac9ad9 Increase diff coverage to 80% ([#2599](sonic-net/sonic-swss#2599)) * 8a0bb36 Handle Mac address 'none' ([#2593](sonic-net/sonic-swss#2593)) * f496ab3 [vstest] Only collect stdout of orchagent_restart_check in vstest ([#2597](sonic-net/sonic-swss#2597)) * 1dab495 Avoid aborting orchagent when setting TUNNEL attributes ([#2591](sonic-net/sonic-swss#2591)) * 4395cea Fix neighbor doesn't update all attribute ([#2577](sonic-net/sonic-swss#2577)) Signed-off-by: dprital <[email protected]>

- What I did Fix the following potential risks: Uncaught exceptions in /src/nvos-swss/orchagent/main.cpp and /src/nvos-swss/portsyncd/portsyncd.cpp Uninitialized scalar fields in /src/nvos-swss/orchagent/port.h Unchecked return value in /src/nvos-swss/orchagent/portsorch.cpp Pointer to local outside scope in /src/nvos-swss/orchagent/portsorch.cpp - How I did it Add "catch" to the uncaught exceptions Add initialization value to the uninitialized fields Add a check to the unchecked return value Delete an unnecessary comma - How I verified it Inspection and sanity tests Signed-off-by: Liran Artzi <[email protected]>

Fix potential risks

120c8c9

Signed-off-by: Liran Artzi <[email protected]>

Liran-Ar requested a review from prsunny as a code owner November 13, 2022 15:32

prsunny reviewed Nov 14, 2022

View reviewed changes

liat-grozovik changed the title ~~Fix potential risks~~ Fix potential defects: Uncaught exceptions, Uninitialized scalar, Unchecked return value and Pointer to local outside scope Nov 28, 2022

Liran-Ar closed this Dec 7, 2022

Liran-Ar reopened this Dec 7, 2022

liat-grozovik added the Request for 202211 Branch label Jan 11, 2023

liat-grozovik approved these changes Jan 17, 2023

View reviewed changes

prsunny approved these changes Jan 17, 2023

View reviewed changes

liat-grozovik merged commit 7f03db2 into sonic-net:master Jan 23, 2023

liat-grozovik changed the title ~~Fix potential defects: Uncaught exceptions, Uninitialized scalar, Unchecked return value and Pointer to local outside scope~~ Fix potential defects Jan 23, 2023

Junchao-Mellanox mentioned this pull request Jan 30, 2023

[submodule] Update sonic-swss pointer sonic-net/sonic-buildimage#13538

Closed

dprital mentioned this pull request Jan 30, 2023

[submodule] Advance sonic-swss pointer sonic-net/sonic-buildimage#13551

Merged

8 tasks

StormLiangMS added the Included in 202211 Branch label Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix potential defects #2516

Fix potential defects #2516

Liran-Ar commented Nov 13, 2022 •

edited by liat-grozovik

Loading

prsunny Nov 14, 2022

Liran-Ar Nov 15, 2022 •

edited

Loading

Liran-Ar Dec 28, 2022

liat-grozovik Jan 5, 2023

liat-grozovik commented Nov 28, 2022

Liran-Ar commented Dec 7, 2022

azure-pipelines bot commented Dec 7, 2022

liat-grozovik commented Jan 11, 2023

liat-grozovik commented Jan 17, 2023

azure-pipelines bot commented Jan 17, 2023

prsunny commented Jan 17, 2023

qiluo-msft commented Jan 23, 2023

Fix potential defects #2516

Fix potential defects #2516

Conversation

Liran-Ar commented Nov 13, 2022 • edited by liat-grozovik Loading

prsunny Nov 14, 2022

Choose a reason for hiding this comment

Liran-Ar Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Liran-Ar Dec 28, 2022

Choose a reason for hiding this comment

liat-grozovik Jan 5, 2023

Choose a reason for hiding this comment

liat-grozovik commented Nov 28, 2022

Liran-Ar commented Dec 7, 2022

azure-pipelines bot commented Dec 7, 2022

liat-grozovik commented Jan 11, 2023

liat-grozovik commented Jan 17, 2023

azure-pipelines bot commented Jan 17, 2023

prsunny commented Jan 17, 2023

qiluo-msft commented Jan 23, 2023

Liran-Ar commented Nov 13, 2022 •

edited by liat-grozovik

Loading

Liran-Ar Nov 15, 2022 •

edited

Loading