[agent] Add udev DB reader and FillFromCrawler for device map

[fastrapier]

Summary

PR 2 of 3 in the effort to replace lsblk exec-based block device discovery.

Adds initial device population support to DeviceMap:

• ReadUdevDB — reads /run/udev/data/b<major>:<minor> and extracts
  E: properties. Crawler events from sysfs only carry basic kernel uevent
  fields (MAJOR, MINOR, DEVNAME, DEVTYPE); this function provides the
  udev-enriched fields (serial, model, wwn).
• EnrichWithUdevDB — merges udev DB properties into a raw event env
  map. Event keys take priority over DB keys. On any failure returns the
  original env with an explanatory error.
• FillFromCrawler — atomically replaces the entire DeviceMap with
  devices from crawler.Device (go-udev), enriching each with the udev
  database. Unparseable devices are skipped; all errors are returned to
  the caller.

How this will be used

PR 3 will call FillFromCrawler during scanner startup (after
go-udev/crawler.ExistingDevices) to populate the DeviceMap before
the netlink monitor starts receiving events.

What is NOT in this PR

• No crawler invocation (only accepts []crawler.Device)
• No sysfs I/O or BuildDevice / Snapshot
• No scanner integration

Test plan

• Unit tests covering ReadUdevDB, EnrichWithUdevDB (merge priority,
  error cases, immutability), FillFromCrawler (replace semantics, skip
  bad devices, nil input, udev DB enrichment)

[szhem]

Code Review

Blockers: none. Two High-severity findings that should be addressed before merge; the rest can be follow-ups.

Summary

Severity	Count
Critical	0
High	2
Medium	4
Low	5
Nit	1

Positive Observations

• Clean mutex discipline: file I/O in FillFromCrawler happens outside the lock; map swap is atomic under mu.Lock().
• EnrichWithUdevDB does not mutate the input env — explicitly covered by TestEnrichWithUdevDB_DoesNotMutateOriginal.
• All() returns a shallow copy — aliasing race prevented and validated by TestAll_ReturnsCopy.
• Sentinel ErrUnknownAction follows the Go guideline; errors.Is usage covered in tests.
• Field-resolution priorities (SCSI_IDENT_SERIAL → ID_SERIAL, ID_WWN_WITH_EXTENSION → ID_WWN, ID_MODEL_ENC → ID_MODEL) mirror lsblk exactly — good behavioural compatibility with the existing tooling.
• unhexmangle + normalizeWhitespace covered by honest tests, including malformed sequences (\xZZ, \x4).
• Every branch of HandleEvent is exercised (add/change/remove/bind/unbind/move/online/offline/unknown + bad MAJOR/MINOR + missing fields) — exemplary coverage.
• TestDeviceMap_ConcurrentAccess now collects goroutine errors instead of swallowing them (addresses prior review).

PR-level notes

• Tests (Nit): TestFillFromCrawler_ReplacesAll checks only Len() and absence of the old key — consider asserting that the new entries carry the expected DevName/Model so regressions in the fill path surface faster.
• Tests (Nit): TestHandleEvent_RemoveNonexistent_NoError codifies silent remove-of-unknown. A one-line comment explaining this is intentional (kernel may emit remove for devices the agent never saw) would help future readers.

[szhem]

Severity: Medium — Outdated godoc

After this PR the map is also populated by FillFromCrawler, but the doc still says it is only populated by netlink events. Readers of godoc will miss the bootstrap path.

Recommendation:

// DeviceMap is a thread-safe in-memory store of block device properties
// keyed by "major:minor". It is populated by FillFromCrawler at startup
// and by HandleEvent from netlink events thereafter. A consistent snapshot
// is available via All().

[fastrapier]

Fixed in cd4f80d. Godoc now mentions both FillFromCrawler and HandleEvent.

[szhem]

Severity: Low — HandleEvent godoc does not mention ErrUnknownAction

Returning the sentinel is part of the contract — tests rely on errors.Is(err, ErrUnknownAction), but callers reading the godoc will not know they can differentiate this failure mode.

Recommendation: add one line:

// Returns ErrUnknownAction (wrapped) when the action is not one of the
// listed values; callers can check via errors.Is.

[fastrapier]

Fixed in cd4f80d. Added ErrUnknownAction to HandleEvent godoc.

[szhem]

Severity: Medium — udevDBPath as a method parameter, not a DeviceMap field

The path to the udev DB is an environment invariant, not a per-call detail. Every caller now has to know and thread DefaultRunUdevDataPath through. If a future change wants to enrich in HandleEvent as well, the signature has to change again.

Recommendation:

type DeviceMap struct {
    mu         sync.RWMutex
    devices    map[string]Properties
    udevDBPath string
}

func NewDeviceMap(udevDBPath string) *DeviceMap {
    return &DeviceMap{
        devices:    make(map[string]Properties),
        udevDBPath: udevDBPath,
    }
}

func (dm *DeviceMap) FillFromCrawler(devices []crawler.Device) []error { ... }

Bonus: easier to mock in the PR 3 scanner tests — pass t.TempDir() into the constructor, no extra argument plumbing.

