support cgroups pathnames with colons #68
Conversation
| @dtrugman could you have a look any time soon? |
| Thanks for the comment. Managed to miss this. |
src/utils.cpp Outdated
| size_t index = buffer.find(delim, curr); | ||
| if (index == std::string::npos) | ||
| { | ||
| out.emplace_back(buffer.substr(curr)); |
There was a problem hiding this comment.
Let's avoid the extra copy by passing the iterators directly? Something like this should work
out.emplace_back(buffer.begin() + curr, buffer.begin() + index); There was a problem hiding this comment.
Changed but the resulting string will be moved so there is no extra copy, so performance gain is hardly noticeable while readability is decreased
| return out; | ||
| } | ||
| | ||
| std::pair<std::string, std::string> split_once(const std::string& buffer, |
There was a problem hiding this comment.
Let's update this implementation to use split_times?
There was a problem hiding this comment.
split_once always returns 2 elements, even if there is no delimiter. split_times returns 1 element if there is no delimiter.
| { | ||
| std::string input; | ||
| std::vector<std::string> tokens; | ||
| char delim = ' '; |
There was a problem hiding this comment.
this should not have a default value, same as times.
every test should set those.
There was a problem hiding this comment.
I don't mind removing the default value, but this will generate the uninitialized warning and setting default values for the variables of basic types is consistent with other tests in this file.
test/test_utils.cpp Outdated
| times = 1; | ||
| } | ||
| | ||
| REQUIRE(split_times(input, delim, times) == tokens); |
There was a problem hiding this comment.
[nitpick] let's extract the method call out of the REQUIRE clause
tokens = split_times(input, delim, times) There was a problem hiding this comment.
doesn't make a difference for me, but in most tests here the method call is within the REQUIRE clause
test/test_utils.cpp Outdated
| TEST_CASE("Split times", "[utils]") | ||
| { | ||
| std::string input; | ||
| std::vector<std::string> tokens; |
| | ||
| auto tokens = utils::split(line, DELIM, /* keep_empty = */ true); | ||
| if (tokens.size() != COUNT) | ||
| auto tokens = utils::split_times(line, DELIM, COUNT - 1); |
There was a problem hiding this comment.
[comment belongs to line 42]
Let's add an example where this actually matters?
| | ||
| auto tokens = utils::split(line, DELIM, /* keep_empty = */ true); | ||
| if (tokens.size() != COUNT) | ||
| auto tokens = utils::split_times(line, DELIM, COUNT - 1); |
There was a problem hiding this comment.
Let's add a comment here clearly explaining why we use split_times(..., COUNT - 1) split here instead of split.
| auto tokens = utils::split(line, DELIM, /* keep_empty = */ true); | ||
| if (tokens.size() != COUNT) | ||
| auto tokens = utils::split_times(line, DELIM, COUNT - 1); | ||
| if (tokens.size() < COUNT) |
There was a problem hiding this comment.
This can never happen, right? So we should remove this.
There was a problem hiding this comment.
Do you mean it can't happen from C++ perspective or from Linux perspective? From C++ perspective it can definitely happen because split_times can return less (but not more) than COUNT elements. From Linux perspective it shouldn't but this might change or there can be some case which we didn't account for
| Thanks for the PR @thisptr-hub ! |
| @dtrugman thanks a lot for the review! Sorry it took me so long, but I addressed some of the review comments and responded to the others so please take another look |
2 participants
When using CentOS 9 I encountered cgroup paths with colons for multiple Gnome apps:
this pull request prevents errors when encountering cgroup paths with colons in them