Skip to content

Add Linux dirty paging kernel parameters dump.#712

Open
eynhaender wants to merge 1 commit intolibbitcoin:masterfrom
eynhaender:feature/dump_kernel_parameters
Open

Add Linux dirty paging kernel parameters dump.#712
eynhaender wants to merge 1 commit intolibbitcoin:masterfrom
eynhaender:feature/dump_kernel_parameters

Conversation

@eynhaender
Copy link
Copy Markdown
Contributor

Add Linux dirty paging kernel parameters dump.

Comment thread console/dirty_pages.hpp
{
std::ifstream file("/proc/sys/vm/" + key);
T value{};
if (file >> value)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule we avoid using iostream for parse/serialize. We use the operators (in config classes) where we have overridden them, or when we are taking I/O via boost properties. This is because they are locale dependent, then to throw unexpectedly, and are slow. In this case the better approach is to use https://en.cppreference.com/cpp/utility/from_chars.

Comment thread console/dirty_pages.hpp
T value{};
if (file >> value)
return value;
return std::nullopt;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return can have no value...

Comment thread console/dirty_pages.hpp
{
dirty_page_params p{};
if (const auto v = read_proc_vm<uint64_t>("dirty_bytes"))
p.dirty_bytes = *v;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... in which case these dereferences will throw an exception and crash the application. So crash just from an unexpected value in the "file", or even missing file, which means all macos/windows. There needs to be consideration for how/what to present on Linux when these values are expected but one or more is failed.

% LIBBITCOIN_SYSTEM_VERSION);
}

void executor::dump_kernel_parameters() const
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the place to implement the exclusion for platforms that don't support these features.

capture_.start();
dump_version();
dump_hardware();
dump_kernel_parameters();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about we are mosre specific and call this dump_paging()

Comment thread console/dirty_pages.hpp
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
#ifndef LIBBITCOIN_BS_DIRTY_PAGES_HPP
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be implemented in a header file, as it's neither inline nor template. The read_proc_vm<> template method is a local utility (cpp). So this should follow the existing pattern and be implemented in executor_paging.cpp, with prototype in executor.hpp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants