nut: fix hotplug access and other bugs

[danielfdickinson]

📦 Package Details

Maintainer: @danielfdickinson

Description:

On boot, hotplug events were causing excessive start and stop action
for the upsd daemon and driver daemons. We fix that with two primary
actions:

1. Don't restart service daemons on hotplug until after first boot
   has completed.
2. Use more robust handling of procd instance starts by ensuring
   that the first start starts the nut-server service and all
   others add to the nut-server service (rather than replacing it).

In addition clean up some logging.

In the process, this fixes #30375 "hotplugging for setting usb access
right[s] doesn't work anymore"

Closes: #30375

We also fix configuration of custom notification messages

Short-circuit not applicable portions of hotplug script on remove, and
make sure we ignore events with incomplete information (no DEVNAME
or no ACTION).

The match against known device was badly formatted, and was
not being used as a result. Fix that.

Finally, a previous PR missed updating the hotplug scripts for the
new find_runas and find_statepath functions which emit
the value on stdout instead of setting a variable in the
caller's cope. Update that usage.

---

🧪 Run Testing Details

Server and Self-client (with and without SSL)

• OpenWrt Version: OpenWrt SNAPSHOT r35906-3d1645ee26
• OpenWrt Target/Subtarget: bcm27xx/bcm2709
• OpenWrt Device: Raspberry Pi 2 Model B Rev 1.1
• UPS: Tripp-Lite ECO 550 UPS

Client (with and without SSL)

• OpenWrt Version: OpenWrt SNAPSHOT r35921-f0204d78e1
• OpenWrt Target/Subtarget: mediatek/filogic
• OpenWrt Device: OpenWrt One

---

✅ Formalities

• I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

[danielfdickinson]

Missed some of the needed diff. Will update when I am back from work tonight.

[openwrt-ai]

Reviewed 5 new commits.

Two findings look like they block a merge:

• net/nut/files/nut-server.init:577 calls loop_through_drivers, which is not defined in the package or the base system — boot() fails that line with "not found" every boot.
• The new rc_procd_service nests a second procd_open_service/procd_close_service pair inside the one rc.common's rc_procd already opens around start_service, so the trailing outer procd_close_service set submits the inner (script-less, single-instance) JSON. That is the opposite of the "add rather than replace" behaviour commit f8a50b8 is aiming for.

The duplicated log_msg in nut-serial.hotplug and the set -f leak in has_running_driver are smaller but concrete. The rest are questions or optional cleanups.

Commit checks

• ff5b9a5 "nut: fix setting RUNAS and STATEPATH in hotplug scrips" — the change itself matches the message, but the subject has a typo: scrips → scripts (and the body has caller's cope → caller's scope).

---

Generated by Claude Code

[openwrt-ai]

Reviewed the 5 rebased commits (ff5b9a5 → 1273b52).

Most of the previous round is addressed: loop_through_drivers is gone, has_running_driver no longer leaks set -f, the boot-complete sentinel is now created from start_service so a fresh install isn't stuck without hotplug until a reboot, the duplicate log_msg in nut-serial.hotplug and the duplicate start_server_instance in reload_ups_driver are removed, the nd_driver_config_error check now runs before the remove short-circuit, and both hotplug sleeps are dropped.

One finding looks like it blocks a merge:

• The new start() at nut-server.init:20 is dead code. rc.common sources the init script first and then, because USE_PROCD=1, installs its own start() { rc_procd start_service "$@"; } over it. start/restart therefore still nest a second procd_open_service/procd_close_service inside rc_procd's — which is the behaviour commit e199278 is trying to remove — and only boot() gets the intended single-open path. Details and two possible fixes inline.

The remaining set -f leak in stop_no_longer_configured_instances is smaller but concrete, and this PR makes it reachable on every cold start. The other two comments are optional cleanups.

Commit checks

• e199278 "nut: prevent service thrashing on boot or hotplug" — the body says "Also introduce some a 'sleep' in hotplug to reduce bouncing", but after the rebase this commit adds no sleep to either hotplug script; the only sleep it introduces is the sleep 1 in boot() in nut-server.init. (git grep sleep net/nut/files/*.hotplug at head returns nothing.) The same stale sentence is in the PR body. There is also a stray "some a" in that line.

---

Generated by Claude Code

[danielfdickinson]

Re-testing will be required

[danielfdickinson]

Re-tested On

Including boot, restart, reload, and hotplug (adding and removing).

Server and Self-client (with and without SSL enabled)

• OpenWrt Version: OpenWrt SNAPSHOT r35906-3d1645ee26
• OpenWrt Target/Subtarget: bcm27xx/bcm2709
• OpenWrt Device: Raspberry Pi 2 Model B Rev 1.1
• UPS: Tripp-Lite ECO 550 UPS
• UPS 2: Tripp-Lite AVR550U

Client (with and without SSL enabled)

• OpenWrt Version: OpenWrt SNAPSHOT r35921-f0204d78e
• OpenWrt Target/Subtarget: mediatek/filogic
• OpenWrt Device: OpenWrt One

[openwrt-ai]

Reviewed the rebased series plus the two new commits (1273b52 → da30610).

The previous round's main blocker is resolved: the script now drops USE_PROCD and defines start/stop/reload/status/rc_procd itself, so the override actually takes effect and there is no nested procd_open_service on start/restart any more. The remaining set -f leaks in stop_no_longer_configured_instances and stop_all_instances are fixed too, and the tr -d '\"' → tr -d '"' change is correct for BusyBox tr. CI is green on da30610, so nothing to report there.

Two findings on the new state that look worth resolving before merge:

• boot() is the only path that reaches rc_procd without procd_lock, and it does so after start_service has already enabled hotplug — which is the exact race the new comment in libhid-ups.hotplug says is covered.
• Dropping USE_PROCD also drops the unconditional procd_close_service, so service_triggers() (and the nut_server reload trigger) is no longer registered on a start with no configured/startable instances.

One more in nut-serial.hotplug: the new return 0 error paths feed did_set_perms, so a failed chgrp/chmod now reports success and triggers a reload; and config_foreach ignores the return status anyway, so the previous return 1 was not aborting anything. Details inline. The remaining three comments are typos/quoting.

Commit checks

• da30610 "nut: improve documentation and logging" — body says "the nut-serve initscript" (nut-serve → nut-server). The diff is otherwise comments/logging as described, except one non-documentation hunk in rc_procd (procd_running "nut-server" → procd_running "$(basename "$service_name")") that the subject doesn't cover — behaviourally equivalent here, but not a doc change.

---

Generated by Claude Code

[openwrt-ai]

Reviewed the rebased series (da30610 → 34827a5).

Both findings from last round are addressed: boot() now takes procd_lock, and rc_procd : in boot()/start() makes the service and its service_triggers() register even when no instance starts. In nut-serial.hotplug the error paths in nut_set_serial_port_permissions are back to return 1, so did_set_perms is no longer set on a failed chgrp/chmod. The ${IPKG_INSTROOT} quoting, local to the, SUB → USB and the nut-serve typo in the commit message are all fixed. Full CI matrix is green on 34827a5, so nothing to report there.

One finding on the new state looks worth resolving before merge:

• rc_procd : submits an empty instances object, and rc_procd selects set (not add) whenever nothing is currently running. A set replaces the service definition, so on the start "$ups" path the hotplug script uses, drivers that are registered but not yet running are dropped and never re-registered by that invocation. Details and two possible fixes inline.

The other two comments are a correction to my previous round (the added procd_lock calls turn out to be redundant — procd.sh already takes the lock when sourced) and a stale sentence in the PR body.

All six commit messages match their diffs this time, including the squashed serial-hotplug error handling now described in bacf0f1.

---

Generated by Claude Code

[openwrt-ai]

Reviewed the rebased series (34827a5 → 3badb58). The only tree change since last round is in nut-server.init: the set/add selection in rc_procd gained && ! service_active_no_instances, and the sleep 1 in boot() is gone (the stale "introduce some a 'sleep' in hotplug" sentence is out of the PR body too). Full CI matrix is green on 3badb58, so nothing to report there.

One finding on the new guard:

• service_active_no_instances is called without its service-name argument, so it fails check_safe_uci_name "" and always returns 1. The added condition is therefore always true and rc_procd still picks set/add exactly as it did before this change — the case I raised last round is not actually closed. Details, a one-line fix, and the remaining "registered but not yet running" case inline.

The other comment is a typo on the reworded comment line above it.

All six commit messages match their diffs; 3badb58 "nut: improve documentation and logging" is comments and one log_msg only.

---

Generated by Claude Code

[openwrt-ai]

Reviewed the rebased series (3badb58 → 000c3d4). The only tree change since last round is the rc_procd / service_active_no_instances pair: the function now returns 2 for "active, with instances", and rc_procd selects set only on a literal 1. Full CI matrix is green on 000c3d4, so nothing to report there.

This closes both cases from last round. The service name is now passed, so the guard is no longer always-true, and "instances registered but none running yet" returns 2 → add instead of set, so procd_running's asynchronous view no longer drives a destructive replace — procd_running is out of the rc_procd decision entirely. The other two call sites (nut-server.init:458, nut-monitor.init:134, plus :477) all use if service_active_no_instances …, so the 1 → 2 change is transparent to them.

One follow-on on the new tri-state contract:

• 1 is returned for three different outcomes — "service not registered" plus two error paths (check_safe_uci_name failure, unexpected jsonfilter output) — and rc_procd maps all three to the destructive set. Since rc_procd runs once per driver during a start, an error part-way through the loop would drop the instances registered by the earlier iterations. Details and a suggested split inline.

The other comment is a stale sentence (and a procd_close_instance / procd_close_service slip) in the file's header comment, which this change makes inaccurate.

Commit checks

• 000c3d4 "nut: improve documentation and logging" — the diff is comments only now (29 insertions, 1 deletion, every one of them a # line). The log_msg "… was not active, so start it" that justified the logging half moved into 2457dfc0 in this rebase, so the subject's "and logging" and the body's "In addition, add additional logging." no longer match anything in this commit.

---

Generated by Claude Code

[openwrt-ai]

Reviewed the rebased series (000c3d4 → 2ff4fdf). The only tree change since last round is the service_active_no_instances error split: check_safe_uci_name failure and unexpected jsonfilter output now return 3 instead of 1, both with a log_error, and the header comment in nut-server.init was rewritten and folded into 96bd093c. Full CI matrix is green on 2ff4fdf, so nothing to report there.

That closes last round's finding. rc_procd picks set only on a literal 1, so an error part-way through the per-driver loop now falls through to add and no longer drops the instances registered by earlier iterations. The three other call sites (nut-server.init:458, :477, nut-monitor.init:134) are all if service_active_no_instances …, so 3 behaves exactly as the old 1 did for them, and the log_error argument order matches the existing call in the same function. The stale procd_close_instance wording in the file header comment is fixed too.

No blocking findings this round. The two comments below are both optional comment-text nits on the lines this change touched.

Commit checks

All six commit messages match their diffs. 2ff4fdf "nut: improve documentation" is now comments plus one blank line only, and the "and logging" half of the old subject/body correctly moved to 96bd093c, which does carry the log_error additions.

---

Generated by Claude Code