Skip to content
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

Maintenance: Add dedicated /etc/hosts manager #1810

Draft
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

yadij
Copy link
Contributor

@yadij yadij commented May 7, 2024

Replace the legacy parser for /etc/hosts file using
a modern Tokenizer and SBuf based parse managed
by a Registered Runner class.

yadij and others added 27 commits May 8, 2024 03:16
Initial class Cfg::File combines:

a) file management design from ConfigParser::CfgFile,

Extended to load files in large chunks (up to 1MB) and
parse lines in memory instead of via fgets().

b) "#line NN "filename" line changing from parseOneConfigFile(),

Altered to only apply when processing FIFO pipes instead
of allowing regular files.

c) FIFO pipe alternative input from parseOneConfigFile()

Extended to also perform FIFO auto-detection and enable
pipe handling differences without special configuration by
admin.

d) prefix whitespace tolerance from ConfigParser::strtokFile()
Apparently it can contain garbage values.
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
- content may not actually be configuration. It has not been validated.
 - fread(2) is used internally not read(2).
also remove now unused stat(2) complexity.
Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>
@yadij
Copy link
Contributor Author

yadij commented May 7, 2024

Based on PR #928, the additions for this PR are in commit #8f03367

@yadij yadij added S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels labels May 7, 2024
@yadij yadij marked this pull request as draft May 7, 2024 16:18
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Based on PR #928

Please do not post PRs based on PR 928. That PR is likely to require significant changes, making such posts unproductive.

I marked a few problems, but please request my review after PR 928 is closed and this PR is updated accordingly, even if those problems are addressed earlier.

@@ -31,7 +31,7 @@
#include "CpuAffinity.h"
#include "debug/Messages.h"
#include "DiskIO/DiskIOModule.h"
#include "dns/forward.h"
#include "dns/EtcHosts.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

The newer Registrater Runner API (commit 230d441) that this PR is using does not require exposing main.cc to EtcHosts details. The following suggestion undoes PR changes:

Suggested change
#include "dns/EtcHosts.h"
#include "dns/forward.h"

@@ -10151,9 +10151,9 @@ DOC_START
DOC_END

NAME: hosts_file
TYPE: string
TYPE: SBuf
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT, this TYPE change (among other things) converts a previously required filename parameter to an optional one. Please do not do that.

FWIW, I recommend keeping Config.etcHostsPath (field and type) unchanged in this PR. There are other problems with parse/dump_SBuf() that we would be able to avoid solving in this PR if we remove that non-essential change from this PR.

if (Path.isEmpty() || Path.cmp("none") == 0)
return;

etcHostsFile = new Configuration::File(Path.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use a raw pointer to store heap-allocated objects.

AFAICT, the proposed Dns::EtcHosts::etcHostsFile data member should be converted into an ordinary local non-pointer variable instead, eliminating this problem and simplifying code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants