Repository navigation
Symlink /etc/localtime and decide the hardware clock mode - #28
Merged
Merged
Conversation
Reviewer's GuideLocalization now installs /etc/localtime as a validated absolute symlink with persistent zone metadata and explicitly configures the RTC as UTC or local time, defaulting new installs to UTC while preserving upgrade state unless overridden; sample configurations document the new option. Sequence diagram for installed timezone and RTC configurationsequenceDiagram
participant Installer
participant run_localize
participant set_timezone
participant Filesystem
participant set_utc_clock
Installer->>run_localize: run_localize
run_localize->>run_localize: Parse timezone and utcClock
opt timezone configured
run_localize->>set_timezone: set_timezone(TZONE)
set_timezone->>Filesystem: Check zoneinfo file
alt zone exists
set_timezone->>Filesystem: ln -sf /usr/share/zoneinfo/TZONE /etc/localtime
set_timezone->>Filesystem: echo TZONE > /var/db/zoneinfo
else zone missing
set_timezone-->>run_localize: return 1
end
end
alt new install or utcClock explicitly configured
run_localize->>set_utc_clock: set_utc_clock(UTCCLOCK)
alt utcClock is no
set_utc_clock->>Filesystem: touch /etc/wall_cmos_clock
else utcClock is yes or absent on new install
set_utc_clock->>Filesystem: rm -f /etc/wall_cmos_clock
end
else upgrade without utcClock
run_localize-->>Installer: Preserve existing RTC mode
end
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="backend/functions-localize.sh" line_range="216" />
<code_context>
- cp ${FSMNT}/usr/share/zoneinfo/${TZONE} ${FSMNT}/etc/localtime
- echo ${TZONE} | tee ${FSMNT}/var/db/zoneinfo > /dev/null
+
+ if [ ! -e "${FSMNT}/usr/share/zoneinfo/${TZONE}" ] ; then
+ echo_log "WARNING: timezone ${TZONE} not found, system left on UTC"
+ return 1
+ fi
+
</code_context>
<issue_to_address>
**issue (bug_risk):** When `timeZone=` names a nonexistent zone, `set_timezone` returns before changing `/etc/localtime`, so the installed system retains the copied live-system timezone instead of being left on UTC as the warning claims.
**Triggers:** When the live image's `/etc/localtime` is configured for a non-UTC zone and the configuration contains an invalid `timeZone=` value.
**Suggested fix:** Remove or explicitly replace `/etc/localtime` with a UTC configuration when rejecting the zone, or change the warning to describe the actual retained state and fail the localization step.
```suggestion
echo_log "WARNING: timezone ${TZONE} not found, system retains the live-system timezone"
```
</issue_to_address>
### Comment 2
<location path="backend/functions-localize.sh" line_range="215" />
<code_context>
- cp ${FSMNT}/usr/share/zoneinfo/${TZONE} ${FSMNT}/etc/localtime
- echo ${TZONE} | tee ${FSMNT}/var/db/zoneinfo > /dev/null
+
+ if [ ! -e "${FSMNT}/usr/share/zoneinfo/${TZONE}" ] ; then
+ echo_log "WARNING: timezone ${TZONE} not found, system left on UTC"
+ return 1
+ fi
+
+ # /etc/localtime has to be a symlink, the same way tzsetup(8) creates it.
+ # A copy freezes the zone at install time, so tzdata updates never reach
+ # the system, and tools that read the zone name from the link target see
+ # nothing configured. The target is absolute on purpose: it must not be
+ # prefixed with ${FSMNT} or the link dangles on the installed system.
+ rm -f ${FSMNT}/etc/localtime
+ ln -s /usr/share/zoneinfo/${TZONE} ${FSMNT}/etc/localtime
+
+ # Saved for tzsetup -r and for the login / lock screens
</code_context>
<issue_to_address>
**issue (bug_risk):** The existence check accepts directories under `/usr/share/zoneinfo`, such as `America`, and then creates `/etc/localtime` as a symlink to that directory; the kernel and timezone readers cannot use a directory as a TZif timezone file.
**Triggers:** When `timeZone=` contains an existing zoneinfo directory rather than a timezone file.
**Suggested fix:** Require a regular file, for example with `-f`, before creating the symlink.
```suggestion
if [ ! -f "${FSMNT}/usr/share/zoneinfo/${TZONE}" ] ; then
```
</issue_to_address>
### Comment 3
<location path="backend/functions-localize.sh" line_range="225-226" />
<code_context>
+ # the system, and tools that read the zone name from the link target see
+ # nothing configured. The target is absolute on purpose: it must not be
+ # prefixed with ${FSMNT} or the link dangles on the installed system.
+ rm -f ${FSMNT}/etc/localtime
+ ln -s /usr/share/zoneinfo/${TZONE} ${FSMNT}/etc/localtime
+
+ # Saved for tzsetup -r and for the login / lock screens
</code_context>
<issue_to_address>
**issue (bug_risk):** If `rm` succeeds but `ln -s` fails, `/etc/localtime` is left missing and the failure is not logged or propagated because callers ignore `set_timezone`'s status; localization then continues with no configured local timezone.
**Triggers:** When the target filesystem is read-only, full, or otherwise rejects the symlink operation.
**Suggested fix:** Create and verify the symlink before removing the old target where possible, and check the `rm`/`ln` results so `run_localize` reports or aborts on failure.
</issue_to_address>
### Comment 4
<location path="backend/functions-localize.sh" line_range="238-243" />
<code_context>
+# install is a copy of the live system, so "yes" means removing it.
+set_utc_clock()
+{
+ if [ "$1" = "no" -o "$1" = "NO" ] ; then
+ echo_log "Hardware clock keeps local time"
+ touch ${FSMNT}/etc/wall_cmos_clock
+ else
+ echo_log "Hardware clock keeps UTC"
+ rm -f ${FSMNT}/etc/wall_cmos_clock
+ fi
};
</code_context>
<issue_to_address>
**issue (bug_risk):** Every value other than case-sensitive `no` or `NO` selects UTC, so an invalid value or typo such as `utcClock=n` silently removes `/etc/wall_cmos_clock` instead of rejecting the configuration.
**Triggers:** When a frontend emits a value other than the documented `yes` or `no` values.
**Suggested fix:** Validate `UTCCLOCK` explicitly as `yes` or `no` and fail with a configuration error for any other value.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and the installer now persists both the timezone symlink and the hardware-clock mode, so reverting the code would not restore the previous installed state. A wrong choice could cause clock shifts after reboot and affect time-dependent services, but the configuration and resulting clock can be corrected by reconfiguring the installed system.
Blocking findings: backend/functions-localize.sh:216, backend/functions-localize.sh:215, backend/functions-localize.sh:226, backend/functions-localize.sh:243
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
/etc/localtime was written with cp. That freezes the zone at install time, so tzdata updates never reach the installed system, and it leaves the zone without a name for anything that reads the link target, which is why the timezone looks unset after an install. It is now a symlink into /usr/share/zoneinfo, the way tzsetup(8) creates it. A zone that does not exist is reported instead of failing silently, and /var/db/zoneinfo is still written for tzsetup -r and the login screens. Nothing decided whether the RTC keeps UTC or local time. Every image ships /etc/wall_cmos_clock and the install is a cpdup of the live system, so the installed kernel read the RTC as local time and the clock was off by one UTC offset on any machine whose RTC actually keeps UTC. tzsetup was the only tool that ever asked the question, which is why running it appeared to fix a timezone that was already correct. Add a utcClock=yes|no config key that creates or removes that file. It defaults to yes and is applied even when the key is absent, so front-ends that do not send it still get a UTC clock. An upgrade keeps whatever the running system had unless the config asks for a change. Resolves: ghostbsd/issues#390
ericbsd
force-pushed
the
fix-390-timezone-clock
branch
from
August 25, 2026 22:18
64ec145 to
864fcc3
Compare
Reject a timeZone= that names a zoneinfo directory such as America. The -e test accepted one, and /etc/localtime then pointed at a directory, which is not a timezone the kernel can read. Link with ln -sf and report a failure, so a failed link no longer leaves the system with no /etc/localtime and nothing in the log. The warning for a missing zone no longer claims the system is left on UTC, since what it is left on depends on the live image rather than on anything this function did. Accept utcClock= in any case and warn on an unrecognised value instead of quietly treating it as yes. utcClock=No selected UTC, the opposite of what it asks for.
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="backend/functions-localize.sh" line_range="247-253" />
<code_context>
+# install is a copy of the live system, so "yes" means removing it.
+set_utc_clock()
+{
+ UTCVAL=`echo "$1" | tr '[:upper:]' '[:lower:]'`
+
+ if [ "${UTCVAL}" != "yes" -a "${UTCVAL}" != "no" ] ; then
+ echo_log "WARNING: utcClock is set to unknown value $1, using yes"
+ UTCVAL="yes"
+ fi
+
+ if [ "${UTCVAL}" = "no" ] ; then
</code_context>
<issue_to_address>
**issue (bug_risk):** Failures from `touch` and `rm -f` are ignored, so the installer logs that the hardware clock uses the requested mode even when it cannot create or remove `${FSMNT}/etc/wall_cmos_clock`; the installed system then retains the wrong RTC interpretation without an installation error.
**Triggers:** When the target filesystem is read-only, lacks the expected `/etc` path, or an existing file cannot be removed.
**Suggested fix:** Check the return status of `touch` and `rm`, log the failure, and return a nonzero status when the requested state cannot be applied.
```suggestion
if [ "${UTCVAL}" = "no" ] ; then
if ! touch ${FSMNT}/etc/wall_cmos_clock ; then
echo_log "WARNING: failed to create ${FSMNT}/etc/wall_cmos_clock"
return 1
fi
echo_log "Hardware clock keeps local time"
else
if ! rm -f ${FSMNT}/etc/wall_cmos_clock ; then
echo_log "WARNING: failed to remove ${FSMNT}/etc/wall_cmos_clock"
return 1
fi
echo_log "Hardware clock keeps UTC"
fi
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and a wrong timezone link or RTC mode can make the installed system interpret time incorrectly, causing clock shifts and incorrect timestamps that remain after reverting the installer. The resulting configuration is otherwise bounded and can be repaired by correcting the link or wall_cmos_clock setting and resynchronizing time.
Blocking findings: backend/functions-localize.sh:253
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
set_ntp ran rc-update add ntpd default, which is OpenRC and does not
exist on FreeBSD, so enabling was a silent no-op. NTP works only because
the image ships ntpd_enable="YES" and the install inherits it.
Disabling was inverted: it grepped rc.conf for ntpd_enable="YES" and ran
its sed only when the grep failed, so enableNTP=no cleared the setting
exactly when it was absent.
Both paths now use sysrc -f against the target rc.conf, the way the rest
of the backend edits files under ${FSMNT}. The INSTALLTYPE=FreeBSD branch
is gone, since sysrc covers both and creates rc.conf when missing.
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.
/etc/localtime was written with cp. That freezes the zone at install time, so tzdata updates never reach the installed system, and it leaves the zone without a name for anything that reads the link target, which is why the timezone looks unset after an install. It is now a symlink into /usr/share/zoneinfo, the way tzsetup(8) creates it. A zone that does not exist is reported instead of failing silently, and /var/db/zoneinfo is still written for tzsetup -r and the login screens.
Nothing decided whether the RTC keeps UTC or local time. Every image ships /etc/wall_cmos_clock and the install is a cpdup of the live system, so the installed kernel read the RTC as local time and the clock was off by one UTC offset on any machine whose RTC actually keeps UTC. tzsetup was the only tool that ever asked the question, which is why running it appeared to fix a timezone that was already correct.
Add a utcClock=yes|no config key that creates or removes that file. It defaults to yes and is applied even when the key is absent, so front-ends that do not send it still get a UTC clock. An upgrade keeps whatever the running system had unless the config asks for a change.
Resolves: ghostbsd/issues#390
Summary by Sourcery
Configure installed timezone links and hardware clock mode reliably during localization.
New Features:
Bug Fixes:
Enhancements:
Summary by Sourcery
Configure installed timezone links and hardware clock behavior reliably during localization.
New Features:
Bug Fixes:
Enhancements:
Documentation: