marker: validate def BPF once, skip re-validation on inherited fan-out

A marker definition validated its BPF N times — once per link in the fan-out
(start_marker runs validate_bpf_syntax on every copy), spawning one tcpdump -d
subprocess per link for the same expression. Definitions did not validate BPF
at all; only direction was checked.

Move the single validation point to the definition layer (create/update), and
validate each definition's BPF on project load (dropping any that have gone
invalid, like private markers). The inherited fan-out (start_marker) and def
sync (update_marker) now skip validate_bpf_syntax for inherited copies, since
the BPF comes from an already-validated definition. Private per-link markers
still validate inline as before. uBridge still runs pcap_compile at install, so
an invalid expression can never slip through.

Creating a definition over N links now runs one tcpdump instead of N.

docs/features/marker-traffic-insight.md

@@ -348,6 +348,12 @@ direction relative to the capture node; see [Direction](#direction).
 - **Render hints are not enforced.** `color` and `highlight_duration` (milliseconds, `>= 1`)
   are stored on the link and never sent to uBridge; `null` lets the UI apply its own
   default. A partial PUT (e.g. changing only `bpf`) leaves them untouched.
+- **BPF is validated once per source.** A private per-link marker validates its BPF inline
+  on create/update. A definition validates its BPF once at create/update (and once per
+  definition on project load, dropping any whose BPF has gone invalid); the inherited
+  fan-out to every link then skips re-validation, so creating a definition over *N* links
+  runs one `tcpdump -d` rather than *N*. (uBridge still runs `pcap_compile` itself at
+  install time, so an invalid expression can never slip through.)
 - **Supported node types.** A marker needs a uBridge bridge: `vpcs`, `qemu`, `docker`,
   `iou`, `dynamips`, `cloud` (one capable endpoint suffices). Types without a uBridge are
   silently skipped by the inheritance fan-out. IOU uses one shared `IOL-BRIDGE` per node but

gns3server/controller/project.py

@@ -965,6 +965,21 @@ class Project:
         """
         return self._marker_definitions
 
+    def _validate_marker_definition_bpf(self, name, bpf):
+        """
+        Validate a marker definition's BPF once, here, so the fan-out to every
+        link (``_apply_def_to_all_links`` → ``inherit_marker`` → ``start_marker``)
+        and the per-link sync (``update_marker_definition`` → ``update_marker``)
+        can skip re-validation for the inherited copies — otherwise one
+        ``tcpdump -d`` subprocess runs per link for the same expression. A
+        private per-link marker still validates in ``start_marker``/``update_marker``.
+        """
+        result = validate_bpf_syntax(bpf)
+        if not result.get("valid"):
+            raise ControllerError(
+                f"Marker definition '{name}': invalid BPF — {result.get('error', 'unknown error')}"
+            )
+
     def _validate_marker_definition_direction(self, name, direction):
         """
         Reject tx/rx on a marker definition: a definition fans out to every link
@@ -997,6 +1012,7 @@ class Project:
                 f"Marker definition '{name}' already exists in this project"
             )
 
+        self._validate_marker_definition_bpf(name, bpf)
         self._validate_marker_definition_direction(name, direction)
         self._marker_definitions[name] = {"bpf": bpf, "tag": tag, "direction": direction, "color": color, "highlight_duration": highlight_duration, "paused": False}
         await self._apply_def_to_all_links(name)
@@ -1015,6 +1031,7 @@ class Project:
 
         d = self._marker_definitions[name]
         if bpf is not None:
+            self._validate_marker_definition_bpf(name, bpf)
             d["bpf"] = bpf
         if tag is not None:
             d["tag"] = tag
@@ -1489,10 +1506,27 @@ class Project:
                     setattr(self, key, val)
 
             # marker_definitions is loaded separately (it is not a __init__ kwarg
-            # nor a simple attribute — it backs a read-only property).
+            # nor a simple attribute — it backs a read-only property). Each BPF
+            # is validated once here so the inherited fan-out (start_marker) can
+            # skip re-validation; an invalid definition is dropped with a warning
+            # rather than failing the open — it could not fan out anyway.
             defs = project_data.get("marker_definitions")
             if isinstance(defs, dict):
-                self._marker_definitions = defs
+                clean_defs = {}
+                for def_name, d in defs.items():
+                    bpf = d.get("bpf")
+                    if not bpf:
+                        log.warning("Dropping marker definition '%s' on load: missing bpf", def_name)
+                        continue
+                    result = validate_bpf_syntax(bpf)
+                    if not result.get("valid"):
+                        log.warning(
+                            "Dropping marker definition '%s' on load: invalid BPF (%s)",
+                            def_name, result.get("error")
+                        )
+                        continue
+                    clean_defs[def_name] = d
+                self._marker_definitions = clean_defs
 
             topology = project_data["topology"]
             for compute in topology.get("computes", []):

gns3server/controller/udp_link.py

@@ -379,9 +379,15 @@ class UDPLink(Link):
         if name in self._markers:
             raise ControllerError(f"Marker '{name}' already exists on link {self._id}")
 
-        result = validate_bpf_syntax(bpf)
-        if not result.get("valid"):
-            raise ControllerError(f"Invalid BPF expression: {result.get('error', 'unknown error')}")
+        # Validate the BPF only for private per-link markers. An inherited copy
+        # (``inherited_from`` set) fans out from a definition whose BPF was
+        # already validated once at create/update (and on project load), so
+        # re-validating per link would spawn one ``tcpdump -d`` per link for the
+        # same expression.
+        if not inherited_from:
+            result = validate_bpf_syntax(bpf)
+            if not result.get("valid"):
+                raise ControllerError(f"Invalid BPF expression: {result.get('error', 'unknown error')}")
 
         if capture_node_id and not inherited_from:
             marker_side = self._node_by_id(capture_node_id)
