Structured logging to journald - #2969
Draft
darkexplosiveqwx wants to merge 33 commits into
Draft
Conversation
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
darkexplosiveqwx
force-pushed
the
log-journal
branch
from
July 26, 2026 08:49
98971ad to
13547b5
Compare
|
Conflicts have been resolved. |
darkexplosiveqwx
force-pushed
the
log-journal
branch
from
July 26, 2026 09:15
13547b5 to
3b06c28
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
darkexplosiveqwx
force-pushed
the
log-journal
branch
from
July 30, 2026 19:42
3b06c28 to
7929ce9
Compare
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
5 tasks
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
the embedded dnsmasq already does a fd cleanup on startup, so this is not strictly necessary, but still good practice Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
message Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
The fd == -1 fast path in write_log_line() sat before the reopen_needed check, so SIGUSR2 could not revive a log whose initial open failed: a missing /var/log/pihole or a transient EACCES disabled that log for the lifetime of the process. Move the reopen before the fd test and access both fields only under the lock (flush_dnsmasq_log() reassigns the descriptor under the lock, so the unlocked fast path was racy). Also drop the fd == -1 early return in FTL_write_dnsmasq_log() which would have bypassed the reopen for pihole.log entirely. Code Review: **2. `write_log_line()` can never recover a log whose initial open failed.** The `if(log->fd == -1) return false;` fast path sits before the `reopen_needed` check, so `SIGUSR2` cannot revive it. A missing `/var/log/pihole` at first start, or a transient `EACCES`, disables that log for the lifetime of the process. Testing the reopen flag first fixes it. **Smaller:** `log->fd` is read outside the lock as a fast path while `flush_dnsmasq_log()` reassigns it under the lock Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
strftime("%b %e %H:%M:%S") into a 16 byte buffer overflows in
non-English locales: init_locale() calls setlocale(LC_ALL, ""), so %b
is the localized month abbreviation (six bytes for ru_RU, more
elsewhere). When it does not fit, strftime() returns 0 and leaves the
buffer contents unspecified, so pihole.log gets garbage. Use dnsmasq's
own idiom instead: ctime(&now) + 4 truncated to 15 characters. ctime()
renders the weekday/month in the C locale regardless of the process
locale, so this cannot overflow and keeps the on-disk format
byte-identical to what we write today.
Code Review:
**3. The dnsmasq timestamp buffer is too small for non-English locales.** `char ts_buf[16]` fits `"Jan 1 12:00:00"` exactly in the C locale, but `init_locale()` calls `setlocale(LC_ALL, "")`, so `%b` is the localized abbreviation - six bytes for `ru_RU`, more elsewhere. When it does not fit, `strftime()` returns 0 and leaves the buffer contents *unspecified*, so `pihole.log` gets garbage. dnsmasq itself sidesteps this with `ctime(&time_now) + 4` and `%.15s`, which is locale-independent and keeps the on-disk format byte-identical to what we write today.
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
open_log_fds() warned when webserver.log could not be opened but said nothing for pihole.log. A failed open therefore silently disabled every dnsmasq log line for the lifetime of the process. Add the matching warning so the missing log is visible in FTL.log at startup. Code Review: **4. dnsmasq messages have no fallback left.** `my_syslog()` returns before dnsmasq's own syslog path and `FTL_write_dnsmasq_log()` returns silently when the descriptor is -1, so if `pihole.log` cannot be opened every dnsmasq message is lost. `_FTL_log()` and `_log_web()` both fall back, and `open_log_fds()` warns for `webserver.log` but says nothing for `pihole.log`. Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
dnsmasq forks per TCP query while FTL threads are running. If a fork happens while another thread holds dnsmasq_log.lock, the child inherits it locked and the first my_syslog() there blocks forever, hanging that query. Register pthread_atfork() handlers that take all three per-file mutexes before fork() and release them in both parent and child, so a fork can never observe a locked log mutex. Code Review: **5. The per-file mutex is not fork-safe.** dnsmasq forks per TCP query while our threads are running. If a fork happens while another thread holds `dnsmasq_log.lock`, the child inherits it locked and the first `my_syslog()` there blocks forever, hanging that query. Our SHM lock is process-shared and robust for exactly this reason. Since every write is `O_APPEND`, and a single `write()` to a regular file is atomic, the lock only guards the reopen - `pthread_atfork()` handlers or an atomic descriptor swap would be enough. Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
open_log_fds(false) runs before FTL drops privileges, so a fresh install now gets root-owned webserver.log and pihole.log; dnsmasq used to create pihole.log after dropping to pihole, and webserver.log was created on the first request. FTL_fork_and_bind_sockets() chowned only files.log.ftl. Chown the webserver and dnsmasq log files alongside it when actually dropping from root. Code Review: **6. New root-owned log files.** `open_log_fds(false)` runs before we drop privileges, so a fresh install now gets root-owned `webserver.log` and `pihole.log`; dnsmasq used to create `pihole.log` after dropping to `pihole`, and `webserver.log` was created on the first request. `FTL_fork_and_bind_sockets()` chowns only `files.log.ftl` and should cover the other two. Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
my_syslog() bypasses dnsmasq's own logging path and FTL writes every pihole.log line synchronously through a cached descriptor, so the log-async queue is never populated. Emitting log-async in the generated config is misleading dead configuration. Code Review: **7. `log-async` is now dead code in the generated config.** With `my_syslog()` bypassed, dnsmasq's async queue is never used, so query logging became a blocking `write()` in the DNS thread - which is the stall that queue exists to prevent. Either drop the `log-async` lines or say why the synchronous write is acceptable for us. Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
The rest of the tree is ASCII; replace the em dashes introduced in the recent log comments with plain hyphens. Code Review: **Smaller:** five of the new comments contain em dashes while the rest of the tree is ASCII Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
The clamp that protects the writes into the 2048 byte line buffer ran after off had already been used as an offset into line and as the size argument (sizeof(line) - off would underflow if the first snprintf() ever truncated). Clamp off between the two formatting calls in FTL_write_dnsmasq_log(), _FTL_log() and _log_web(). Code Review: **Smaller:** `off` is used as an offset into `line` before it is clamped Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
The open failure warning is shown regardless of the hide_dnsmasq_warn setting, but the message now notes whether dnsmasq warnings are hidden by it, so the notice reflects the actual behaviour instead of always claiming warnings are relayed to the FTL log. Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
is_log_fd() was incorrectly stripped of __attribute__((pure)) in an earlier review-fix pass. The attribute is correct: pure means no side effects and may depend on global state (unlike const which is a pure function). GCC only caches the result when it can prove the read state hasn't changed, which it cannot for struct members accessed through pointers that other threads may modify. The compiler itself warns suggest-attribute=pure when the attribute is missing, confirming it belongs here. Also fix the ctime_r() return type: use const char* to match the fallback string literal and avoid -Wdiscarded-qualifiers. Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
free(log->path) is called before path has been set on the first invocation, triggering a spurious WARNING from the custom FTLfree() wrapper. Add a NULL check to avoid the noise. Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
log_to_json() runs on every log line when structured JSON logging is active. The cJSON path allocates an object, two cJSON_strdup()s per field, and cJSON_PrintUnformatted() with its growing buffer - roughly fifteen malloc/free pairs per line. The schema is six fixed keys, so emit it directly into a stack buffer with snprintf(). Only the message field needs JSON escaping; the other five are controlled by the code. A new json_escape() helper writes into a caller-supplied buffer so the log path needs no allocation. Valid UTF-8 passes through unchanged. Also use write(STDOUT_FILENO, ...) instead of printf() to avoid stdio buffering. Code Review: pi-hole#2968 (comment) Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
https://discourse.pi-hole.net/t/something-on-my-network-trying-to-contact-zoom/86982/11 is too funny, had to do it Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
TID= is specified for use in man:systemd.journal-fields(7) Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
darkexplosiveqwx
force-pushed
the
log-journal
branch
from
August 19, 2026 18:28
7929ce9 to
62beb50
Compare
|
Conflicts have been resolved. |
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
darkexplosiveqwx
force-pushed
the
log-journal
branch
from
August 19, 2026 19:59
30cc87d to
32a773c
Compare
Signed-off-by: darkexplosiveqwx <101737077+darkexplosiveqwx@users.noreply.github.com>
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Thank you for your contribution to the Pi-hole Community!
Please read the comments below to help us consider your Pull Request.
We are all volunteers and completing the process outlined will help us review your commits quicker.
Please make sure you
What does this PR aim to accomplish?:
Discussed in #2897
Stacked on top of #2958, #2960 and #2968
Requires pi-hole/docker-base-images#175 to build.
How does this PR accomplish the above?:
Link documentation PRs if any are needed to support this PR:
By submitting this pull request, I confirm the following:
git rebase)