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).

Also introduce some a 'sleep' in hotplug to reduce bouncing, and
ignore hotplug events without a DEVNAME.

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]

boot() is now the only entry point that reaches rc_procd without holding procd_lock.

start() (line 668), stop() (line 688) and reload() (line 698) all take it, but boot() calls start_service directly. start_service creates $NUT_HOTPLUG_BOOT_COMPLETE_PATH at line 596 before it returns, so from that point on hotplug is permitted — and boot() then goes on to run config_foreach start_ups_driver driver and start_server_instance, both of which open/close the nut-server service via rc_procd, while a hotplug-triggered /etc/init.d/nut-server start "$ups" can be doing the same thing in a second process.

That is exactly the window the comment added to libhid-ups.hotplug:79-81 claims is covered ("the driver start goes through the initscript's start which has a procd_lock … even if hotplug fires as soon as hotplugging is allowed in initscript's boot()") — the hotplug side takes the lock, but the boot() side never does, so the two procd_open_service/procd_close_service sequences can still interleave and a set from one can drop instances registered by the other.

procd_lock is a flock on fd 1000 held for the life of the process (procd.sh:48-60), so taking it at the top of boot() is safe and re-entrant.

Suggested change

	boot() {
	# start_service will call manage_service start which will start the drivers
	# and upsd instances, following which hotplug will be allowed
	start_service || return 1
	boot() {
	# Ensure start actions occur one at a time, as start_service enables
	# hotplug before this function has finished starting instances
	procd_lock
	# start_service will call manage_service start which will start the drivers
	# and upsd instances, following which hotplug will be allowed
	start_service || return 1

---

Generated by Claude Code

[openwrt-ai]

Dropping USE_PROCD and calling start_service directly means procd_open_service/procd_close_service — and therefore service_triggers() — is no longer guaranteed to run on a start.

On master today, start is rc_procd start_service "$@", so procd_close_service fires unconditionally and _procd_close_service runs service_triggers even when zero instances were opened. After this change the only routes to rc_procd are start_server_instance and start_ups_driver, and both are conditional:

• config_foreach start_ups_driver driver is a no-op when no driver section is configured;
• start_ups_driver returns at line 190/197 when ensure_usb_ups_access fails or driver is empty;
• start_server_instance only calls rc_procd when have_driver_instance = true && have_upsd_instance = false.

So with nut-server enabled but not yet configured (fresh install — the same scenario that motivated moving the hotplug sentinel into start_service), /etc/init.d/nut-server start registers nothing with procd: no nut-server service object, and procd_add_reload_trigger "nut_server" plus the interface triggers from service_triggers() are never installed. A later uci commit nut_server then has no trigger to fire, where on master it would.

Would it be worth having start() (and boot()) call rc_procd once around start_service — or at minimum register the service/triggers when no instance was opened — so the trigger registration stays unconditional?

---

Generated by Claude Code

[openwrt-ai]

Turning these failures into return 0 makes the caller treat a failed device as a success.

nut_serial at line 109 uses this return value as the per-device success flag:

if nut_set_serial_port_permissions "$normal_devname" "$ups"; then
	did_set_perms="true"
fi

With return 0 on the error paths, a chgrp/chmod failure or a device that vanished still sets did_set_perms="true", so line 153 logs "Successfully set permissions for serial device(s)" and line 167 fires /etc/init.d/nut-server reload for a device whose permissions were never set.

The stated motivation also doesn't hold: nothing was aborting before. nut_set_serial_port_permissions is only reached from nut_serial, which is invoked by config_foreach — that loop evals the function and ignores its exit status, and this script does not run under set -e. A non-zero return from one device never stopped the others being processed.

Same applies to lines 60-61, 68-69 and 73-74 below.

Suggested change

	# Do not prevent other drivers from being configured
	return 0
	return 1

---

Generated by Claude Code

[openwrt-ai]

nit: unquoted ${IPKG_INSTROOT} — the other six . lines in this file (lines 41, 51, 58, 64, 70, 76, 82) all quote it. Copied from rc.common, but it now lives in a file whose own convention is to quote.

Suggested change

	. ${IPKG_INSTROOT}/lib/functions/procd.sh || {
	. "${IPKG_INSTROOT}"/lib/functions/procd.sh || {

---

Generated by Claude Code

[openwrt-ai]

nit: local the the → local to the.

Suggested change

	# they are local the the _caller_ and visible/modifiable in this
	# they are local to the _caller_ and visible/modifiable in this

---

Generated by Claude Code

[openwrt-ai]

nit: SUB → USB.

Suggested change

	# Group of RUNAS user; used to allow access to the SUB serial device
	# Group of RUNAS user; used to allow access to the USB serial device

---

Generated by Claude Code