@@ -474,9 +480,13 @@ class UDPLink(Link):
 
         # Merge every changed field into the marker state first.
         if bpf is not None and bpf != marker_info["bpf"]:
-            result = validate_bpf_syntax(bpf)
-            if not result.get("valid"):
-                raise ControllerError(f"Invalid BPF expression: {result.get('error', 'unknown error')}")
+            # An inherited marker is synced from a definition whose BPF was
+            # already validated at create/update (or load); re-validating per
+            # link is redundant. Private markers validate here as before.
+            if not inherited:
+                result = validate_bpf_syntax(bpf)
+                if not result.get("valid"):
+                    raise ControllerError(f"Invalid BPF expression: {result.get('error', 'unknown error')}")
             marker_info["bpf"] = bpf
         if tag is not None:
             marker_info["tag"] = tag

tests/controller/test_marker.py

@@ -28,6 +28,7 @@ import uuid
 
 import pytest
 from unittest.mock import MagicMock, patch
+from contextlib import ExitStack
 
 from tests.utils import AsyncioMagicMock
 
@@ -38,11 +39,20 @@ from gns3server.controller.controller_error import ControllerError, ControllerNo
 
 
 def _valid_bpf():
-    """Bypass tcpdump-based BPF validation so tests don't depend on tcpdump."""
-    return patch(
+    """Bypass tcpdump-based BPF validation so tests don't depend on tcpdump.
+
+    Patches both namespaces that import ``validate_bpf_syntax`` by name: the
+    per-link ``udp_link`` (private marker create/update) and the project layer
+    (definition create/update/load), which is now the single validation point
+    for inherited copies.
+    """
+    stack = ExitStack()
+    for target in (
         "gns3server.controller.udp_link.validate_bpf_syntax",
-        return_value={"valid": True, "error": None},
-    )
+        "gns3server.controller.project.validate_bpf_syntax",
+    ):
+        stack.enter_context(patch(target, return_value={"valid": True, "error": None}))
+    return stack
 
 
 async def _make_link(project):
@@ -438,6 +448,73 @@ async def test_apply_defs_to_new_link(project):
     assert new_link.markers["global-arp"]["inherited_from"] == "arp"
 
 
+@pytest.mark.asyncio
+async def test_create_marker_definition_validates_bpf_once(project):
+    # A definition validates its BPF once (project layer); the inherited fan-out
+    # to every link must NOT re-validate — no tcpdump subprocess per link.
+    with patch("gns3server.controller.project.validate_bpf_syntax",
+               return_value={"valid": True, "error": None}) as proj_val, \
+         patch("gns3server.controller.udp_link.validate_bpf_syntax",
+               return_value={"valid": True, "error": None}) as link_val:
+        await _make_link(project)
+        await _make_link(project)
+        await project.create_marker_definition("arp", "arp")
+
+    assert proj_val.call_count == 1          # validated once at the def layer
+    assert link_val.call_count == 0          # fan-out skipped per-link validation
+
+
+@pytest.mark.asyncio
+async def test_create_marker_definition_rejects_invalid_bpf(project):
+
+    with patch("gns3server.controller.project.validate_bpf_syntax",
+               return_value={"valid": False, "error": "syntax error"}):
+        with pytest.raises(ControllerError):
+            await project.create_marker_definition("arp", "not a real bpf")
+    assert "arp" not in project.marker_definitions
+
+
+@pytest.mark.asyncio
+async def test_update_marker_definition_skips_per_link_validation(project):
+    # Updating a def's BPF validates once more (project); the per-link sync
+    # (update_marker with inherited=True) must NOT re-validate.
+    with patch("gns3server.controller.project.validate_bpf_syntax",
+               return_value={"valid": True, "error": None}) as proj_val, \
+         patch("gns3server.controller.udp_link.validate_bpf_syntax",
+               return_value={"valid": True, "error": None}) as link_val:
+        await _make_link(project)
+        await _make_link(project)
+        await project.create_marker_definition("arp", "arp")
+        await project.update_marker_definition("arp", bpf="arp or rarp")
+
+    assert proj_val.call_count == 2          # once on create, once on update
+    assert link_val.call_count == 0          # sync skipped per-link validation
+
+
+@pytest.mark.asyncio
+async def test_start_marker_skips_validation_for_inherited(project):
+    # An inherited marker rides an already-validated definition BPF, so
+    # start_marker must not call validate_bpf_syntax.
+    with patch("gns3server.controller.udp_link.validate_bpf_syntax",
+               return_value={"valid": True, "error": None}) as link_val:
+        link = await _make_link(project)
+        await link.inherit_marker("arp", {"bpf": "arp"})
+
+    assert link_val.call_count == 0
+    assert link.markers["global-arp"]["bpf"] == "arp"
+
+
+@pytest.mark.asyncio
+async def test_start_marker_validates_for_private(project):
+    # A private (non-inherited) marker still validates inline.
+    with patch("gns3server.controller.udp_link.validate_bpf_syntax",
+               return_value={"valid": True, "error": None}) as link_val:
+        link = await _make_link(project)
+        await link.start_marker("icmp", "icmp")
+
+    assert link_val.call_count == 1
+
+
 @pytest.mark.asyncio
 async def test_markers_aggregation(project):