Fix 3 real bugs found while writing new vnet test coverage#10
Merged
Conversation
All three surfaced while adding vnet/tests/16-known_devices.test.sh and vnet/tests/17-tor.test.sh (gargoyle-tools), which gave the Device Groups feature (PR #9) and gargoyle-tor their first vnet test coverage. 1. manage_groups.sh: no explicit exit code. The script's exit status was whatever its last internal "[ -n "$ip" ]" check happened to evaluate to - a caller checking $? saw a spurious failure whenever the last device processed simply had no active DHCP lease yet, even though the script did all its actual work correctly. Added an explicit `exit 0`. 2. manage_groups.sh: orphan-set cleanup used invalid nftables syntax. "nft list sets $family $table" is a syntax error (list sets takes no family/table arguments); this failed silently every time (stderr redirected to /dev/null), so the loop that's supposed to delete nftables sets for groups no longer defined in UCI never found anything to delete - stale grp_* sets accumulated forever instead of being cleaned up. Fixed to "nft list table $family $table", scoped to the actual table, which correctly enumerates its sets. 3. restart_firewall.sh: same missing-exit-code bug as #1. Its last line is "service_enabled webmon_gargoyle && /etc/init.d/webmon_gargoyle start" with no explicit exit afterward, so its exit status was whatever that conditional evaluated to - non-zero whenever that particular optional service isn't enabled (the common case), even though the firewall restart itself succeeded. This script is called directly by the Restrictions/DHCP/Quotas pages' save flow, so any caller that checks its exit code was getting a false failure signal on every save. Added an explicit `exit 0`. 4. tor.firewall: references $tmp_cron (needed by the shared update_cron() helper every other bwmon-integrated feature in this codebase - OpenVPN, QoS, WireGuard, bwmon-gargoyle itself - already defines locally before using) but never defines it itself. Every firewall reload with Tor enabled threw "can't create : nonexistent directory" / "can't open ''" errors while trying to update the cron entry for the Tor relay-set refresh job. Added `tmp_cron="/tmp/tmp.tor.cron"`, matching the naming convention every other caller uses. Verified individually via direct live testing on a booted vnet VM: each fix was confirmed to change a script's exit code from non-zero to 0 (or, for the nftables fix, confirmed the orphaned set actually disappears after reconciliation) before and after, not just inspected by reading the diff.
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
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.
Summary
All three surfaced while writing two new vnet test phases (
vnet/tests/16-known_devices.test.shandvnet/tests/17-tor.test.sh, in gargoyle-tools), which gave the Device Groups feature (#9) andgargoyle-tortheir first vnet test coverage.manage_groups.sh: no explicit exit code. The script's exit status was whatever its last internal[ -n "$ip" ]check happened to evaluate to — a caller checking$?saw a spurious failure whenever the last device processed simply had no active DHCP lease yet, even though the script did all its actual work correctly. Added an explicitexit 0.manage_groups.sh: orphan-set cleanup used invalid nftables syntax.nft list sets $family $tableis a syntax error (list setstakes no family/table arguments); this failed silently every time (stderr redirected to/dev/null), so the loop that's supposed to delete nftables sets for groups no longer defined in UCI never found anything to delete — stalegrp_*sets accumulated forever instead of being cleaned up. Fixed tonft list table $family $table, scoped to the actual table, which correctly enumerates its sets.restart_firewall.sh: same missing-exit-code bug as Fix WireGuard config file import to handle flexible formatting #1. Its last line isservice_enabled webmon_gargoyle && /etc/init.d/webmon_gargoyle startwith no explicit exit afterward, so its exit status was non-zero whenever that particular optional service isn't enabled (the common case), even though the firewall restart itself succeeded. This script is called directly by the Restrictions/DHCP/Quotas pages' save flow, so any caller checking its exit code was getting a false failure signal on every save.tor.firewall: references$tmp_cron(needed by the sharedupdate_cron()helper — every other bwmon-integrated feature in this codebase, OpenVPN/QoS/WireGuard/bwmon-gargoyle itself, already defines it locally before using) but never defines it itself. Every firewall reload with Tor enabled threwcan't create : nonexistent directory/can't open ''errors while trying to update the cron entry for the Tor relay-set refresh job. Addedtmp_cron="/tmp/tmp.tor.cron", matching the naming convention every other caller uses.Verification
Each fix was confirmed individually via direct live testing on a booted vnet VM — checked a script's exit code changed from non-zero to
0(or, for the nftables fix, confirmed the orphaned set actually disappears after reconciliation) before and after the fix, not just inspected by reading the diff.The two new vnet test phases that surfaced these bugs are in a separate gargoyle-tools commit —
16-known_devices.test.shpassed 10/10 in a full automatedbuild-and-test.shcycle with these fixes applied.17-tor.test.sh's 12 checks were each verified individually via manual live testing; a full automated single-script run of that phase could not be completed this session due to unrelated vnet network degradation after many hours of continuous heavy activity (not a code issue — noted in that commit).Test plan
manage_groups.shexit code confirmed 0 after fix (was 1)manage_groups.shorphan set confirmed deleted after fix (was never deleted before)restart_firewall.shexit code confirmed 0 after fix (was 1)tor.firewallconfirmed error-free after fix (was throwing errors every reload)build-and-test.shwith these fixes: 124/139 (matches baseline + new16-known_devicesphase passing 10/10)