0.6.2: firewalld --check in one parallel wave of whole listings (#29)

README.md

@@ -135,7 +135,7 @@ Upgrades then arrive with the rest of your system updates.
 ### Any distribution, static binary
 
 ```sh
-curl -fsSL https://github.com/tui-tools/tui-firewall/releases/download/v0.6.1/tui-firewall_0.6.1_linux_amd64.tar.gz | tar -xz tui-firewall
+curl -fsSL https://github.com/tui-tools/tui-firewall/releases/download/v0.6.2/tui-firewall_0.6.2_linux_amd64.tar.gz | tar -xz tui-firewall
 sudo install -m0755 tui-firewall /usr/local/bin/tui-firewall
 ```
 
@@ -263,6 +263,30 @@ distinguishable from "ufw is not installed" without running the detection
 again. The version is probed for the selected backend only: probing the other
 would run a binary nobody asked this tool to touch.
 
+What it costs is the backend's own reads, so it depends on the backend:
+
+| Backend | What `--check` runs | Typical time |
+|---|---|---|
+| ufw | `ufw status verbose`, `ufw status numbered`, `ufw app list` | well under half a second |
+| nftables | `nft -j list ruleset` | well under half a second |
+| iptables | `iptables-save -c` and `ip6tables-save -c` | well under half a second |
+| firewalld | eight `firewall-cmd` reads started together, plus `firewall-cmd --version` | about 1.7 s on a 2-vCPU Fedora 44 VM (firewalld 2.4.4), down from 8 s |
+
+firewalld is the slow one because every `firewall-cmd` is a Python
+interpreter that imports the firewalld client before it makes its D-Bus
+calls: 100 to 300 ms each, most of it start-up rather than firewalld
+answering. Since 0.6.2 the tool asks for whole listings and starts them
+together, so the cost is roughly that of the slowest one on an idle machine,
+where 0.6.1 paid for 10 reads plus 2 per policy object one after another
+(about 8 s on the lab's Fedora guest). With firewalld stopped every read
+waits about 10 s for the daemon on D-Bus before giving up, so `--check`
+takes that long to say it is not running.
+
+A tool that reads `--check` on every refresh, as tui-wireguard and
+tui-tailscale do, can keep the answer for a few seconds rather than asking
+again for each redraw: the firewall rarely changes between two reloads, and
+the tool that changes it knows when it did.
+
 [tui-lab](https://github.com/tui-tools/tui-lab) uses it to test this tool
 against real firewalls on Ubuntu, Fedora and Omarchy Server; the assertions live
 in [`test/smoke.sh`](test/smoke.sh).
@@ -724,10 +748,14 @@ are capped, so a firewall under a scan cannot grow it without bound.
 
 **firewalld**
 
-- Read `--state`, `--get-default-zone`, `--get-active-zones`,
-  `--list-all-zones` (runtime and permanent), `--get-services`,
-  `--get-log-denied`, `--query-panic`, `--query-lockdown` and the policy
-  objects from `--get-policies`.
+- Read `--list-all-zones` and `--list-all-policies` (runtime and permanent),
+  `--get-services`, `--get-log-denied`, `--query-panic` and `--query-lockdown`,
+  all eight started together. The default zone and the active zones come from
+  the `(default, active)` flags of the zone listing; a firewalld that prints no
+  flags, or has no `--list-all-policies`, is asked `--get-default-zone`,
+  `--get-active-zones` and `--get-policies` instead, and `--state` is asked
+  only when the zone listing fails, to tell a stopped daemon from a refused
+  read.
 - One group per zone, default zone first, then the other active zones; policy
   objects follow as further groups.
 - Every entry kind, each marked runtime-only or permanent-only where they

cmd/tui-firewall/check.go

@@ -364,6 +364,15 @@ func countDumpRules(d iptables.Dump) int {
 	return n
 }
 
+// checkFacts is what --check reports beside the model: the probed backend
+// version, what the detector saw of every backend, and why this one was
+// chosen. They are gathered while the backend loads; see run.
+type checkFacts struct {
+	compat    compat.Result
+	backends  []backends.State
+	selection string
+}
+
 // runCheck exercises the backend's real read path and prints the parsed model
 // as JSON. It returns an error when the backend cannot be read, which main
 // turns into a non-zero exit — so a caller can treat the exit code alone as
@@ -372,15 +381,16 @@ func countDumpRules(d iptables.Dump) int {
 // A backend that cannot be read fails here, and that is the correct result:
 // the exit code alone says whether this machine's firewall is legible to the
 // tool.
-func runCheck(backend firewall.Backend, backendCompat compat.Result,
-	states []backends.State, selection string, out io.Writer) error {
+func runCheck(backend firewall.Backend, facts <-chan checkFacts, out io.Writer) error {
 	ctx, cancel := context.WithTimeout(context.Background(), checkTimeout)
 	defer cancel()
 
 	model, err := backend.Load(ctx)
 	if err != nil {
 		return fmt.Errorf("%s backend read failed: %w", backend.Name(), err)
 	}
+	survey := <-facts
+	backendCompat, states, selection := survey.compat, survey.backends, survey.selection
 
 	report := checkReport{
 		Tool:      toolName,

cmd/tui-firewall/iptables_test.go

@@ -136,7 +136,9 @@ func TestIptablesStagingAppliesThroughRestoreAndRollsBack(t *testing.T) {
 func TestIptablesCheckReportsTheBlock(t *testing.T) {
 	fake := iptables.NewFake()
 	var out bytes.Buffer
-	if err := runCheck(fake, compat.Result{}, backends.Inspect("demo"), "demo", &out); err != nil {
+	facts := make(chan checkFacts, 1)
+	facts <- checkFacts{compat: compat.Result{}, backends: backends.Inspect("demo"), selection: "demo"}
+	if err := runCheck(fake, facts, &out); err != nil {
 		t.Fatalf("runCheck: %v", err)
 	}
 	var report struct {

cmd/tui-firewall/main.go

@@ -201,14 +201,24 @@ func run(args []string) error {
 	// and never starts a terminal program. It is checked after the backend is
 	// built so that a machine with no usable firewall fails here with the same
 	// message the UI would have shown.
-	// The backend version is probed once, here, and used by both paths: the
-	// header shows it, --check reports it, and the smoke test records it.
-	backendCompat := probeCompat(context.Background(), backend.Name(), opts.demo.on)
-
+	// The backend version is probed once and used by both paths: the header
+	// shows it, --check reports it, and the smoke test records it.
 	if opts.check {
-		return runCheck(backend, backendCompat, backends.Inspect(backend.Name()),
-			selectionDetail(cfg, opts), os.Stdout)
+		// The version probe and the backend survey are separate processes
+		// that do not depend on the firewall read, so --check runs them while
+		// the backend loads instead of before it: sibling tools call --check
+		// on every reload, and its cost is what they wait for.
+		facts := make(chan checkFacts, 1)
+		go func() {
+			facts <- checkFacts{
+				compat:    probeCompat(context.Background(), backend.Name(), opts.demo.on),
+				backends:  backends.Inspect(backend.Name()),
+				selection: selectionDetail(cfg, opts),
+			}
+		}()
+		return runCheck(backend, facts, os.Stdout)
 	}
+	backendCompat := probeCompat(context.Background(), backend.Name(), opts.demo.on)
 
 	a := newApp(backend, theme.New(), backendCompat)
 	a.queueOpen(opts.open.ports, opts.comment)

internal/firewalld/firewalld.go

@@ -12,15 +12,23 @@
 // `firewall-cmd` costs a process per read and buys a preview the user can
 // copy, paste and run themselves, which is the whole point of the tool.
 //
+// A process per read is not free: every firewall-cmd is a Python interpreter
+// that imports the firewalld client before its D-Bus calls, several hundred
+// milliseconds on a small VM. So Load asks for whole listings rather than
+// piecemeal answers, and starts its reads together; see loadSnapshot.
+//
 // # The mapping
 //
 // firewalld is zone-based, and a zone holds several species of entry at once,
 // so it maps onto the generic model like this:
 //
 //   - Model.Groups: one firewall.Group per zone, from `--list-all-zones`,
-//     ordered default zone first, then the other active zones
-//     (`--get-active-zones`), then the rest. Policy objects
-//     (`--get-policies`) follow as further groups, named with PolicyPrefix.
+//     ordered default zone first, then the other active zones, then the
+//     rest. Which zone is the default and which are active comes from the
+//     "(default, active)" flags of that same listing (`--get-default-zone`
+//     and `--get-active-zones` only on a firewalld that does not print
+//     them). Policy objects (`--list-all-policies`) follow as further
+//     groups, named with PolicyPrefix.
 //   - Group.Default.Target: the zone target, exposed through the single
 //     policy slot firewall.PolicyTarget. firewalld has no global
 //     incoming/outgoing/routed policies, so those slots stay empty.
@@ -35,7 +43,8 @@
 //     an entry present in only one is marked "runtime only" or
 //     "permanent only".
 //   - Model.Services: `--get-services`.
-//   - Model.Enabled: `--state`. Model.Logging: `--get-log-denied`.
+//   - Model.Enabled: the daemon answered the runtime zone listing (or
+//     `--state`, when it did not). Model.Logging: `--get-log-denied`.
 //   - Model.Warning: panic mode, from `--query-panic`.
 //   - Mutations: the runtime command and the same command with `--permanent`,
 //     both shown in the confirm dialog. No reload, so no connection is
@@ -110,12 +119,13 @@ func (r *Real) Run(ctx context.Context, change firewall.Change) (string, error)
 // Load reads the whole firewalld state: the daemon's own status, the runtime
 // and permanent zone listings, the policy objects and the global settings.
 //
-// Only `--state` is fatal. Everything else degrades: a firewalld too old for
+// Only a stopped daemon is fatal. Everything else degrades: a firewalld too old for
 // policy objects, or one that refuses a read, loses that part of the picture
-// rather than the whole screen.
+// rather than the whole screen. The reads themselves, and why they run
+// together rather than one after another, are described on loadSnapshot.
 func (r *Real) Load(ctx context.Context) (firewall.Model, error) {
-	state, err := r.run.Read(ctx, Bin, "--state")
-	if err != nil || !strings.Contains(state, "running") {
+	snapshot, running := loadSnapshot(ctx, r.read)
+	if !running {
 		// firewall-cmd cannot read anything while the daemon is down, so
 		// there is no partial picture to show — only the reason.
 		return firewall.Model{
@@ -124,51 +134,14 @@ func (r *Real) Load(ctx context.Context) (firewall.Model, error) {
 				"`systemctl start firewalld` and press R",
 		}, nil
 	}
-
-	snapshot := Snapshot{Running: true}
-	snapshot.DefaultZone = firstLine(r.read(ctx, "--get-default-zone"))
-	snapshot.Active = ParseActiveZones(r.read(ctx, "--get-active-zones"))
-	snapshot.Zones = ParseSections(r.read(ctx, "--list-all-zones"))
-	snapshot.PermanentZones = ParseSections(r.read(ctx, "--permanent", "--list-all-zones"))
-	snapshot.Services = ParseList(r.read(ctx, "--get-services"))
-	snapshot.LogDenied = firstLine(r.read(ctx, "--get-log-denied"))
-	snapshot.Panic = strings.TrimSpace(r.read(ctx, "--query-panic")) == "yes"
-	// Lockdown was removed in firewalld 2.2, where --query-lockdown exits 0
-	// and prints a deprecation sentence instead of an answer. Reading the
-	// answer rather than the exit code is therefore the version-proof test:
-	// only a literal "yes" means the feature exists and is on.
-	snapshot.Lockdown = strings.TrimSpace(r.read(ctx, "--query-lockdown")) == "yes"
-	r.readPolicies(ctx, &snapshot)
-
 	return BuildModel(snapshot), nil
 }
 
-// read runs one read and swallows its failure: every caller above treats a
-// missing answer as an absent feature rather than a broken tool.
-func (r *Real) read(ctx context.Context, args ...string) string {
-	out, err := r.run.Read(ctx, append([]string{Bin}, args...)...)
-	if err != nil {
-		return ""
-	}
-	return out
-}
-
-// readPolicies fills in the policy objects, which firewalld grew in 0.9. On an
-// older daemon `--get-policies` simply fails and the snapshot keeps none.
-func (r *Real) readPolicies(ctx context.Context, snapshot *Snapshot) {
-	names := ParseList(r.read(ctx, "--get-policies"))
-	if len(names) > maxPolicies {
-		names = names[:maxPolicies]
-	}
-	for _, name := range names {
-		if err := checkAtom("policy", name); err != nil {
-			continue
-		}
-		snapshot.Policies = append(snapshot.Policies,
-			ParseSections(r.read(ctx, "--policy="+name, "--list-all"))...)
-		snapshot.PermanentPolicies = append(snapshot.PermanentPolicies,
-			ParseSections(r.read(ctx, "--permanent", "--policy="+name, "--list-all"))...)
-	}
+// read runs one firewall-cmd read. It is the only place the real backend
+// reaches the runner for a read, so loadSnapshot can be driven by a table of
+// captured outputs in the tests.
+func (r *Real) read(ctx context.Context, args ...string) (string, error) {
+	return r.run.Read(ctx, append([]string{Bin}, args...)...)
 }
 
 // firstLine trims a one-value answer.