[fastrapier]

Fixed in cd4f80d. udevDBPath is now a DeviceMap struct field, passed via NewDeviceMap(udevDBPath).

[szhem]

Severity: Medium — FillFromCrawler does not accept a context.Context

The method performs synchronous file I/O on N devices with no timeout and no shutdown path. In PR 3 this runs on agent startup; if /run/udev/data/ is on slow storage or the pod is being terminated mid-start, the agent will hang.

Recommendation: prepare the signature now so PR 3 can wire a real context:

func (dm *DeviceMap) FillFromCrawler(ctx context.Context, devices []crawler.Device) []error {
    for _, dev := range devices {
        if err := ctx.Err(); err != nil {
            return append(errs, err)
        }
        ...
    }
}

[fastrapier]

Fixed in cd4f80d. FillFromCrawler now accepts context.Context and checks ctx.Err() on each iteration.

[szhem]

Severity: Nit — Preallocate errs

Upper bound is known (at most 2*len(devices) entries — enrichment + parse). A pre-sized slice avoids a couple of growslice calls.

Recommendation:

errs := make([]error, 0, len(devices))

[fastrapier]

Fixed in cd4f80d. errs is now preallocated: make([]error, 0, len(devices)).

[szhem]

Severity: Low — Consider returning error (via errors.Join) instead of []error

Go 1.20+ errors.Join is the idiomatic way to aggregate multiple errors. The caller gets a single error that still supports errors.Is/errors.As, logs as one line, and integrates with existing error-propagation code. Today every caller will have to loop through []error and glue the strings together.

Recommendation:

func (dm *DeviceMap) FillFromCrawler(...) error {
    ...
    return errors.Join(errs...)
}

[fastrapier]

Fixed in cd4f80d. FillFromCrawler now returns error via errors.Join(errs...).

[szhem]

Severity: Medium — Use filepath.Join instead of fmt.Sprintf for path construction

Current form does not handle a trailing slash in basePath cleanly (/run/udev/data/ → /run/udev/data//b8:0). It works, but the idiomatic Go equivalent is safer and self-documents intent.

Recommendation:

path := filepath.Join(basePath, fmt.Sprintf("b%d:%d", major, minor))

[fastrapier]

Fixed in cd4f80d. Now uses filepath.Join(basePath, fmt.Sprintf("b%d:%d", major, minor)).

[szhem]

Severity: Low — TrimSpace of the whole line can introduce a leading space in the key

The udev DB format is strict (E:KEY=VALUE, no whitespace), so today this does not matter. But if the format ever grows a tolerant variant like E: FOO=BAR, strings.CutPrefix(strings.TrimSpace(line), "E:") yields " FOO=BAR", and the key stored in the map becomes " FOO" with a leading space.

Recommendation: keep the prefix check literal and trim key/value separately if needed:

after, ok := strings.CutPrefix(line, "E:")
if !ok {
    continue
}
if idx := strings.IndexByte(after, '='); idx > 0 {
    props[after[:idx]] = strings.TrimRight(after[idx+1:], "\r\n")
}

[fastrapier]

Fixed in cd4f80d. Removed TrimSpace, now CutPrefix(line, "E:") directly. Value trimmed with TrimRight(after[idx+1:], "\r\n"). Also switched to IndexByte.

[szhem]

Severity: Low — Priority list allocated on every call

[]string{"SCSI_IDENT_SERIAL", ...} is a new literal every time serialFromUdevEnv runs. For hot reconciliation loops this is avoidable GC pressure.

Recommendation: hoist to a package-level var:

var serialKeyPriority = []string{
    "SCSI_IDENT_SERIAL",
    "ID_SCSI_SERIAL",
    "ID_SERIAL_SHORT",
    "ID_SERIAL",
}

func serialFromUdevEnv(env map[string]string) string {
    for _, key := range serialKeyPriority {
        ...
    }
}

[fastrapier]

Fixed in cd4f80d. Hoisted to package-level var serialKeyPriority.

[szhem]

Severity: Low — ensureDevPrefix has no defence against path traversal in DEVNAME

DEVNAME="../etc/passwd" becomes /dev/../etc/passwd. Today this value is only used as an identifier, so the risk is latent. But if a follow-up PR starts doing os.Open(props.DevName) or otherwise dereferences the path (for ioctl, size probing, etc.), this escapes /dev/ silently. The source of DEVNAME is trusted (kernel → udev), but defence in depth matters here.

Recommendation:

func ensureDevPrefix(devName string) string {
    if devName == "" || strings.HasPrefix(devName, "/dev/") {
        return devName
    }
    if strings.ContainsAny(devName, "/\x00") {
        return "" // or return an error from ParseProperties
    }
    return "/dev/" + devName
}

[fastrapier]

Leaving as-is. DEVNAME comes from the kernel (kobject uevent → dev->disk_name), not from user input. The value is always a flat device name like sda, nvme0n1p1, dm-0 — no path components. We don't do any file I/O on props.DevName; it's used purely as an identifier string. Adding validation for a kernel-sourced field that can never contain traversal sequences would be dead code.