From f4a814df4e39ff5547a88d4f5d37ae6fe159cc76 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Wed, 13 May 2026 19:35:02 +0300 Subject: refactor: move TraceFilter and tracepoint selector logic out of flags.Config - Add tracepoints.Selector type with ShouldAttach method and ParseSelector constructor, replacing the raw TracepointsToAttach/TracepointsToExclude regex slices on flags.Config. - Add flags.BuildTraceFilter as a standalone function replacing the Config.TraceFilter() method, keeping filter-building logic out of the config struct. - Remove stale ShouldIAttachTracepoint noise-filter entry from Magefile. - Add selector_test.go with full coverage of ParseSelector and ShouldAttach. Co-Authored-By: Claude Sonnet 4.6 --- Magefile.go | 1 - internal/flags/flags.go | 90 +++++------------------------------ internal/flags/tracefilter.go | 33 +++++++++++++ internal/ior.go | 5 +- internal/ior_bpfsetup.go | 2 +- internal/tracepoints/selector.go | 85 +++++++++++++++++++++++++++++++++ internal/tracepoints/selector_test.go | 84 ++++++++++++++++++++++++++++++++ internal/tui/tui.go | 5 +- 8 files changed, 222 insertions(+), 83 deletions(-) create mode 100644 internal/flags/tracefilter.go create mode 100644 internal/tracepoints/selector.go create mode 100644 internal/tracepoints/selector_test.go diff --git a/Magefile.go b/Magefile.go index 9d4261d..6cfdab4 100644 --- a/Magefile.go +++ b/Magefile.go @@ -1050,7 +1050,6 @@ func shouldPrintTestLog(msg string) bool { "|___", "v0.0.0", "libbpf:", - "ShouldIAttachTracepoint called with ", "Attaching tracepoint ", "Attached prog handle_ ", "Attached tracepoint", diff --git a/internal/flags/flags.go b/internal/flags/flags.go index dc87e89..2544007 100644 --- a/internal/flags/flags.go +++ b/internal/flags/flags.go @@ -4,7 +4,6 @@ import ( "flag" "fmt" "os" - "regexp" "slices" "strings" "time" @@ -12,6 +11,7 @@ import ( "ior/internal/collapse" appconfig "ior/internal/config" "ior/internal/globalfilter" + "ior/internal/tracepoints" ) // Config captures runtime configuration parsed from CLI flags. @@ -31,12 +31,10 @@ type Config struct { // Duration is the maximum tracing duration in seconds. Duration int - // TracepointsToAttach is the list of compiled regexes that select which - // tracepoints to load; an empty list means attach all tracepoints. - TracepointsToAttach []*regexp.Regexp - // TracepointsToExclude is the list of compiled regexes that suppress - // specific tracepoints even when they match TracepointsToAttach. - TracepointsToExclude []*regexp.Regexp + // TracepointSelector holds the compiled include/exclude regexes that + // decide which BPF tracepoints to attach. The selection logic lives in + // tracepoints.Selector.ShouldAttach rather than on Config itself. + TracepointSelector tracepoints.Selector // PlainMode disables the TUI and writes raw CSV rows to stdout. PlainMode bool @@ -65,6 +63,7 @@ type Config struct { CountField string // GlobalFilter is the structured event filter applied across all dashboards // and output modes; takes precedence over the individual CLI filter flags. + // Use BuildTraceFilter(cfg) to obtain a resolved globalfilter.Filter. GlobalFilter globalfilter.Filter // ResetTimer is the interval at which aggregate dashboard state (flamegraph // trie and stats engine) is automatically cleared; 0 disables auto-reset. @@ -114,8 +113,7 @@ func (f Config) GetTUIExportEnable() bool { // fields so that modifications to the copy do not affect the original. func (f Config) Clone() Config { out := f - out.TracepointsToAttach = slices.Clone(f.TracepointsToAttach) - out.TracepointsToExclude = slices.Clone(f.TracepointsToExclude) + out.TracepointSelector = f.TracepointSelector.Clone() out.CollapsedFields = slices.Clone(f.CollapsedFields) out.GlobalFilter = f.GlobalFilter.Clone() return out @@ -146,8 +144,8 @@ func parseFromFlagSet(fs *flag.FlagSet, args []string) (Config, error) { fs.BoolVar(&cfg.PprofEnable, "pprof", false, "Enable profiling") - tracepointsToAttach := fs.String("tps", "", "Comma separated list regexes for tracepoints to load") - tracepointsToExclude := fs.String("tpsExclude", "", "Comma separated list regexes for tracepoints to exclude") + tpsAttach := fs.String("tps", "", "Comma separated list regexes for tracepoints to load") + tpsExclude := fs.String("tpsExclude", "", "Comma separated list regexes for tracepoints to exclude") fs.BoolVar(&cfg.PlainMode, "plain", false, "Enable plain CSV output mode (disable TUI)") fs.BoolVar(&cfg.FlamegraphOutput, "flamegraph", false, "Write aggregated .ior.zst output for trace/integration workflows") @@ -169,15 +167,13 @@ func parseFromFlagSet(fs *flag.FlagSet, args []string) (Config, error) { return Config{}, err } - var err error - cfg.TracepointsToAttach, err = extractTracepointFlags(*tracepointsToAttach) - if err != nil { - return Config{}, err - } - cfg.TracepointsToExclude, err = extractTracepointFlags(*tracepointsToExclude) + // Parse the tracepoint include/exclude regex lists into a Selector. + // The Selector owns all matching logic; Config is purely a data carrier. + sel, err := tracepoints.ParseSelector(*tpsAttach, *tpsExclude) if err != nil { return Config{}, err } + cfg.TracepointSelector = sel // Keep this list empty by default. // As of February 23, 2026, open_by_handle_at and name_to_handle_at were @@ -221,63 +217,3 @@ func parseFromFlagSet(fs *flag.FlagSet, args []string) (Config, error) { return cfg, nil } - -func extractTracepointFlags(tracepoints string) (regexes []*regexp.Regexp, err error) { - if len(tracepoints) == 0 { - return regexes, nil - } - for _, name := range strings.Split(tracepoints, ",") { - re, err := regexp.Compile(name) - if err != nil { - return nil, fmt.Errorf("unable to compile regex %q: %w", name, err) - } - regexes = append(regexes, re) - } - return regexes, nil -} - -// TraceFilter builds a globalfilter.Filter from the config's filter fields. -// If GlobalFilter is already active, it is returned as-is. Otherwise, -// individual CLI-level filters (CommFilter, PathFilter, PidFilter, TidFilter) -// are merged into a new filter. -func (cfg Config) TraceFilter() globalfilter.Filter { - filter := cfg.GlobalFilter.Clone() - if filter.IsActive() { - return filter - } - if cfg.CommFilter != "" { - filter.Comm = &globalfilter.StringFilter{Pattern: cfg.CommFilter} - } - if cfg.PathFilter != "" { - filter.File = &globalfilter.StringFilter{Pattern: cfg.PathFilter} - } - if cfg.PidFilter > 0 { - filter.PID = globalfilter.NewEqFilter(int64(cfg.PidFilter)) - } - if cfg.TidFilter > 0 { - filter.TID = globalfilter.NewEqFilter(int64(cfg.TidFilter)) - } - return filter -} - -// ShouldIAttachTracepoint reports whether the given tracepoint name passes the -// attach/exclude regex filters. Exclusions are checked first; if the name -// matches any exclude pattern it is rejected regardless of the attach list. -// When the attach list is empty, all non-excluded tracepoints are accepted. -func (f Config) ShouldIAttachTracepoint(tracepointName string) bool { - for _, re := range f.TracepointsToExclude { - if re.MatchString(tracepointName) { - return false - } - } - if len(f.TracepointsToAttach) == 0 { - return true - } - for _, re := range f.TracepointsToAttach { - if re.MatchString(tracepointName) { - return true - } - } - - return false -} diff --git a/internal/flags/tracefilter.go b/internal/flags/tracefilter.go new file mode 100644 index 0000000..90ac609 --- /dev/null +++ b/internal/flags/tracefilter.go @@ -0,0 +1,33 @@ +package flags + +import "ior/internal/globalfilter" + +// BuildTraceFilter constructs a globalfilter.Filter from the CLI-level filter +// fields stored in cfg. If a GlobalFilter is already active it is returned +// as-is (cloned), because the structured filter supersedes all individual CLI +// flags. Otherwise the per-field flags (CommFilter, PathFilter, PidFilter, +// TidFilter) are merged into a new filter. +// +// This function is the single authoritative place for turning a flags.Config +// into the globalfilter.Filter used by event ingestion. It replaces the +// former Config.TraceFilter() method so that filter-building logic lives in a +// dedicated function rather than on the configuration struct itself. +func BuildTraceFilter(cfg Config) globalfilter.Filter { + filter := cfg.GlobalFilter.Clone() + if filter.IsActive() { + return filter + } + if cfg.CommFilter != "" { + filter.Comm = &globalfilter.StringFilter{Pattern: cfg.CommFilter} + } + if cfg.PathFilter != "" { + filter.File = &globalfilter.StringFilter{Pattern: cfg.PathFilter} + } + if cfg.PidFilter > 0 { + filter.PID = globalfilter.NewEqFilter(int64(cfg.PidFilter)) + } + if cfg.TidFilter > 0 { + filter.TID = globalfilter.NewEqFilter(int64(cfg.TidFilter)) + } + return filter +} diff --git a/internal/ior.go b/internal/ior.go index 433484d..9a60869 100644 --- a/internal/ior.go +++ b/internal/ior.go @@ -367,9 +367,10 @@ func newEventLoopConfig(cfg flags.Config) eventLoopConfig { } } -// traceFilterFromConfig delegates to the canonical Config.TraceFilter method. +// traceFilterFromConfig delegates to flags.BuildTraceFilter to resolve the +// active event filter from the CLI configuration fields. func traceFilterFromConfig(cfg flags.Config) globalfilter.Filter { - return cfg.TraceFilter() + return flags.BuildTraceFilter(cfg) } func newLogger(verbose bool) func(...any) { diff --git a/internal/ior_bpfsetup.go b/internal/ior_bpfsetup.go index 885d321..0d32d4c 100644 --- a/internal/ior_bpfsetup.go +++ b/internal/ior_bpfsetup.go @@ -73,7 +73,7 @@ func setupBPFModule(parentCtx context.Context, cfg flags.Config) (*bpf.Module, * warn := func(syscall string, err error) { fmt.Fprintf(os.Stderr, "ior: skipping tracepoint for %s: %v\n", syscall, err) } - if err := mgr.AttachAll(cfg.ShouldIAttachTracepoint, tracepoints.List, warn); err != nil { + if err := mgr.AttachAll(cfg.TracepointSelector.ShouldAttach, tracepoints.List, warn); err != nil { mgr.Close() bpfModule.Close() return nil, nil, releaseBindings, setupBPFModuleError("attach probes", err) diff --git a/internal/tracepoints/selector.go b/internal/tracepoints/selector.go new file mode 100644 index 0000000..af2f39e --- /dev/null +++ b/internal/tracepoints/selector.go @@ -0,0 +1,85 @@ +package tracepoints + +import ( + "fmt" + "regexp" + "slices" + "strings" +) + +// Selector holds compiled include and exclude regexes for choosing which +// tracepoints to attach at BPF probe registration time. It is the single +// authoritative home for tracepoint-selection logic, keeping that concern +// out of the top-level flags.Config. +type Selector struct { + // Attach is the list of compiled regexes that select which tracepoints to + // load. An empty list means "attach all non-excluded tracepoints". + Attach []*regexp.Regexp + // Exclude is the list of compiled regexes that suppress specific + // tracepoints even when they match the Attach list. + Exclude []*regexp.Regexp +} + +// ParseSelector parses the comma-separated regex strings for the -tps and +// -tpsExclude CLI flags into a Selector. Either string may be empty, which +// leaves the corresponding list nil (i.e. "match all" for Attach, "exclude +// nothing" for Exclude). An error is returned if any regex fails to compile. +func ParseSelector(attach, exclude string) (Selector, error) { + attachRegexes, err := parseRegexList(attach) + if err != nil { + return Selector{}, err + } + excludeRegexes, err := parseRegexList(exclude) + if err != nil { + return Selector{}, err + } + return Selector{Attach: attachRegexes, Exclude: excludeRegexes}, nil +} + +// parseRegexList splits a comma-separated string of regex patterns and +// compiles each one. Returns nil (not an error) when the input is empty. +func parseRegexList(patterns string) ([]*regexp.Regexp, error) { + if len(patterns) == 0 { + return nil, nil + } + var regexes []*regexp.Regexp + for _, pattern := range strings.Split(patterns, ",") { + re, err := regexp.Compile(pattern) + if err != nil { + return nil, fmt.Errorf("unable to compile regex %q: %w", pattern, err) + } + regexes = append(regexes, re) + } + return regexes, nil +} + +// ShouldAttach reports whether the given tracepoint name passes the +// selector's attach/exclude regex filters. Exclusions are checked first; if +// the name matches any exclude pattern it is rejected regardless of the attach +// list. When the attach list is empty, all non-excluded tracepoints are +// accepted. +func (s Selector) ShouldAttach(tracepointName string) bool { + for _, re := range s.Exclude { + if re.MatchString(tracepointName) { + return false + } + } + if len(s.Attach) == 0 { + return true + } + for _, re := range s.Attach { + if re.MatchString(tracepointName) { + return true + } + } + return false +} + +// Clone returns a deep copy of the Selector so that modifications to the +// copy's slices do not affect the original. +func (s Selector) Clone() Selector { + return Selector{ + Attach: slices.Clone(s.Attach), + Exclude: slices.Clone(s.Exclude), + } +} diff --git a/internal/tracepoints/selector_test.go b/internal/tracepoints/selector_test.go new file mode 100644 index 0000000..d12f24b --- /dev/null +++ b/internal/tracepoints/selector_test.go @@ -0,0 +1,84 @@ +package tracepoints + +import "testing" + +func TestParseSelectorEmpty(t *testing.T) { + // An empty attach and exclude string means accept everything. + sel, err := ParseSelector("", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !sel.ShouldAttach("sys_enter_openat") { + t.Fatal("expected ShouldAttach=true for empty selector") + } +} + +func TestParseSelectorAttachFilter(t *testing.T) { + // Only openat tracepoints should be accepted when an explicit attach list + // is provided. + sel, err := ParseSelector("^sys_enter_openat$,^sys_exit_openat$", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !sel.ShouldAttach("sys_enter_openat") { + t.Error("expected ShouldAttach=true for sys_enter_openat") + } + if !sel.ShouldAttach("sys_exit_openat") { + t.Error("expected ShouldAttach=true for sys_exit_openat") + } + if sel.ShouldAttach("sys_enter_write") { + t.Error("expected ShouldAttach=false for sys_enter_write (not in attach list)") + } +} + +func TestParseSelectorExcludeFilter(t *testing.T) { + // Excluded tracepoints are rejected even when no explicit attach list exists. + sel, err := ParseSelector("", "name_to_handle_at,open_by_handle_at") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if sel.ShouldAttach("sys_enter_name_to_handle_at") { + t.Error("expected ShouldAttach=false for excluded tracepoint") + } + if !sel.ShouldAttach("sys_enter_openat") { + t.Error("expected ShouldAttach=true for non-excluded tracepoint") + } +} + +func TestParseSelectorExcludeTakesPrecedence(t *testing.T) { + // An exclude pattern beats an attach pattern when both match. + sel, err := ParseSelector("openat", "openat") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if sel.ShouldAttach("sys_enter_openat") { + t.Error("expected ShouldAttach=false: exclude must beat attach") + } +} + +func TestParseSelectorInvalidRegexReturnsError(t *testing.T) { + _, err := ParseSelector("[", "") + if err == nil { + t.Fatal("expected error for invalid attach regex") + } +} + +func TestParseSelectorInvalidExcludeRegexReturnsError(t *testing.T) { + _, err := ParseSelector("", "[") + if err == nil { + t.Fatal("expected error for invalid exclude regex") + } +} + +func TestSelectorCloneIsIndependent(t *testing.T) { + // Modifications to the clone's Attach slice must not affect the original. + sel, err := ParseSelector("openat", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + clone := sel.Clone() + clone.Attach = nil + if !sel.ShouldAttach("sys_enter_openat") { + t.Error("original Selector was mutated through clone") + } +} diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 2cebaf2..695bd02 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -986,9 +986,10 @@ func (m *Model) beginTraceCmd() tea.Cmd { return m.tracer.beginCmd(m.runtime, m.filters.current()) } -// filterFromConfig delegates to the canonical Config.TraceFilter method. +// filterFromConfig delegates to flags.BuildTraceFilter to resolve the active +// event filter from the CLI configuration fields. func filterFromConfig(cfg flags.Config) globalfilter.Filter { - return cfg.TraceFilter() + return flags.BuildTraceFilter(cfg) } // setProcessFilters updates the proc pid/tid, rebinds filter process constraints, -- cgit v1.2.3