Skip to content

Commit 98ec474

Browse files
ktsaouktsaou
committed
Reduce cgroups lookup protocol duplication
1 parent e2bd452 commit 98ec474

2 files changed

Lines changed: 93 additions & 61 deletions

File tree

‎.agents/sow/current/SOW-0027-20260629-netdata-vendor-memory-safety-update.md‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,22 @@ Risks:
6969
- Updating the wrong Netdata checkout can create unrelated branch drift.
7070
- Skipping the vendor diff can hide missing language-specific updates.
7171

72+
### PR CI Follow-up - 2026-07-01
73+
74+
Evidence:
75+
76+
- Netdata PR `netdata/netdata#22936` passed Codacy after source commit `e2bd45254b9f7182692fda8ed7c811d57fd03397` was vendored.
77+
- SonarCloud then reported zero open issues, but the quality gate still failed on duplication density: `new_duplicated_lines_density=3.0010718113612005`, threshold `3`.
78+
- Sonar duplication evidence pointed to `src/go/pkg/netipc/protocol/cgroups_lookup.go`:
79+
- duplicated block at lines 313-341 and 386-414: repeated cgroups lookup item-header parsing in validation and decode paths;
80+
- smaller duplicated builder conversion blocks later in the same file and against `apps_lookup.go`.
81+
82+
Plan:
83+
84+
- Fix the largest duplicated source block in `plugin-ipc`, not directly in Netdata, by extracting private cgroups lookup item-header parsing into one helper.
85+
- Keep the wire format, public Go API, and validation semantics unchanged.
86+
- Run focused Go protocol tests in source, commit/push source, then re-vendor into Netdata and re-check Sonar.
87+
7288
## Pre-Implementation Gate
7389

7490
Status: in-progress

‎src/go/pkg/netipc/protocol/cgroups_lookup.go‎

Lines changed: 77 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,16 @@ type CgroupsLookupItemView struct {
4040
labelTableOffset int
4141
}
4242

