-
Notifications
You must be signed in to change notification settings - Fork 415
Fix saisdkdump profile file handling. #1993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
judsonwilson-nvidia
wants to merge
4
commits into
sonic-net:master
Choose a base branch
from
judsonwilson-nvidia:fix_saisdkdump_profile
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| #include "ProfileMap.h" | ||
|
|
||
| #include <cerrno> | ||
| #include <cstring> | ||
| #include <fstream> | ||
|
|
||
| #include "swss/logger.h" | ||
|
|
||
| ProfileMap::ProfileMap() | ||
| : m_iter(m_map.begin()) | ||
| { | ||
| } | ||
|
|
||
| bool ProfileMap::loadFromFile(const std::string& profileMapFile) | ||
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| m_map.clear(); | ||
| m_iter = m_map.begin(); | ||
|
|
||
| if (profileMapFile.empty()) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| std::ifstream profile(profileMapFile); | ||
|
|
||
| if (!profile.is_open()) | ||
| { | ||
| SWSS_LOG_ERROR("failed to open profile map file: '%s' : %s", | ||
| profileMapFile.c_str(), strerror(errno)); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| std::string line; | ||
|
|
||
| while (getline(profile, line)) | ||
| { | ||
| if (!line.empty() && line.back() == '\r') | ||
| { | ||
| line.pop_back(); | ||
| } | ||
|
|
||
| if (line.size() > 0 && (line[0] == '#' || line[0] == ';')) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| size_t pos = line.find("="); | ||
|
|
||
| if (pos == std::string::npos) | ||
| { | ||
| SWSS_LOG_WARN("not found '=' in line %s", line.c_str()); | ||
| continue; | ||
| } | ||
|
|
||
| std::string key = line.substr(0, pos); | ||
| std::string value = line.substr(pos + 1); | ||
|
|
||
| m_map[key] = value; | ||
|
|
||
| SWSS_LOG_INFO("inserted: %s:%s", key.c_str(), value.c_str()); | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| void ProfileMap::clear() | ||
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| m_map.clear(); | ||
| m_iter = m_map.begin(); | ||
| } | ||
|
|
||
| const char* ProfileMap::getValue(const char* variable) const | ||
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| if (variable == NULL) | ||
| { | ||
| SWSS_LOG_WARN("variable is null"); | ||
| return NULL; | ||
| } | ||
|
|
||
| auto it = m_map.find(variable); | ||
|
|
||
| if (it == m_map.end()) | ||
| { | ||
| SWSS_LOG_NOTICE("%s: NULL", variable); | ||
| return NULL; | ||
| } | ||
|
|
||
| SWSS_LOG_NOTICE("%s: %s", variable, it->second.c_str()); | ||
|
|
||
| return it->second.c_str(); | ||
| } | ||
|
|
||
| int ProfileMap::getNextValue(const char** variable, const char** value) | ||
| { | ||
| SWSS_LOG_ENTER(); | ||
|
|
||
| if (value == NULL) | ||
| { | ||
| SWSS_LOG_INFO("resetting profile map iterator"); | ||
|
|
||
| m_iter = m_map.begin(); | ||
| return 0; | ||
| } | ||
|
|
||
| if (variable == NULL) | ||
| { | ||
| SWSS_LOG_WARN("variable is null"); | ||
| return -1; | ||
| } | ||
|
|
||
| if (m_iter == m_map.end()) | ||
| { | ||
| SWSS_LOG_INFO("iterator reached end"); | ||
| return -1; | ||
| } | ||
|
|
||
| *variable = m_iter->first.c_str(); | ||
| *value = m_iter->second.c_str(); | ||
|
|
||
| SWSS_LOG_INFO("key: %s:%s", *variable, *value); | ||
|
|
||
| m_iter++; | ||
|
|
||
| return 0; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| #pragma once | ||
|
|
||
| #include <map> | ||
| #include <string> | ||
|
|
||
| class ProfileMap | ||
| { | ||
| public: | ||
| ProfileMap(); | ||
|
|
||
| bool loadFromFile(const std::string& profileMapFile); | ||
|
|
||
| void clear(); | ||
|
|
||
| const char* getValue(const char* variable) const; | ||
|
|
||
| int getNextValue(const char** variable, const char** value); | ||
|
|
||
| private: | ||
| std::map<std::string, std::string> m_map; | ||
| mutable std::map<std::string, std::string>::iterator m_iter; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| SUBDIRS = meta lib vslib syncd proxylib saidump | ||
| SUBDIRS = meta lib vslib syncd proxylib saidump saisdkdump |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| AM_CXXFLAGS = $(SAIINC) -I$(top_srcdir)/saisdkdump | ||
|
|
||
| bin_PROGRAMS = tests | ||
|
|
||
| LDADD_GTEST = -L/usr/src/gtest -lgtest -lgtest_main | ||
|
|
||
| tests_SOURCES = main.cpp TestProfileMap.cpp | ||
|
|
||
| tests_CXXFLAGS = $(DBGFLAGS) $(AM_CXXFLAGS) $(CXXFLAGS_COMMON) $(CODE_COVERAGE_CXXFLAGS) | ||
| tests_LDADD = $(LDADD_GTEST) $(top_srcdir)/saisdkdump/libsaisdkdump_profile.a \ | ||
| -lswsscommon -lpthread $(CODE_COVERAGE_LIBS) | ||
|
|
||
| TESTS = tests |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Possible regression for the default (no-
-p) case.sai_profiledefaults to/tmp/sai.profile(not empty), and-pis optional.loadFromFilereturnsfalsewhen the file can't be opened, and here that maps toexit(EXIT_FAILURE). So if a user runssaisdkdumpwithout-pand/tmp/sai.profiledoesn't exist, the tool now hard-fails — whereas before this PR it ran fine (the callbacks just returned the path /-1and 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("")returnstrueand 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
-fhandling: track whether-pwas explicitly given (likefileSpecified) 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--pcase while still catching a genuine "pointed at a profile that doesn't exist" mistake.If the bluefield techsupport flow always passes
-pand the default-path fail-hard is acceptable/intended, feel free to confirm and disregard — just flagging sincesaisdkdumpis invoked as a general diagnostic tool.