From 2c497b738f05a8f2d1ae75619badc264679231b6 Mon Sep 17 00:00:00 2001 From: Christoph Ostarek Date: Wed, 25 Sep 2024 17:14:53 +0200 Subject: [PATCH] pkg/pillar: make ioBundleError deepcopy-able error types very often have private members that cannot be marshalled with `json.Marshal` we fix this by only storing the error type and the error string (err.Error()) as values in uppercase members of our own ioBundleError type log output: ``` json Unmarshal in deepCopy: json: cannot unmarshal object into Go struct field IoBundleError.IoBundleList.Error.Errors of type error ``` introduced by d1e13b8f880218470de48c4d569957dd89410de4 Signed-off-by: Christoph Ostarek --- pkg/pillar/pubsub/publish.go | 6 +- pkg/pillar/pubsub/subscribe.go | 2 +- pkg/pillar/pubsub/util.go | 4 +- pkg/pillar/pubsub/util_test.go | 61 ++++++++ pkg/pillar/types/assignableadapters.go | 150 +++++++++++++------- pkg/pillar/types/assignableadapters_test.go | 39 ++++- 6 files changed, 201 insertions(+), 61 deletions(-) create mode 100644 pkg/pillar/pubsub/util_test.go diff --git a/pkg/pillar/pubsub/publish.go b/pkg/pillar/pubsub/publish.go index 387154f253f..b4676718f7e 100644 --- a/pkg/pillar/pubsub/publish.go +++ b/pkg/pillar/pubsub/publish.go @@ -110,7 +110,7 @@ func (pub *PublicationImpl) Publish(key string, item interface{}) error { pub.log.Fatalf("Publish got a pointer for %s", name) } // Perform a deepCopy in case the caller might change a map etc - newItem := deepCopy(pub.log, item) + newItem := DeepCopy(pub.log, item) if m, ok := pub.km.key.Load(key); ok { if cmp.Equal(m, newItem) { pub.log.Tracef("Publish(%s/%s) unchanged\n", name, key) @@ -190,7 +190,7 @@ func (pub *PublicationImpl) ClearRestarted() error { func (pub *PublicationImpl) Get(key string) (interface{}, error) { m, ok := pub.km.key.Load(key) if ok { - newIntf := deepCopy(pub.log, m) + newIntf := DeepCopy(pub.log, m) return newIntf, nil } else { name := pub.nameString() @@ -203,7 +203,7 @@ func (pub *PublicationImpl) Get(key string) (interface{}, error) { func (pub *PublicationImpl) GetAll() map[string]interface{} { result := make(map[string]interface{}) assigner := func(key string, val interface{}) bool { - newVal := deepCopy(pub.log, val) + newVal := DeepCopy(pub.log, val) result[key] = newVal return true } diff --git a/pkg/pillar/pubsub/subscribe.go b/pkg/pillar/pubsub/subscribe.go index f9129ab9a44..680ad284cae 100644 --- a/pkg/pillar/pubsub/subscribe.go +++ b/pkg/pillar/pubsub/subscribe.go @@ -263,7 +263,7 @@ func handleModify(ctxArg interface{}, key string, itemcb []byte) { sub.dump("after handleModify") } // Need a copy in case the caller will modify e.g., embedded maps - newItem := deepCopy(sub.log, item) + newItem := DeepCopy(sub.log, item) if created { if sub.CreateHandler != nil { (sub.CreateHandler)(sub.userCtx, key, newItem) diff --git a/pkg/pillar/pubsub/util.go b/pkg/pillar/pubsub/util.go index b0461975b6f..28c857c9263 100644 --- a/pkg/pillar/pubsub/util.go +++ b/pkg/pillar/pubsub/util.go @@ -14,7 +14,7 @@ import ( "github.com/lf-edge/eve/pkg/pillar/base" ) -// deepCopy returns the same type as what is passed as input +// DeepCopy returns the same type as what is passed as input // Warning: only public fields will be exported // Note why json marshalling is used: // Type casting and associated type assertions in golang are only @@ -24,7 +24,7 @@ import ( // Golang doesn't have support for a deep copy. Once can build it // oneself using reflect package, but it ends up doing the same thing // as json encode+decode apart from the exported fields check. -func deepCopy(log *base.LogObject, in interface{}) interface{} { +func DeepCopy(log *base.LogObject, in interface{}) interface{} { b, err := json.Marshal(in) if err != nil { log.Fatal("json Marshal in deepCopy", err) diff --git a/pkg/pillar/pubsub/util_test.go b/pkg/pillar/pubsub/util_test.go new file mode 100644 index 00000000000..c9124a56b32 --- /dev/null +++ b/pkg/pillar/pubsub/util_test.go @@ -0,0 +1,61 @@ +// Copyright (c) 2024 Zededa, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pubsub_test + +import ( + "fmt" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/lf-edge/eve/pkg/pillar/base" + "github.com/lf-edge/eve/pkg/pillar/pubsub" + "github.com/lf-edge/eve/pkg/pillar/types" + "github.com/sirupsen/logrus" +) + +func TestDeepCopyIoBundlError(t *testing.T) { + logger := logrus.StandardLogger() + log := base.NewSourceLogObject(logger, "test", 1234) + + errs := []error{ + fmt.Errorf("some error"), + types.ErrOwnParent{}, + types.ErrParentAssigngrpMismatch{}, + types.ErrEmptyAssigngrpWithParent{}, + types.ErrCycleDetected{}, + types.ErrIOBundleCollision{ + Collisions: []types.IOBundleCollision{{ + Phylabel: "phy1", + USBAddr: "usb1", + USBProduct: "usbprod1", + PCILong: "pcilong", + Assigngrp: "assigngrp", + }, + }, + }, + } + iob := types.IoBundle{ + Error: types.IOBundleError{ + TimeOfError: time.Time{}, + }, + } + + for _, err := range errs { + iob.Error.Append(err) + } + output := pubsub.DeepCopy(log, iob) + + t.Logf("copy: %v", output) + + for _, err := range errs { + if !iob.Error.HasError(err) { + t.Fatalf("error %v missing", err) + } + } + + if !cmp.Equal(output, iob) { + t.Fatalf("not equal: %s", cmp.Diff(output, iob)) + } +} diff --git a/pkg/pillar/types/assignableadapters.go b/pkg/pillar/types/assignableadapters.go index d9bf185cacd..f04888c84b6 100644 --- a/pkg/pillar/types/assignableadapters.go +++ b/pkg/pillar/types/assignableadapters.go @@ -12,7 +12,6 @@ package types // file on boot. import ( - "errors" "fmt" "reflect" "strings" @@ -30,16 +29,27 @@ type AssignableAdapters struct { IoBundleList []IoBundle } -type ioBundleError struct { - Errors []error +type ioBundleErrorBase struct { + ErrStr string + TypeStr string +} + +func (i ioBundleErrorBase) Error() string { + return i.ErrStr +} + +// IOBundleError is an error stored in IoBundles that can be marshalled +type IOBundleError struct { + Errors []ioBundleErrorBase TimeOfError time.Time } -func (iobe *ioBundleError) ErrorTime() time.Time { +// ErrorTime returns the time of the last error added +func (iobe *IOBundleError) ErrorTime() time.Time { return iobe.TimeOfError } -func (iobe *ioBundleError) String() string { +func (iobe *IOBundleError) String() string { if len(iobe.Errors) == 0 { return "" } @@ -50,17 +60,25 @@ func (iobe *ioBundleError) String() string { return strings.Join(errorStrings, "; ") } -func (iobe *ioBundleError) Append(err error) { +// Append converts an error to ioBundleErrorBase and adds it +func (iobe *IOBundleError) Append(err error) { if iobe.Errors == nil { - iobe.Errors = make([]error, 0, 1) + iobe.Errors = make([]ioBundleErrorBase, 0, 1) } - iobe.Errors = append(iobe.Errors, err) + typeStr := reflect.TypeOf(err).String() + baseErr := ioBundleErrorBase{ + ErrStr: err.Error(), + TypeStr: typeStr, + } + + iobe.Errors = append(iobe.Errors, baseErr) iobe.TimeOfError = time.Now() } -func (iobe *ioBundleError) Empty() bool { +// Empty returns true if no error has been added +func (iobe *IOBundleError) Empty() bool { if iobe.Errors == nil || len(iobe.Errors) == 0 { return true } @@ -68,9 +86,15 @@ func (iobe *ioBundleError) Empty() bool { return false } -func (iobe *ioBundleError) hasError(e error) bool { +// HasErrorByType returns true if error of the same type is found +func (iobe *IOBundleError) HasErrorByType(e error) bool { + typeStr := reflect.TypeOf(e).String() + base, ok := e.(ioBundleErrorBase) + if ok { + typeStr = base.TypeStr + } for _, err := range iobe.Errors { - if reflect.TypeOf(e) == reflect.TypeOf(err) { + if typeStr == err.TypeStr { return true } } @@ -78,10 +102,11 @@ func (iobe *ioBundleError) hasError(e error) bool { return false } -func (iobe *ioBundleError) removeByType(e error) { +func (iobe *IOBundleError) removeByType(e error) { + typeStr := reflect.TypeOf(e).String() toRemoveIndices := []int{} for i, err := range iobe.Errors { - if reflect.TypeOf(e) == reflect.TypeOf(err) { + if typeStr == err.TypeStr { toRemoveIndices = append(toRemoveIndices, i) } } @@ -92,8 +117,9 @@ func (iobe *ioBundleError) removeByType(e error) { } } -func (iobe *ioBundleError) Clear() { - iobe.Errors = make([]error, 0) +// Clear clears all errors +func (iobe *IOBundleError) Clear() { + iobe.Errors = make([]ioBundleErrorBase, 0) iobe.TimeOfError = time.Time{} } @@ -164,7 +190,7 @@ type IoBundle struct { // Do not put device under pciBack, instead keep it in dom0 as long as it is not assigned to any application. // In other words, this does not prevent assignments but keeps unassigned devices visible to EVE. KeepInHost bool - Error ioBundleError + Error IOBundleError // Only used in PhyIoNetEthPF Vfs sriov.VFList @@ -525,13 +551,33 @@ func (aa *AssignableAdapters) LookupIoBundleIfName(ifname string) *IoBundle { return nil } -var errOwnParent = errors.New("IOBundle cannot be it's own parent") +// ErrOwnParent describes an error where an IoBundle is parent of itself +type ErrOwnParent struct{} -var errParentAssigngrpMismatch = errors.New("IOBundle with parentassigngrp mismatch found") +func (ErrOwnParent) Error() string { + return "IOBundle cannot be it's own parent" +} -var errEmptyAssigngrpWithParent = errors.New("IOBundle with empty assigngrp cannot have a parent") +// ErrParentAssigngrpMismatch describes an error where an IoBundle has a mismatch with the parentassigngrp +type ErrParentAssigngrpMismatch struct{} -var errCycleDetected = errors.New("Cycle detected, please check provided parentassigngrp/assigngrp") +func (ErrParentAssigngrpMismatch) Error() string { + return "IOBundle with parentassigngrp mismatch found" +} + +// ErrEmptyAssigngrpWithParent describes an error where an IoBundle without assigngrp has a parentassingrp +type ErrEmptyAssigngrpWithParent struct{} + +func (ErrEmptyAssigngrpWithParent) Error() string { + return "IOBundle with empty assigngrp cannot have a parent" +} + +// ErrCycleDetected describes an error where an IoBundle has cycles with parentassigngrp +type ErrCycleDetected struct{} + +func (ErrCycleDetected) Error() string { + return "Cycle detected, please check provided parentassigngrp/assigngrp" +} // CheckParentAssigngrp finds dependency loops between ioBundles and sets/clears the error func (aa *AssignableAdapters) CheckParentAssigngrp() bool { @@ -540,10 +586,10 @@ func (aa *AssignableAdapters) CheckParentAssigngrp() bool { for i := range aa.IoBundleList { ioBundle := &aa.IoBundleList[i] for _, parentAssigngrpErr := range []error{ - errOwnParent, - errParentAssigngrpMismatch, - errEmptyAssigngrpWithParent, - errCycleDetected, + ErrOwnParent{}, + ErrParentAssigngrpMismatch{}, + ErrEmptyAssigngrpWithParent{}, + ErrCycleDetected{}, } { ioBundle.Error.removeByType(parentAssigngrpErr) } @@ -554,17 +600,17 @@ func (aa *AssignableAdapters) CheckParentAssigngrp() bool { ioBundle := &aa.IoBundleList[i] if ioBundle.AssignmentGroup == ioBundle.ParentAssignmentGroup && ioBundle.AssignmentGroup != "" { - ioBundle.Error.Append(errOwnParent) + ioBundle.Error.Append(ErrOwnParent{}) return true } parentassigngrp, ok := assigngrp2parent[ioBundle.AssignmentGroup] if ok && parentassigngrp != ioBundle.ParentAssignmentGroup { - ioBundle.Error.Append(errParentAssigngrpMismatch) + ioBundle.Error.Append(ErrParentAssigngrpMismatch{}) return true } if ioBundle.AssignmentGroup == "" && ioBundle.ParentAssignmentGroup != "" { - ioBundle.Error.Append(errEmptyAssigngrpWithParent) + ioBundle.Error.Append(ErrEmptyAssigngrpWithParent{}) return true } assigngrp2parent[ioBundle.AssignmentGroup] = ioBundle.ParentAssignmentGroup @@ -598,34 +644,36 @@ func (aa *AssignableAdapters) CheckParentAssigngrp() bool { for i := range aa.IoBundleList { ioBundle := &aa.IoBundleList[i] if ioBundle.AssignmentGroup == cycleDetectedAssigngrp { - ioBundle.Error.Append(errCycleDetected) + ioBundle.Error.Append(ErrCycleDetected{}) } } return true } -type ioBundleCollision struct { - phylabel string - usbaddr string - usbproduct string - pcilong string - assigngrp string +// IOBundleCollision has the members IoBundles can collide on +type IOBundleCollision struct { + Phylabel string + USBAddr string + USBProduct string + PCILong string + Assigngrp string } -func (i ioBundleCollision) String() string { - return fmt.Sprintf("phylabel %s - usbaddr: %s usbproduct: %s pcilong: %s assigngrp: %s", i.phylabel, i.usbaddr, i.usbproduct, i.pcilong, i.assigngrp) +func (i IOBundleCollision) String() string { + return fmt.Sprintf("phylabel %s - usbaddr: %s usbproduct: %s pcilong: %s assigngrp: %s", i.Phylabel, i.USBAddr, i.USBProduct, i.PCILong, i.Assigngrp) } -type ioBundleCollisionErr struct { - collisions []ioBundleCollision +// ErrIOBundleCollision describes an error where an IoBundle collides with another IoBundle +type ErrIOBundleCollision struct { + Collisions []IOBundleCollision } -func (i ioBundleCollisionErr) Error() string { +func (i ErrIOBundleCollision) Error() string { collisionErrStrPrefix := "ioBundle collision:" - collisionStrs := make([]string, 0, len(i.collisions)) - for _, collision := range i.collisions { + collisionStrs := make([]string, 0, len(i.Collisions)) + for _, collision := range i.Collisions { collisionStrs = append(collisionStrs, collision.String()) } collisionErrStrBody := strings.Join(collisionStrs, "||") @@ -633,9 +681,9 @@ func (i ioBundleCollisionErr) Error() string { return fmt.Sprintf("%s||%s||", collisionErrStrPrefix, collisionErrStrBody) } -func newIoBundleCollisionErr() ioBundleCollisionErr { - return ioBundleCollisionErr{ - collisions: []ioBundleCollision{}, +func newIoBundleCollisionErr() ErrIOBundleCollision { + return ErrIOBundleCollision{ + Collisions: []IOBundleCollision{}, } } @@ -644,7 +692,7 @@ func (aa *AssignableAdapters) CheckBadUSBBundles() { usbProductsAddressMap := make(map[[4]string][]*IoBundle) for i := range aa.IoBundleList { ioBundle := &aa.IoBundleList[i] - ioBundle.Error.removeByType(ioBundleCollisionErr{}) + ioBundle.Error.removeByType(ErrIOBundleCollision{}) } for i := range aa.IoBundleList { @@ -668,12 +716,12 @@ func (aa *AssignableAdapters) CheckBadUSBBundles() { collisionErr := newIoBundleCollisionErr() for _, bundle := range bundles { - collisionErr.collisions = append(collisionErr.collisions, ioBundleCollision{ - phylabel: bundle.Phylabel, - usbaddr: bundle.UsbAddr, - usbproduct: bundle.UsbProduct, - pcilong: bundle.PciLong, - assigngrp: bundle.AssignmentGroup, + collisionErr.Collisions = append(collisionErr.Collisions, IOBundleCollision{ + Phylabel: bundle.Phylabel, + USBAddr: bundle.UsbAddr, + USBProduct: bundle.UsbProduct, + PCILong: bundle.PciLong, + Assigngrp: bundle.AssignmentGroup, }) } for _, bundle := range bundles { diff --git a/pkg/pillar/types/assignableadapters_test.go b/pkg/pillar/types/assignableadapters_test.go index 623c38fb846..f910fa89915 100644 --- a/pkg/pillar/types/assignableadapters_test.go +++ b/pkg/pillar/types/assignableadapters_test.go @@ -6,6 +6,7 @@ package types import ( "fmt" "testing" + "time" "github.com/google/go-cmp/cmp" zcommon "github.com/lf-edge/eve-api/go/evecommon" @@ -852,14 +853,14 @@ func (testErr2) Error() string { } func TestIoBundleError(t *testing.T) { - iobe := ioBundleError{} + iobe := IOBundleError{} iobe.Append(testErr1{}) - if !iobe.hasError(testErr1{}) { + if !iobe.HasErrorByType(testErr1{}) { t.Fatal("has not error testErr1") } - if iobe.hasError(testErr2{}) { + if iobe.HasErrorByType(testErr2{}) { t.Fatal("has error testErr2, but shouldn't") } @@ -880,7 +881,7 @@ func TestIoBundleError(t *testing.T) { if iobe.String() != "err2" { t.Fatalf("expected error string to be 'err2', but got '%s'", iobe.String()) } - if !iobe.hasError(testErr2{}) { + if !iobe.HasErrorByType(testErr2{}) { t.Fatal("has not error testErr2") } @@ -905,3 +906,33 @@ func TestIoBundleCmpable(t *testing.T) { cmp.Diff(io1, io2) } + +func TestIoBundleErrorRemove(t *testing.T) { + errs := []error{ + fmt.Errorf("some error"), + ErrOwnParent{}, + ErrParentAssigngrpMismatch{}, + ErrEmptyAssigngrpWithParent{}, + ErrCycleDetected{}, + newIoBundleCollisionErr(), + } + iob := IoBundle{ + Error: IOBundleError{ + TimeOfError: time.Time{}, + }, + } + + for _, err := range errs { + iob.Error.Append(err) + } + + iob.Error.removeByType(ErrOwnParent{}) + + if len(iob.Error.Errors) != 5 { + for _, err := range iob.Error.Errors { + t.Logf("\t- %s -- %v", err.TypeStr, err) + } + + t.Fatalf("expected only 5 errors, but got %d", len(iob.Error.Errors)) + } +}