smart-reboot: add network-idle aware scheduled reboot utility - #28586
smart-reboot: add network-idle aware scheduled reboot utility#28586minicom365 wants to merge 1 commit into
Conversation
31b0d1e to
a8cda07
Compare
|
Thanks for the suggestion. I rewrote Changes included:
Please take another look when you have time. 감사합니다. |
| let sample_seconds = int(cfg_get("sample_seconds", "120")); | ||
| let byte_threshold = int(cfg_get("byte_threshold", "262144")); | ||
| let all_ifaces = cfg_get("all_ifaces", "0"); | ||
| let ifaces = parse_ifaces(cmd_out(`uci -q get ${CFG}.${SECTION}.ifaces`)); |
| function cfg_get(key, def) { | ||
| let val = cmd_out(`uci -q get ${CFG}.${SECTION}.${key}`); | ||
| return length(val) ? val : def; | ||
| } |
| ifaces = list_all_ifaces(); | ||
| } | ||
| else if (!length(ifaces)) { | ||
| let wan_dev = cmd_out("uci -q get network.wan.device"); |
| let delta = end_bytes - start_bytes; | ||
|
|
||
| if (delta <= byte_threshold) { | ||
| logger(`Network idle detected (delta=${delta} bytes, threshold=${byte_threshold}), rebooting now`); |
| system("/sbin/reboot"); | ||
| } | ||
| else { | ||
| logger(`Skip reboot, network is active (delta=${delta} bytes, threshold=${byte_threshold})`); |
| SECTION:=utils | ||
| CATEGORY:=Utilities | ||
| TITLE:=Smart reboot based on network idle state | ||
| DEPENDS:=+busybox +uci +ucode |
There was a problem hiding this comment.
Formatting is off here
| /etc/config/smart-reboot | ||
| endef | ||
|
|
||
| define Build/Compile |
There was a problem hiding this comment.
I think add a true here can prevent warning
|
Applied follow-up review updates and force-pushed. Latest commit:
Thanks again for the detailed feedback. |
| let wan_dev = cursor.get("network", "wan", "device"); | ||
| if (length(wan_dev)) | ||
| ifaces = [ wan_dev ]; | ||
| } |
There was a problem hiding this comment.
does it have to be the wan device?
Some users have multiple wan, so making this configurable is a good idea.
b9d909e to
8d31c86
Compare
|
Good point, thanks. I removed the hardcoded Latest commit: New behavior when
This should better support multi-WAN/custom setups. |
| start_service() { | ||
| [ -f "$CRON_FILE" ] || touch "$CRON_FILE" | ||
| gen_schedule | ||
| /etc/init.d/cron restart >/dev/null 2>&1 |
There was a problem hiding this comment.
is a restart necessary or does a reload work just as well?
There was a problem hiding this comment.
cron restart is not strictly necessary; reload is sufficient and less disruptive when supported. Updated to prefer reload and fall back to restart if reload is unavailable/fails.
| function main() { | ||
| log.openlog("smart-reboot", log.LOG_PID); | ||
|
|
||
| if (system(`lock -n ${LOCK_FILE} >/dev/null 2>&1`) != 0) |
There was a problem hiding this comment.
ucode has a lock function:
|
|
||
| let enabled = cfg_get("enabled", "0"); | ||
| if (enabled != "1") { | ||
| system(`lock -u ${LOCK_FILE}`); |
There was a problem hiding this comment.
|
|
||
| let now = localtime(time()); | ||
| if (now.hour != hour || now.min != minute) { | ||
| system(`lock -u ${LOCK_FILE}`); |
There was a problem hiding this comment.
| logger(`Skip reboot, network is active (delta=${delta} bytes, threshold=${byte_threshold})`); | ||
| } | ||
|
|
||
| system(`lock -u ${LOCK_FILE}`); |
There was a problem hiding this comment.
| } | ||
|
|
||
| let start_bytes = sum_iface_bytes(ifaces); | ||
| system(`sleep ${sample_seconds}`); |
There was a problem hiding this comment.
| } | ||
| else if (!length(ifaces)) { | ||
| logger("No monitored interfaces configured. Set smart-reboot.settings.ifaces or enable all_ifaces."); | ||
| system(`lock -u ${LOCK_FILE}`); |
There was a problem hiding this comment.
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the full package (new submission, no prior automated review). CI grounding: FormalityCheck / OpenWrt Makefiles and Git & Commits are failing on the head commit. See inline notes; the most actionable is the missing PKG_MAINTAINER/PKG_LICENSE which blocks the Makefile formality check.
Generated by Claude Code
| include $(TOPDIR)/rules.mk | ||
|
|
||
| PKG_NAME:=smart-reboot | ||
| PKG_RELEASE:=4 |
There was a problem hiding this comment.
This Makefile is missing PKG_MAINTAINER and PKG_LICENSE, which CONTRIBUTING.md requires for every package (see the "PKG_MAINTAINER" / "PKG_LICENSE" sections). This is why FormalityCheck / OpenWrt Makefiles is currently failing on the head commit. Please add them, e.g.:
PKG_RELEASE:=4
PKG_MAINTAINER:=Your Name <3387910@naver.com>
PKG_LICENSE:=GPL-2.0-or-later
Also consider adding a PKG_VERSION:=1 (see the PKG_VERSION row in CONTRIBUTING.md); local-only packages in this feed conventionally set one.
Generated by Claude Code
| } | ||
|
|
||
| function set_last_auto_reboot() { | ||
| let ts = cmd_out("date '+%Y-%m-%d %H:%M:%S'"); |
There was a problem hiding this comment.
Shelling out to date via fs.popen re-introduces an external-command dependency that the earlier commits deliberately removed (ucode lock/sleep/uci/log were adopted for exactly this reason). The timestamp can be formatted natively from localtime():
| let ts = cmd_out("date '+%Y-%m-%d %H:%M:%S'"); | |
| let t = localtime(); | |
| let ts = sprintf("%04d-%02d-%02d %02d:%02d:%02d", | |
| t.year, t.mon, t.mday, t.hour, t.min, t.sec); |
With this change, the cmd_out() helper (lines 16-24) and the shquote() helper (lines 12-14, already unused) both become dead code and can be dropped.
Generated by Claude Code
| @@ -0,0 +1,8 @@ | |||
| config settings 'settings' | |||
| option enabled '1' | |||
There was a problem hiding this comment.
Question: shipping enabled '1' (together with time '04:00') means that simply installing the package arms an unattended nightly reboot by default, since postinst enables and starts the service. Is that the intended default? Many admins would find an out-of-the-box package that reboots the router surprising; defaulting enabled '0' and letting the user opt in would be safer. If this is deliberate, feel free to disregard.
Generated by Claude Code
|
This PR is stale because it has been inactive for 14 days and has the "not following guidelines" label. |
dc03713 to
3844583
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
The branch was squashed/rewritten since the previous automated review (that commit is no longer reachable), so this is a fresh pass over the current single commit. The commit message matches its diff. Two failing formality checks remain on this head; see inline notes for the actionable OpenWrt Makefiles cause (missing PKG_VERSION/PKG_MAINTAINER/PKG_LICENSE) plus a missing ucode-mod-fs runtime dependency.
Generated by Claude Code
| PKG_NAME:=smart-reboot | ||
| PKG_RELEASE:=4 |
There was a problem hiding this comment.
The FormalityCheck / OpenWrt Makefiles check fails because required metadata is missing: PKG_VERSION (formalities check_pkg_version) and PKG_MAINTAINER + PKG_LICENSE (check_openwrt_meta). Every in-tree package sets these — e.g. utils/watchcat/Makefile. Set PKG_LICENSE to the SPDX identifier of the license you actually intend (the value below is only a placeholder):
| PKG_NAME:=smart-reboot | |
| PKG_RELEASE:=4 | |
| PKG_NAME:=smart-reboot | |
| PKG_VERSION:=1 | |
| PKG_RELEASE:=4 | |
| PKG_MAINTAINER:=Min Choi <3387910@naver.com> | |
| PKG_LICENSE:=GPL-2.0-or-later |
Generated by Claude Code
| SECTION:=utils | ||
| CATEGORY:=Utilities | ||
| TITLE:=Smart reboot based on network idle state | ||
| DEPENDS:=+busybox +uci +ucode +ucode-mod-uci +ucode-mod-log |
There was a problem hiding this comment.
The check script imports the fs module and relies on fs.open, fs.glob, fs.popen and fs.readfile, but fs is a separately-packaged loadable module (ucode-mod-fs in openwrt.git), not built into the interpreter. Without +ucode-mod-fs the script aborts at import * as fs from "fs" on a minimal install. Add the dependency:
| DEPENDS:=+busybox +uci +ucode +ucode-mod-uci +ucode-mod-log | |
| DEPENDS:=+busybox +uci +ucode +ucode-mod-uci +ucode-mod-log +ucode-mod-fs |
Generated by Claude Code
| function shquote(s) { | ||
| return "'" + replace(s, /'/g, "'\\''") + "'"; | ||
| } |
There was a problem hiding this comment.
nit: shquote() is defined here but never called anywhere in the script — it can be removed as dead code.
Generated by Claude Code
91bf913 to
b0b5093
Compare
|
This pull request has been marked stale because it has the "not following guidelines" label and has seen no activity for 14 days. |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the one new commit since the last automated pass (b0b5093..3cc8c72).
The one finding I would call merge-blocking is the removal of +ucode-mod-fs from DEPENDS while the check script still imports fs — that leaves the installed package non-functional on a minimal image. The rest are questions or cosmetics.
Commit checks
- 3cc8c72 "smart-reboot: fix Makefile audit issues" — the message describes three changes (add
PKG_LICENSE_FILES, setPKG_RELEASEto 1, fix description indentation), but the diff also removesPKG_VERSION:=1, removes+ucode-mod-fsfromDEPENDS, changesPKG_LICENSEfromGPL-2.0-or-latertoGPL-2.0-only, and changesPKG_MAINTAINERfromMin Choitominicom365. The dependency removal in particular reverts a fix from the preceding commit and is not an "audit issue". Please either drop the undocumented hunks or describe them — and note the license change from-or-laterto-onlyis a licensing decision, not formatting.
Generated by Claude Code
| SECTION:=utils | ||
| CATEGORY:=Utilities | ||
| TITLE:=Smart reboot based on network idle state | ||
| DEPENDS:=+busybox +uci +ucode +ucode-mod-uci +ucode-mod-log |
There was a problem hiding this comment.
This drops +ucode-mod-fs, but the check script still uses the fs module — import * as fs from "fs" at line 6, plus fs.popen (L15), fs.glob/fs.basename (L50-51), fs.readfile (L75-76) and fs.open (L103). fs is a separately packaged loadable module, not built into the interpreter (ucode-mod-fs in openwrt.git), so on a minimal install the script aborts at the import and the cron job never runs. This dependency was added in the previous commit; removing it here looks unintentional.
| DEPENDS:=+busybox +uci +ucode +ucode-mod-uci +ucode-mod-log | |
| DEPENDS:=+busybox +uci +ucode +ucode-mod-uci +ucode-mod-log +ucode-mod-fs |
Generated by Claude Code
| include $(TOPDIR)/rules.mk | ||
|
|
||
| PKG_NAME:=smart-reboot | ||
| PKG_RELEASE:=1 |
There was a problem hiding this comment.
PKG_VERSION:=1 was dropped here, which the commit message does not mention. Without it the package version is just $(PKG_RELEASE) rather than <version>-r<release> (see include/package-defaults.mk:24-31), so 1-r4 becomes plain 1 and every later PKG_RELEASE bump moves the version number instead of the release number. Nothing has been released yet so there is no downgrade risk, but for a files-only package the usual shape is to keep a PKG_VERSION and bump PKG_RELEASE on top of it. Was the removal intentional?
| PKG_RELEASE:=1 | |
| PKG_VERSION:=1 | |
| PKG_RELEASE:=1 |
Generated by Claude Code
|
|
||
| PKG_MAINTAINER:=minicom365 <3387910@naver.com> | ||
| PKG_LICENSE:=GPL-2.0-only | ||
| PKG_LICENSE_FILES:=LICENSE |
There was a problem hiding this comment.
nit: the formality check runs per commit, not on the merged result, so adding PKG_LICENSE_FILES in a follow-up commit cannot clear the error it reports against b0b5093 — that commit still introduces a Makefile without the parameter, and the bot comment on this PR still lists it. Since this is a single new package, folding this commit into b0b5093 (git commit --amend / git rebase -i + force-push) both silences the check and avoids shipping an introduce-then-fix pair for a package that does not exist upstream yet.
Generated by Claude Code
| GNU GENERAL PUBLIC LICENSE | ||
| Version 2, June 1991 | ||
|
|
||
| Copyright (C) 1989, 1991 Free Software Foundation, Inc. | ||
| 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
|
|
||
| Everyone is permitted to copy and distribute verbatim copies | ||
| of this license document, but changing it is not allowed. |
There was a problem hiding this comment.
This is only the cover page of the GPL-2.0 document (title, FSF copyright, verbatim-copying notice) — none of the actual terms are present, so the file does not grant anyone the license PKG_LICENSE claims. It also still carries the pre-2005 "59 Temple Place" FSF address. Either drop in the full GPL-2.0-only text, or leave the file out entirely: PKG_LICENSE_FILES names a path inside the extracted source tree and is only recorded as metadata, so file-only packages in this feed reference a name without shipping the file (e.g. net/acme-common/Makefile:15).
Generated by Claude Code
| PKG_NAME:=smart-reboot | ||
| PKG_RELEASE:=1 | ||
|
|
||
| PKG_MAINTAINER:=minicom365 <3387910@naver.com> |
There was a problem hiding this comment.
nit: PKG_MAINTAINER is expected to be a real name plus e-mail (Name <email>); minicom365 is the GitHub handle. The previous commit had Min Choi <3387910@naver.com> here, so this commit changes the maintainer identity without saying so, and the same name now appears in three different forms across the series (commit author 최민, Signed-off-by: minicom365, PKG_MAINTAINER:=minicom365). Picking one full name and using it consistently for author, sign-off and PKG_MAINTAINER would also settle the author-name error the formality bot reports.
| PKG_MAINTAINER:=minicom365 <3387910@naver.com> | |
| PKG_MAINTAINER:=Min Choi <3387910@naver.com> |
Generated by Claude Code
a7541a4 to
e3e3f29
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
The branch was squashed and rewritten again since the last automated pass (3cc8c72 is no longer reachable), so this is a fresh review of the current single commit. The commit message matches its diff, and all three FormalityCheck runs are green on this head.
The one finding I would call merge-blocking is the missing USE_PROCD=1 in the init script — without it start_service/stop_service/reload_service are never invoked, so the cron entry is never written and the installed package does nothing. Two further correctness notes concern the idle decision failing open (negative delta, unreadable interface), and two are optional packaging cleanups.
The two earlier comments that are still unaddressed and still apply are not repeated here: shelling out to date via fs.popen in set_last_auto_reboot(), and the enabled '1' default that arms an unattended nightly reboot on install.
Generated by Claude Code
| START=95 | ||
| STOP=10 |
There was a problem hiding this comment.
USE_PROCD=1 is missing, so none of the *_service functions in this file is ever called and the package does nothing once installed.
/etc/rc.common defines start()/stop()/reload() in terms of start_service/stop_service/reload_service only inside the [ -n "$USE_PROCD" ] block (rc.common:121-172). Without it the defaults at rc.common:11-21 apply — start() and stop() are bare return 0. Consequences: the restart in Package/smart-reboot/postinst is a no-op, the S95 boot symlink runs nothing, the cron line is never appended to /etc/crontabs/root, and procd_add_reload_trigger in service_triggers is not even defined (procd.sh is sourced under the same guard).
| START=95 | |
| STOP=10 | |
| START=95 | |
| STOP=10 | |
| USE_PROCD=1 |
Generated by Claude Code
| SECTION:=utils | ||
| CATEGORY:=Utilities | ||
| TITLE:=Smart reboot based on network idle state | ||
| DEPENDS:=+busybox +uci +ucode +ucode-mod-uci +ucode-mod-log +ucode-mod-fs |
There was a problem hiding this comment.
Two packaging points on this block:
- The package ships only config, a shell init script and a ucode script — nothing architecture-dependent — so it should be marked
PKGARCH:=all. The closest sibling in this feed,utils/watchcat/Makefile:23(also a files-only scheduled-reboot utility), does exactly that. Without it the buildbots produce and store one identical copy per architecture. +busyboxhas no precedent here: it is the only occurrence of a busybox dependency across the whole feed. busybox is inDEFAULT_PACKAGESand always installed, so the entry adds nothing.
| DEPENDS:=+busybox +uci +ucode +ucode-mod-uci +ucode-mod-log +ucode-mod-fs | |
| DEPENDS:=+uci +ucode +ucode-mod-uci +ucode-mod-log +ucode-mod-fs | |
| PKGARCH:=all |
Generated by Claude Code
| define Package/smart-reboot/postinst | ||
| #!/bin/sh | ||
| [ -n "$$IPKG_INSTROOT" ] || { | ||
| /etc/init.d/smart-reboot enable >/dev/null 2>&1 | ||
| /etc/init.d/smart-reboot restart >/dev/null 2>&1 | ||
| } | ||
| exit 0 | ||
| endef |
There was a problem hiding this comment.
nit: this scriptlet is redundant — the package system already enables and starts init scripts. The generated postinst is just default_postinst "$0" "$@" (package-pack.mk:519-520), and default_postinst sources this block (installed as postinst-pkg) and then runs "$i" enable / "$i" start for every /etc/init.d/* entry in the package file list — functions.sh:401-410. The IPKG_INSTROOT guard is handled there too. Dropping the whole block yields the same behaviour with less to maintain (optional).
Generated by Claude Code
| let end_bytes = sum_iface_bytes(ifaces); | ||
| let delta = end_bytes - start_bytes; | ||
|
|
||
| if (delta <= byte_threshold) { |
There was a problem hiding this comment.
delta can be negative, and a negative value satisfies delta <= byte_threshold, i.e. it is read as "idle" and triggers the reboot. That happens exactly when the sample is untrustworthy: an interface removed and re-added during the sampling window resets its counters, and on 32-bit kernels rx_bytes/tx_bytes wrap at 4 GiB — reachable in a 120 s window at gigabit rates, so a heavily loaded link can wrap and be classified as idle. Treating a negative sample as "not idle" fails safe:
| if (delta <= byte_threshold) { | |
| if (delta >= 0 && delta <= byte_threshold) { |
Generated by Claude Code
| let rx = trim(fs.readfile(`/sys/class/net/${iface}/statistics/rx_bytes`) ?? "0"); | ||
| let tx = trim(fs.readfile(`/sys/class/net/${iface}/statistics/tx_bytes`) ?? "0"); | ||
| total += int(rx || "0") + int(tx || "0"); |
There was a problem hiding this comment.
A name that has no /sys/class/net/<name> entry fails silently in the unsafe direction: fs.readfile returns null, ?? "0" turns it into 0 for both samples, delta is 0, and the router reboots unconditionally every night as if the link were idle.
This is not hypothetical, because the entries here are kernel netdev names while the shipped default list ifaces 'wan' is the name of a UCI network. The two coincide on DSA targets (such as the ipq806x device you tested on) but not on targets where the WAN netdev is eth0, eth0.2, pppoe-wan, and so on — there the default config silently degrades into "reboot every night, no matter the traffic".
Consider logging and treating an unreadable interface as non-idle (skip the reboot) rather than as zero bytes, so a typo or a logical-vs-kernel name mix-up cannot arm an unconditional reboot.
Generated by Claude Code
e3e3f29 to
0a012ed
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
The branch was squashed and rewritten again since the last automated pass (e3e3f29 is no longer reachable), so this is a fresh review of the current single commit. The commit message matches its diff, and all three FormalityCheck runs are green on this head.
Most of the previous round is addressed: USE_PROCD=1 is in place, PKG_VERSION/PKG_MAINTAINER/PKG_LICENSE/PKGARCH:=all are set, +ucode-mod-fs is back and +busybox is gone, the redundant postinst block is dropped, enabled now defaults to 0, a negative delta no longer counts as idle, and an unreadable interface aborts the check instead of reading as zero bytes.
One correctness gap remains, and it is the same shape as the ones already fixed: sample_seconds is used unvalidated, so a 0 or non-numeric value collapses the sampling window and arms an unconditional reboot. That is the only finding I would hold the merge for. The other four are cosmetic or a question about the shipped ifaces 'wan' default.
Generated by Claude Code
| let hour = parsed ? parsed.hour : int(cfg_get("hour", "4")); | ||
| let minute = parsed ? parsed.minute : int(cfg_get("minute", "0")); | ||
|
|
||
| let sample_seconds = int(cfg_get("sample_seconds", "120")); |
There was a problem hiding this comment.
sample_seconds is taken straight from UCI and never validated, and the failure direction is "reboot". With sample_seconds set to 0 — or to anything non-numeric, which int() turns into NaN — the sleep(sample_seconds * 1000) at line 159 does not wait, so start_bytes and end_bytes are read back to back, delta is 0, and the delta >= 0 && delta <= byte_threshold test at line 169 passes unconditionally — the router reboots no matter how busy the link is.
This is the one tunable a user is most likely to lower ("make the check quicker"), and a stray non-numeric value has the same effect. Clamping it back to the default keeps a bad value from arming an unconditional reboot:
| let sample_seconds = int(cfg_get("sample_seconds", "120")); | |
| let sample_seconds = int(cfg_get("sample_seconds", "120")); | |
| if (!(sample_seconds > 0)) | |
| sample_seconds = 120; |
byte_threshold fails in the safe direction (delta <= NaN is false, so no reboot), so it needs nothing.
Generated by Claude Code
|
|
||
|
|
||
| function cmd_out(cmd) { | ||
| let f = fs.popen(cmd, "r"); | ||
| if (!f) | ||
| return ""; | ||
|
|
||
| let out = f.read("all") ?? ""; | ||
| f.close(); | ||
| return trim(out); | ||
| } |
There was a problem hiding this comment.
nit: cmd_out() is dead code — it is the only fs.popen user left, and nothing calls it now that set_last_auto_reboot() formats the timestamp from localtime(). Dropping the function (and the double blank line above it) removes the last trace of shelling out from the script.
Generated by Claude Code
| let ts = sprintf("%04d-%02d-%02d %02d:%02d:%02d", | ||
| now.year, now.mon, now.mday, now.hour, now.min, now.sec); |
There was a problem hiding this comment.
nit: trailing whitespace after the format string (the only occurrence in the PR).
| let ts = sprintf("%04d-%02d-%02d %02d:%02d:%02d", | |
| now.year, now.mon, now.mday, now.hour, now.min, now.sec); | |
| let ts = sprintf("%04d-%02d-%02d %02d:%02d:%02d", | |
| now.year, now.mon, now.mday, now.hour, now.min, now.sec); |
Generated by Claude Code
| option time '04:00' | ||
| option all_ifaces '0' | ||
| option last_auto_reboot '' | ||
| list ifaces 'wan' |
There was a problem hiding this comment.
nit: ifaces entries are used as kernel netdev names — sum_iface_bytes() reads /sys/class/net/${iface}/statistics/rx_bytes — but wan is the name of a UCI network, and the two only coincide on DSA targets (such as the ipq806x device in the PR description). On targets where the WAN netdev is eth0.2, pppoe-wan, wwan0, etc. there is no /sys/class/net/wan, so the shipped default hits the "missing or unreadable" branch and the package silently never reboots.
The direction is safe now that an unreadable interface aborts the check, so this is not a correctness problem — but a default that does nothing on most targets is easy to mistake for a broken package. Would resolving the configured name through network_get_device / ubus network.interface (falling back to treating it as a netdev name) be worth it, or is documenting "kernel interface name, not UCI network name" enough?
Generated by Claude Code
| CRON_FILE="/etc/crontabs/root" | ||
| MARKER="# smart-reboot" | ||
|
|
||
| . /lib/functions.sh |
There was a problem hiding this comment.
nit: redundant — /etc/rc.common already sources it before the init script is read (. $IPKG_INSTROOT/lib/functions.sh at rc.common:4), which is why config_load/config_get are available in gen_schedule(). None of the 32 init scripts in this feed source it explicitly. The unqualified path also drops the $IPKG_INSTROOT prefix rc.common uses, so the line can just go.
Generated by Claude Code
0a012ed to
93093f6
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
The branch was squashed and rewritten again since the last automated pass (0a012ed is no longer reachable), so this is a fresh review of the current single commit. The commit message matches its diff, and all three FormalityCheck runs are green on this head.
The previous round is addressed: sample_seconds is now clamped, cmd_out() is gone, the trailing whitespace is fixed, the redundant functions.sh source is dropped, and the wan default now carries an explanatory note.
Nothing here is merge-blocking. The one substantive finding is that time is parsed by two different parsers — the case glob in the init script and parse_time() in the check script — which disagree on non-zero-padded values, and the disagreement makes the package silently never reboot rather than fail loudly. The other three are cosmetic or packaging cleanups.
Generated by Claude Code
| config_get time settings time '' | ||
|
|
||
| case "$time" in | ||
| [0-1][0-9]:[0-5][0-9]|2[0-3]:[0-5][0-9]) |
There was a problem hiding this comment.
time is parsed twice, by two parsers that accept different grammars, and the disagreement silently disables the feature.
This pattern only matches a zero-padded HH:MM, while parse_time() in the check script splits on : and accepts any H:MM with 0 <= hour <= 23. Take option time '9:30': here the case falls through to *), which reads settings.hour/settings.minute — options that do not exist in the shipped config — so cron is written as 0 4 * * *. The check script meanwhile parses 9:30 and bails out at the now.hour != hour || now.min != minute guard (L140). Net effect: cron fires at 04:00, the script decides it is not the configured time, and the router never reboots — with nothing logged to explain why.
Accepting a single-digit hour here makes the two agree (the alternative is to tighten parse_time() to the same zero-padded form and log a rejection):
| [0-1][0-9]:[0-5][0-9]|2[0-3]:[0-5][0-9]) | |
| [0-9]:[0-5][0-9]|[0-1][0-9]:[0-5][0-9]|2[0-3]:[0-5][0-9]) |
Generated by Claude Code
| # Note: Use kernel netdev names (e.g. eth0, pppoe-wan), not UCI network names like 'wan' | ||
| list ifaces 'wan' |
There was a problem hiding this comment.
nit: two problems with documenting this as a UCI comment.
First, the note names wan as precisely the kind of value not to use, and the very next line ships list ifaces 'wan' as the default — a reader who follows the note has to conclude the shipped default is wrong. (Same wan default as the still-open thread on this line; the note answers it for targets where the two differ, but leaves the default contradicting its own advice.)
Second, the comment does not survive the package's own operation. libuci's parser drops comment lines, and commit rewrites the file from the parsed tree — so the first time set_last_auto_reboot() calls cursor.commit(CFG) (L32-L38) after a successful reboot, the # Note: line is gone, along with any comments a user added. Guidance that only lives in a file the package rewrites disappears exactly once the package has been used. Package/smart-reboot/description, a README, or the LuCI frontend would hold it durably.
Generated by Claude Code
| SECTION:=utils | ||
| CATEGORY:=Utilities | ||
| TITLE:=Smart reboot based on network idle state | ||
| DEPENDS:=+uci +ucode +ucode-mod-uci +ucode-mod-log +ucode-mod-fs |
There was a problem hiding this comment.
nit: +uci is no longer used by anything this package ships. It pulls in the /sbin/uci CLI, but the check script now goes through the ucode uci module and the init script uses config_load/config_get from /lib/functions.sh, which parse /etc/config directly — neither execs uci. The libuci that the module actually needs already comes in with the module: UcodeModule(uci, UCI_SUPPORT, +libuci, ...) in openwrt.git. Dropping it leaves the installed runtime set unchanged:
| DEPENDS:=+uci +ucode +ucode-mod-uci +ucode-mod-log +ucode-mod-fs | |
| DEPENDS:=+ucode +ucode-mod-uci +ucode-mod-log +ucode-mod-fs |
Generated by Claude Code
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
nit (cosmetic, non-blocking): three blank lines before $(eval ...). Every other package in the feed separates the last endef from the BuildPackage call with a single blank line — cf. utils/watchcat/Makefile:46-48.
Generated by Claude Code
de52e67 to
1df1c9d
Compare
Add smart-reboot, a lightweight OpenWrt utility that performs scheduled reboot during a dawn time window when network traffic is idle. - UCI-based configuration (/etc/config/smart-reboot) - Procd-managed uloop background daemon - Dawn time window with periodic retry on active traffic - Atomic /proc/net/dev telemetry and dynamic ubus L3 device resolution - Failsafe guards (NTP sync, system uptime, and single daily reboot) Signed-off-by: Min Choi <3387910@naver.com>
1df1c9d to
b4894e1
Compare
4 participants
This adds
smart-reboot, a lightweight OpenWrt utility that performs scheduled reboot only when monitored interfaces are considered idle.Features
/etc/config/smart-reboot)HH:MMTesting
ipq806x/genericarm_cortex-a15_neon-vfpv4Notes
Signed-off-by: minicom365 3387910@naver.com