Conversation
Author
openwrt-ai
reviewed
Sep 23, 2026
openwrt-ai
left a comment
openwrt-ai
left a comment
Collaborator
There was a problem hiding this comment.
Commit checks
- 3825bbc "luci-app-keepalived: don't write alt_config_file unless requested" — the stated root cause ("
.defaultis submitted as the field's value whenever it renders with no existing UCI value") contradictsform.js:2152, which removes anoptionaloption whose form value equals.default. Please re-check the mechanism and rewrite the rationale against what the code actually does, or name the LuCI revision where the write happens.
Generated by Claude Code
Comment on lines
38
to
39
| o.optional = true; | ||
| o.default = '/etc/keepalived/keepalived.conf'; |
Collaborator
There was a problem hiding this comment.
With optional = true, a form value equal to .default is never written: form.js:2152 takes the remove() branch when formvalue() == this.default. That makes /etc/keepalived/keepalived.conf the one path a user can never actually select and persist, and dropping the default is the smaller fix for the unwanted write too.
Suggested change
| o.optional = true; | |
| o.default = '/etc/keepalived/keepalived.conf'; | |
| o.optional = true; |
Generated by Claude Code
| o.optional = true; | ||
| o.placeholder = 'OpenWrt'; | ||
|
|
||
| o = s.option(form.Flag, 'use_alt_config_file', _('Use alternative config file'), |
Collaborator
There was a problem hiding this comment.
This gets persisted as option use_alt_config_file '1' in /etc/config/keepalived, but no consumer reads it — the name appears nowhere in keepalived.init. Is a UI-only marker in the config file intended, rather than deriving the state from alt_config_file alone as cfgvalue() below already does?
Generated by Claude Code
| o.default = o.disabled; | ||
| o.rmempty = true; | ||
| o.cfgvalue = function(section_id) { | ||
| var v = form.Flag.prototype.cfgvalue.apply(this, [section_id]); |
Collaborator
There was a problem hiding this comment.
nit: var — this file and current luci JS use let/const. Same on line 31.
Suggested change
| var v = form.Flag.prototype.cfgvalue.apply(this, [section_id]); | |
| const v = form.Flag.prototype.cfgvalue.apply(this, [section_id]); |
Generated by Claude Code
| } | ||
| return v; | ||
| }; | ||
|
|
Collaborator
There was a problem hiding this comment.
The "Alternative config" FileUpload field set:
o.default = '/etc/keepalived/keepalived.conf';
For form.Value-derived widgets, .default is submitted as the
field's value whenever it renders with no existing UCI value --
it is not merely a placeholder. As a result, saving the Global
Settings page for the first time, without the user ever
touching this field, silently wrote
globals.alt_config_file into /etc/config/keepalived.
Once that option is set, keepalived.init's process_config()
symlinks the runtime config straight to that file and skips
generating it from the rest of the UCI sections (vrrp_instance,
ipaddress, track_interface, peer, ...) entirely, so every other
setting configured through LuCI is silently ignored and the
daemon runs whatever native config happens to be at that path.
Add an explicit "Use alternative config file" flag, defaulting
to off, and only show/write alt_config_file when it is enabled.
For upgrades where alt_config_file is already present from a
prior save, default the new flag to on so existing,
intentionally-configured setups are preserved rather than
silently cleared.
Signed-off-by: Jackie Han <jackie.han@gmail.com>
Jackie264
force-pushed
the
enhance-luci-app-keepalived
branch
from
September 24, 2026 22:38
3825bbc to
10b0d4e
Compare
This branch has not been deployed
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.
The "Alternative config" FileUpload field set:
For form.Value-derived widgets, .default is submitted as the field's value whenever it renders with no existing UCI value -- it is not merely a placeholder. As a result, saving the Global Settings page for the first time, without the user ever touching this field, silently wrote
globals.alt_config_file into /etc/config/keepalived.
Once that option is set, keepalived.init's process_config() symlinks the runtime config straight to that file and skips generating it from the rest of the UCI sections (vrrp_instance, ipaddress, track_interface, peer, ...) entirely, so every other setting configured through LuCI is silently ignored and the daemon runs whatever native config happens to be at that path.
Add an explicit "Use alternative config file" flag, defaulting to off, and only show/write alt_config_file when it is enabled. For upgrades where alt_config_file is already present from a prior save, default the new flag to on so existing, intentionally-configured setups are preserved rather than silently cleared.
Tested on
OpenWrt version: 25.12.5 r33051-f5dae5ece4
LuCI version: 26.263.44884~0834d09
Web browser(s): Safari 27.0 (22625.1.29.11.27)
Before
After
Signed-off-by: Jackie Han jackie.han@gmail.com