Fix saisdkdump profile file handling. - #1993
Conversation
It was not parsing the kv pairs out of the file. This code is mostly copied from saidiscovery, and put into a separate file in a class for interfacing with unit tests. Signed-off-by: Judson Wilson <judsonw@nvidia.com>
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Judson Wilson <judsonw@nvidia.com>
2dac3a2 to
3b90ef3
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Judson Wilson <judsonw@nvidia.com>
aae2f88 to
765b654
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Signed-off-by: Judson Wilson <judsonw@nvidia.com>
d4cd17c to
7363cb5
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
2 similar comments
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
11 similar comments
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
|
Hi @sonic-net/sonic-sairedis-maintainer, this approved PR has workflow run(s) waiting for approval. Please help review. Thanks! ---Powered by SONiC BuildBot
|
lolyu
left a comment
lolyu
left a comment
There was a problem hiding this comment.
Reviewed at head 7363cb5. The core fix is correct and well-tested — profile_get_value previously returned the file path for every key and profile_get_next_value always returned -1, so -p was effectively dead; the new ProfileMap (modeled on saidiscovery) properly parses key=value, skips #/; comments, handles CRLF, and backs both callbacks with the right iterator semantics. Good gtest coverage and clean build wiring (.gitignore/configure.ac/Makefiles/unittest subdir).
One behavior concern on the default (no--p) invocation — inline. Otherwise LGTM; nice work.
| SWSS_LOG_INFO("The dump file is not specified, generated \"%s\" file name", fileName.c_str()); | ||
| } | ||
|
|
||
| if (!g_profileMap.loadFromFile(sai_profile)) |
There was a problem hiding this comment.
🟠 Possible regression for the default (no--p) case. sai_profile defaults to /tmp/sai.profile (not empty), and -p is optional. loadFromFile returns false when the file can't be opened, and here that maps to exit(EXIT_FAILURE). So if a user runs saisdkdump without -p and /tmp/sai.profile doesn't exist, the tool now hard-fails — whereas before this PR it ran fine (the callbacks just returned the path / -1 and SAI init proceeded). For a general techsupport/diagnostic tool, failing hard when no profile is present at the default path is a meaningful change.
Note the asymmetry that suggests this is unintended: loadFromFile("") returns true and loads nothing (a deliberate "no profile is fine" path), but the non-empty default defeats it, so that graceful branch is never taken unless the user explicitly passes -p "".
Suggest mirroring the existing -f handling: track whether -p was explicitly given (like fileSpecified) and only hard-fail on a missing file when the user explicitly requested one; tolerate absence of the default path (warn + empty map, continue). That keeps prior behavior for the common no--p case while still catching a genuine "pointed at a profile that doesn't exist" mistake.
If the bluefield techsupport flow always passes -p and the default-path fail-hard is acceptable/intended, feel free to confirm and disregard — just flagging since saisdkdump is invoked as a general diagnostic tool.
4 participants
Why I did it
saisdkdumpdid not parse the SAI profile file passed with-p:profile_get_valuereturned the file path for every key, andprofile_get_next_valuealways failed. A new feature on the nvidia-bluefield platform, DASH CT dumps from the SDK in techsupport, needs a profile key (CT_TABLE_DUMP_ENABLE) to reach SAI to trigger the CT dump in the SDK dump.Work item tracking
How I did it
ProfileMap(based onsaidiscovery) to parsekey=valuelines, skip comments, and back SAI profile callbacks.saisdkdumpbeforesai_api_initialize; buildlibsaisdkdump_profile.a.unittest/saisdkdump/(KV parsing, malformed lines, iterator reset, clear).How to verify it
./autogen.sh && ./configure --disable-static --disable-python2 --with-sai=vs make -C saisdkdump libsaisdkdump_profile.a make -C unittest/saisdkdump checkWhich release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
Description for the changelog
Fix saisdkdump profile file flag.