43+
type cgroupsLookupItemHeader struct {
44+
status uint16
45+
orchestrator uint16
46+
pathOff int
47+
pathLen int
48+
nameOff int
49+
nameLen int
50+
labelCount uint16
51+
}
52+
4353
func validateCgroupsLookupSemantics(status, orchestrator uint16, pathLen, nameLen, labelCount int) error {
4454
if status != CgroupLookupKnown && status != CgroupLookupUnknownRetryLater &&
4555
status != CgroupLookupUnknownPermanent && status != CgroupLookupPayloadExceeded &&
@@ -55,6 +65,38 @@ func validateCgroupsLookupSemantics(status, orchestrator uint16, pathLen, nameLe
5565
return nil
5666
}
5767

68+
func readCgroupsLookupItemHeader(item []byte) (cgroupsLookupItemHeader, error) {
69+
var header cgroupsLookupItemHeader
70+
if len(item) < CgroupsLookupItemHdr {
71+
return header, ErrTruncated
72+
}
73+
74+
header.status = ne.Uint16(item[2:4])
75+
header.orchestrator = ne.Uint16(item[4:6])
76+
var err error
77+
header.pathOff, err = checkedWireU32Int(item, 8)
78+
if err != nil {
79+
return header, err
80+
}
81+
header.pathLen, err = checkedWireU32Int(item, 12)
82+
if err != nil {
83+
return header, err
84+
}
85+
header.nameOff, err = checkedWireU32Int(item, 16)
86+
if err != nil {
87+
return header, err
88+
}
89+
header.nameLen, err = checkedWireU32Int(item, 20)
90+
if err != nil {
91+
return header, err
92+
}
93+
header.labelCount = ne.Uint16(item[24:26])
94+
if ne.Uint16(item[0:2]) != 1 || ne.Uint16(item[6:8]) != 0 || ne.Uint16(item[26:28]) != 0 {
95+
return header, ErrBadLayout
96+
}
97+
return header, nil
98+
}
99+
58100
func EncodeCgroupsLookupRequest(paths [][]byte, buf []byte) (int, error) {
59101
count := len(paths)
60102
count32, ok := checkedU32Int(count)
@@ -310,49 +352,33 @@ func EncodeCgroupsLookupRawResponse(items [][]byte, generation uint64, buf []byt
310352
}
311353

312354
func validateCgroupsLookupItem(item []byte) error {
313-
if len(item) < CgroupsLookupItemHdr {
314-
return ErrTruncated
315-
}
316-
status := ne.Uint16(item[2:4])
317-
orchestrator := ne.Uint16(item[4:6])
318-
pathOff, err := checkedWireU32Int(item, 8)
319-
if err != nil {
320-
return err
321-
}
322-
pathLen, err := checkedWireU32Int(item, 12)
323-
if err != nil {
324-
return err
325-
}
326-
nameOff, err := checkedWireU32Int(item, 16)
327-
if err != nil {
328-
return err
329-
}
330-
nameLen, err := checkedWireU32Int(item, 20)
355+
header, err := readCgroupsLookupItemHeader(item)
331356
if err != nil {
332357
return err
333358
}
334-
labelCount := ne.Uint16(item[24:26])
335-
if ne.Uint16(item[0:2]) != 1 || ne.Uint16(item[6:8]) != 0 || ne.Uint16(item[26:28]) != 0 {
336-
return ErrBadLayout
337-
}
338-
if err := validateCgroupsLookupSemantics(status, orchestrator, pathLen, nameLen, int(labelCount)); err != nil {
359+
if err := validateCgroupsLookupSemantics(
360+
header.status,
361+
header.orchestrator,
362+
header.pathLen,
363+
header.nameLen,
364+
int(header.labelCount)); err != nil {
339365
return err
340366
}
341-
if status != CgroupLookupKnown {
342-
return validateCgroupsLookupUnknownItem(item, pathOff, pathLen, nameOff)
367+
if header.status != CgroupLookupKnown {
368+
return validateCgroupsLookupUnknownItem(item, header.pathOff, header.pathLen, header.nameOff)
343369
}
344-
pathEnd, err := lookupStringInto(item, CgroupsLookupItemHdr, pathOff, pathLen, nil)
370+
pathEnd, err := lookupStringInto(item, CgroupsLookupItemHdr, header.pathOff, header.pathLen, nil)
345371
if err != nil {
346372
return err
347373
}
348-
nameEnd, err := lookupStringInto(item, CgroupsLookupItemHdr, nameOff, nameLen, nil)
374+
nameEnd, err := lookupStringInto(item, CgroupsLookupItemHdr, header.nameOff, header.nameLen, nil)
349375
if err != nil {
350376
return err
351377
}
352-
if overlap(pathOff, pathEnd, nameOff, nameEnd) {
378+
if overlap(header.pathOff, pathEnd, header.nameOff, nameEnd) {
353379
return ErrBadLayout
354380
}
355-
_, err = validateLabels(item, CgroupsLookupItemHdr, labelCount, max(pathEnd, nameEnd))
381+
_, err = validateLabels(item, CgroupsLookupItemHdr, header.labelCount, max(pathEnd, nameEnd))
356382
return err
357383
}
358384

@@ -383,61 +409,51 @@ func decodeCgroupsLookupItem(item []byte) (*CgroupsLookupItemView, error) {
383409
}
384410

385411
func decodeCgroupsLookupItemInto(item []byte, out *CgroupsLookupItemView) error {
386-
if len(item) < CgroupsLookupItemHdr {
387-
return ErrTruncated
388-
}
389-
status := ne.Uint16(item[2:4])
390-
orchestrator := ne.Uint16(item[4:6])
391-
pathOff, err := checkedWireU32Int(item, 8)
392-
if err != nil {
393-
return err
394-
}
395-
pathLen, err := checkedWireU32Int(item, 12)
412+
header, err := readCgroupsLookupItemHeader(item)
396413
if err != nil {
397414
return err
398415
}
399-
nameOff, err := checkedWireU32Int(item, 16)
400-
if err != nil {
416+
if err := validateCgroupsLookupSemantics(
417+
header.status,
418+
header.orchestrator,
419+
header.pathLen,
420+
header.nameLen,
421+
int(header.labelCount)); err != nil {
401422
return err
402423
}
403-
nameLen, err := checkedWireU32Int(item, 20)
404-
if err != nil {
405-
return err
406-
}
407-
labelCount := ne.Uint16(item[24:26])
408-
if ne.Uint16(item[0:2]) != 1 || ne.Uint16(item[6:8]) != 0 || ne.Uint16(item[26:28]) != 0 {
409-
return ErrBadLayout
410-
}
411-
if err := validateCgroupsLookupSemantics(status, orchestrator, pathLen, nameLen, int(labelCount)); err != nil {
412-
return err
413-
}
414-
if status != CgroupLookupKnown {
415-
return decodeCgroupsLookupUnknownItemInto(item, status, pathOff, pathLen, nameOff, out)
424+
if header.status != CgroupLookupKnown {
425+
return decodeCgroupsLookupUnknownItemInto(
426+
item,
427+
header.status,
428+
header.pathOff,
429+
header.pathLen,
430+
header.nameOff,
431+
out)
416432
}
417433
var path CStringView
418-
pathEnd, err := lookupStringInto(item, CgroupsLookupItemHdr, pathOff, pathLen, viewOut(out, &path))
434+
pathEnd, err := lookupStringInto(item, CgroupsLookupItemHdr, header.pathOff, header.pathLen, viewOut(out, &path))
419435
if err != nil {
420436
return err
421437
}
422438
var name CStringView
423-
nameEnd, err := lookupStringInto(item, CgroupsLookupItemHdr, nameOff, nameLen, viewOut(out, &name))
439+
nameEnd, err := lookupStringInto(item, CgroupsLookupItemHdr, header.nameOff, header.nameLen, viewOut(out, &name))
424440
if err != nil {
425441
return err
426442
}
427-
if overlap(pathOff, pathEnd, nameOff, nameEnd) {
443+
if overlap(header.pathOff, pathEnd, header.nameOff, nameEnd) {
428444
return ErrBadLayout
429445
}
430-
table, err := validateLabels(item, CgroupsLookupItemHdr, labelCount, max(pathEnd, nameEnd))
446+
table, err := validateLabels(item, CgroupsLookupItemHdr, header.labelCount, max(pathEnd, nameEnd))
431447
if err != nil {
432448
return err
433449
}
434450
if out != nil {
435451
*out = CgroupsLookupItemView{
436-
Status: status,
437-
Orchestrator: orchestrator,
452+
Status: header.status,
453+
Orchestrator: header.orchestrator,
438454
Path: path,
439455
Name: name,
440-
LabelCount: labelCount,
456+
LabelCount: header.labelCount,
441457
item: item,
442458
labelTableOffset: table,
443459
}

0 commit comments

Comments
 (0)